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

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

ahangarha
Copy link

@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
Author

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

@ahangarha
Copy link
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
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>
Signed-off-by: Mostafa Ahangarha <ahangarha@riseup.net>
Signed-off-by: Mostafa Ahangarha <ahangarha@riseup.net>
Signed-off-by: Mostafa Ahangarha <ahangarha@riseup.net>
Signed-off-by: Mostafa Ahangarha <ahangarha@riseup.net>
Signed-off-by: Mostafa Ahangarha <ahangarha@riseup.net>
@ahangarha
Copy link
Author

Given the recent refactorings, it is hard for me to regularly rebase these changes.

I am committed to finishing this issue, not only for this PR but to add bidi support to the whole NextCloud ecosystem, but that cannot be done without some level of mutual understanding and coordination between me and the development team.

I hold on till I get some response.

Again I invite you to consider the formation of a Working Group for Adding Bidirectional Text Support.

CC: @ChristophWurst @miaulalala @nickvergessen @st3iny

@nickvergessen
Copy link
Member

I had forwarded your PR and issue already and you should get a response soon.

There is no need to live update your PR every day/time the button is shown.

@miaulalala
Copy link
Contributor

Given the recent refactorings, it is hard for me to regularly rebase these changes.

I am committed to finishing this issue, not only for this PR but to add bidi support to the whole NextCloud ecosystem, but that cannot be done without some level of mutual understanding and coordination between me and the development team.

I hold on till I get some response.

Again I invite you to consider the formation of a Working Group for Adding Bidirectional Text Support.

CC: @ChristophWurst @miaulalala @nickvergessen @st3iny

Hey, thanks for all your hard work on adding support for RTL text! We really apprechiate it!

We're currently waiting on our designers to give some feedback on this. The team leads here and in other apps have taken note, so your PRs aren't being ignored.

As for having to rebase the branch all the time, as Joas said, you don't have to. You can keep on working on your stuff and when you're done, you can manually rebase on the most current version of main then. It's not really much use resolving the conflicts now.

@ahangarha
Copy link
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
Copy link

@miaulalala Hi.
Is it possible to get an update on the decisions?

@alimahwer
Copy link

Any updates? Any expected deadlines for this project?

@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
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
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
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!

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.