Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP - Add bidirectional text support #42471

Closed
wants to merge 12 commits into from

Conversation

ahangarha
Copy link
Contributor

@ahangarha ahangarha commented Dec 23, 2023

Summary

This PR is the first step towards adding bidirectional text support to the NextCloud ecosystem. It affects the entire project; accordingly, each app should have subsequent PRs.

The PR is 90+ percent complete. I decided to make the PR to

  • Get early feedback to ensure it is on the right path.
  • Discuss some technical challenges.

I had challenges in building the project. Generating css files and the dist directory during development is something I am uncomfortable with.

Because of supporting Samsung Internet, I needed to take some hacky approach.

Note:
We must update many components in NextCloud/vue. The most important one was the header-menu vue component which is already updated. Others will come soon. First, I need to ensure this PR is getting prepared for merge.

Screenshots

Before After
image image
image image

TODO

Checklist

@ahangarha
Copy link
Contributor Author

@nickvergessen May you give me some feedback before I touch other files?

@ahangarha
Copy link
Contributor Author

To make it easier to review such PRs and create a roadmap for bidirectional text support in the entire NextCloud ecosystem, I have proposed to form a kind of working group here: Working Group for Adding Bidirectional Text Support.

@ahangarha ahangarha force-pushed the add-bidi-support branch 2 times, most recently from 0d5734e to 6cbe6f4 Compare January 2, 2024 12:52
@ahangarha
Copy link
Contributor Author

ahangarha commented Jan 2, 2024

@joshtrichards Thanks for updating labels.

The development is blocked for now because I need some feedback. Also for the mentioned items in my todo list, I need someone with a better understanding of the ecosystem to make decision. Should I rename CSS classes? Should we keep the class names but change the behavior for now? What about tooltips?

If we make decision, I can move forward.

We need more discussion, particularly about the roadmap for apps to adapt.

If I need to contact any particular person, please let me know. Better to finish this PR sooner and start working on other parts.

ghorbani-ali and others added 7 commits January 10, 2024 22:01
Signed-off-by: ali ghorbani <ghorbani.ali.developer@gmail.com>
Signed-off-by: Mostafa Ahangarha <ahangarha@riseup.net>
Signed-off-by: ali ghorbani <ghorbani.ali.developer@gmail.com>
Signed-off-by: Mostafa Ahangarha <ahangarha@riseup.net>
Signed-off-by: Mostafa Ahangarha <ahangarha@riseup.net>
Signed-off-by: ali ghorbani <ghorbani.ali.developer@gmail.com>
Signed-off-by: Mostafa Ahangarha <ahangarha@riseup.net>
@ahangarha
Copy link
Contributor Author

@miaulalala Thanks for your update.

There are cases of changing styles or structures because of refactoring. This PR should address them before considering a merge.

At this stage, I think the best thing to do is to ensure we are all on the same page.

I am looking forward to receiving updates.

@alimahwer
Copy link

I'm very interested in accomplishing the RTL and BiDi support for Nextcloud and I wish I had the required experience to help on this issue. However, I can help preview the result in the Arabic language as it is my mother's thong.

@ghorbani-ali

This comment was marked as off-topic.

@alimahwer

This comment was marked as resolved.

@jancborchardt
Copy link
Member

jancborchardt commented Mar 29, 2024

Should I rename CSS classes? Should we keep the class names but change the behavior for now? What about tooltips?

This sounds more like a separate technical debt issue than something that is a blocker here, right @ahangarha? :) (yes they could be changed, but can be done in a follow-up)

Design-wise your pull request looks good, I think the approval needs to come from frontend/server rather.

@AndyScherzinger @sorbaugh

@AndyScherzinger
Copy link
Member

Design-wise your pull request looks good, I think the approval needs to come from frontend/server rather.

Fine by me from what I read, like you said @jancborchardt it needs some frontend/server feedback freom @sorbaugh and team / community.

As for the questions/issues

Should I rename CSS classes? Should we keep the class names but change the behavior for now? What about tooltips?

I'd like to get some feedback from developers specifically folks developing apps, for example but not limited to @nickvergessen @ChristophWurst @st3iny @susnux @skjnldsv @juliushaertl @julien-nc @artonge

My personal, non-expert view would be to

  • keep the existing one, but create new ones with the new behavior and mark the old ones deprecated via a comment.
  • tooltips should be done native by now, at least in the core implementation they are to be accessible since any lib used before wasn't.

This would be the least breaking way even though non-adapting apps wouldn't support RTL then for the moment until the move to the new classes. But I think this would be the least invasive way to introduce RTL support for the platform.

Does this cover all your points @ahangarha ?

@susnux
Copy link
Contributor

susnux commented Mar 29, 2024

I agree with @AndyScherzinger

Tooltips should be done using the native title prop nowadays, for the classes I would add new one (start instead of left and end instead of right (or inline-...)). And deprecate the old ones.

@ahangarha
Copy link
Contributor Author

Thank you all for sharing your thoughts.

I agree that we must work closely with the design and front-end team. I believe there are some complexities that require to have a clear roadmap to implement such a feature. I believe that this is needed to have a smooth transition with the minimum issues introduced to the existing users.

I already proposed the formation of a Working Group for Adding Bidirectional Text Support.

I try to be available for further discussion with the team.

@skjnldsv skjnldsv added this to the Nextcloud 30 milestone Apr 2, 2024
@DorraJaouad
Copy link

DorraJaouad commented Apr 2, 2024

@ahangarha

Given that I am one of nextcloud front-end developers and native arabic speaker, I would like to share my point of view on the above:

  • For classes renaming, I agree as mentioned above to keep the current ones as deprecated and create new ones because some apps might rely on them thus a high chance to break things.
  • As for RTL text support, I would recommend to set the RTL UI if a RTL language is selected. RTL support per text level can lead to a strange layout especially if both LTR and RTL languages are used at the same time ( like in Talk, user A writes in LTR language and user B writes in RTL language or if user status for example is set in a different language). Messages and manually-set user status are not translated to the selected language.

Indeed, there might be other challenges regarding the implementation and if you could list the ones you have found here, we are happy to discuss them.

Regarding RTL support in general, it will be a long process but doable. Using the selected language, we can modify the layout to adapt to RTL reading, like switching left-side bar to the right or icons to the right etc.. If you are looking for specific information on common styles/behavior we are following, I can help narrow down the scope to those that are relevant.

@susnux
Copy link
Contributor

susnux commented Apr 2, 2024

  • I would recommend to set the RTL UI if a RLT language is selected.

Just for information: We have finally a method to figure out if RTL is used since last l10n release :)
@nextcloud/l10n: isRTL()

@ahangarha
Copy link
Contributor Author

@DorraJaouad Thanks for your reply and it is great to have a person in the core team who understands RTL languages.

RTL support per text level can lead to a strange layout especially if both LTR and RTL languages are used at the same time ( like in Talk, user A writes in LTR language and user B writes in RTL language or if user status for example is set in a different language).

That is the whole point of me pushing for bidirectional text support rather enforcing direction globally. There are many cases I can list where we have mixed content with RTL and LTR text. One is in the chat. The other is in the apps like note where one may need to not only create a document with mixed content, but also create multiple documents, each with different language. Another case is in apps like Deck where having mixed content is inevitable. We may consider the fact that the same text might be seen by people who use different languages for their NC. The content should render in correct direction regardless of the selected language.

In all cases, the direction of each text should be correct. But I agree that sometimes for UX sake, we may enforce text alignment to be set to left or right.

But I understand your concern. And if it was that straight forward to address issues, we would have fixed this issue long time ago. This is why I need to work closely with the core and specially the design team. We need to ensure that this gets implemented properly.

We have finally a method to figure out if RTL is used since last l10n release

I think the new implementation is better because we set the global direction in the body tag. This way, only in the backend we need to make a list for RTL lanaugages, so we wont need to be worried about keeping the list in sync between FE and BE.

@nickvergessen
Copy link
Member

Hi @ahangarha

Thanks for all the work and thoughts already. Some comments from organisational point of view:

  • I invited you to our GitHub Org, so you can send the PRs from branches of our repository, so GitHub actions work properly and don't need manual approving

  • We also try to coordinate reviews and we think the best way to get this in would be "component" by component, to reduce conflicts with other work taking place which breaks/conflicts the compiled assets all the time. We would assign some people that review the PRs regularly and make sure to merge them in timely fashion. Our proposal would be:

    • You send a PR touching a single app / area only
    • We assign someone to review and merge it within the next two working days
    • Once you see it's done, you can send the next chunk

    Starting with the current diff (replacing left/right on margins with inline-start/inline-end kind of things should be quick to review and can be done even without any conflict or dependency onto the actual providing RTL, (but it helps with your testing of course).

  • At some point we can take care of populating the actual "this language is RTL" logic in the PHP code

  • If you send me an email to <my github nickname>@nextcloud.com with an email address, I would add a guest account on our instance, so we can also align via chat and also have options for quicker/ad hoc calls and chats to discuss the approach

@ahangarha
Copy link
Contributor Author

@nickvergessen

I totally agree with you. Your suggestion is very practical, making it very easy to move forward.

I try to plan to work on this issue very soon again.

Thanks for making it easy to work on this feature.

@liorkesos
Copy link

+1 on the need for proper RTL.
So glad this seems to be gaining momentum!

@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
@juliushaertl juliushaertl removed their request for review August 16, 2024 18:42
@ahangarha
Copy link
Contributor Author

@nickvergessen
I am going to start working on this issue in 10 days.
Please let me know how best I can coordinate with the team to make things move as smooth and fast as possible.

@AndyScherzinger
Copy link
Member

@ahangarha sounds great. I created a Talk room on our Nextcloud instance where you are already having a guest account for any adhoc discussions where Github might not be the right tool and to have a short more chatty communication channel where comments would just polute a PR or Github issue.

@alimahwer
Copy link

@ahangarha sounds great. I created a Talk room on our Nextcloud instance where you are already having a guest account for any adhoc discussions where Github might not be the right tool and to have a short more chatty communication channel where comments would just polute a PR or Github issue.

I'm interested as well. Can I join this chat room?

@AndyScherzinger
Copy link
Member

@ahangarha I took the liberty to go through the pain of rebasing and resolving the conflicts and opened a new PR with a branch from Nextcloud's repository where you have push right at #47343

I would argue for using a in-repo branch which makes life a lot easier in terms of keeping up with the master branch and to have to ability to run all CI actions

I did remove any compiled assets from the original commits that created conflicts. So it needs a re-compile.

@AndyScherzinger
Copy link
Member

I'm interested as well. Can I join this chat room?

@alimahwer sure thing. Do you already have a guest account on that server?

@alimahwer
Copy link

I'm interested as well. Can I join this chat room?

@alimahwer sure thing. Do you already have a guest account on that server?

Not yet. could you pls provide me with the sign-up link.

@AndyScherzinger
Copy link
Member

AndyScherzinger commented Oct 1, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RTL (Right-to-Left) text direction for some Asian languages such as Arabic, Hebrew, Persian etc.