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

fix: Automate text direction based on the language in messages #29367

Merged
merged 8 commits into from
Jan 11, 2024

Conversation

anefzaoui
Copy link
Contributor

@anefzaoui anefzaoui commented May 25, 2023

Proposed changes (including videos or screenshots)

This pull request resolves the issue #29365. It ensures that the dir attribute for message boxes is set to "auto", and dir="auto" attribute is added to any div with data-qa-type="message-body". This ensures proper handling of text direction for languages that require RTL orientation.

Issue(s)

Fixes #29365

Steps to test or reproduce

Please follow the steps outlined in the issue to test the changes.

Further comments

This change ensures that the Rocket.Chat application correctly handles the text direction for languages that require RTL orientation, improving overall accessibility and usability. I'm not so sure about the correct tests to cover this?

@anefzaoui anefzaoui requested a review from a team as a code owner May 25, 2023 15:00
@changeset-bot
Copy link

changeset-bot bot commented May 25, 2023

🦋 Changeset detected

Latest commit: 16bdce7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 30 packages
Name Type
@rocket.chat/meteor Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/gazzodown Patch
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/api-client Patch
@rocket.chat/license Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/ddp-client Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/models Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/instance-status Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@anefzaoui
Copy link
Contributor Author

Can someone please look at this? This is fairly simple although I'm not sure how to get this to RocketChat's attention? Cc @juliajforesti :)

@anefzaoui
Copy link
Contributor Author

Can someone please look at this? We really need it
@rodrigok @sampaiodiego @engelgabriel @ggazzo @marceloschmidt @MartinSchoeler
Thank you!

@dougfabris
Copy link
Member

dougfabris commented Jan 4, 2024

@anefzaoui Thanks for the contribution!
The entire UI is switched to right to left when using an RTL language in your preference

Screenshot 2024-01-04 at 18 02 57

Sorry for the ignorance, but in your opinion, we should align the messages on the right even using an LTR language?

@anefzaoui
Copy link
Contributor Author

Hi @dougfabris

The dir="auto" attribute is essential for handling mixed language content. It's not about UI design or text alignment per se, instead, it ensures that words in a sentence with both Left-to-Right (LTR like English, French) and Right-to-Left (RTL like Arabic and Hebrew) languages are displayed in their correct order. This attribute lets the browser automatically decide the text direction based on the content, maintaining readability regardless of the overall UI language.

Pain points when using a forced or inherited dir

  1. Mixed Language Sentence (Forced LTR)

    • Scenario: A sentence that combines English (LTR) and Arabic (RTL) words, but the container is forced to LTR.
    • Example: <div dir="ltr">This is a test مرحبا بكم</div>
    • Issue: The Arabic phrase "مرحبا بكم" (meaning "Welcome") will be displayed in a left-to-right order, disrupting the natural reading flow. The words get jumbled, making the sentence confusing and difficult to read.
  2. Inherited Directionality

    • Scenario: A nested element containing Arabic text inherits the LTR direction from its parent.
    • Example:
      <div dir="ltr">
        Welcome to our site
        <span>مرحبا بكم</span>
      </div>
    • Issue: The Arabic phrase, being in an LTR context due to inheritance, will not display correctly. The phrase will be rendered in reverse, leading to a nonsensical and hard-to-read text.

Correct behavior when using dir="auto"

  • Mixed Language Content
    • Scenario: A sentence that mixes English and Arabic words.
    • Example: <div dir="auto">This is a test مرحبا بكم</div>
    • Benefit: With dir="auto", the browser automatically detects and applies the correct direction for each part of the sentence. English is displayed LTR, and Arabic is displayed RTL. This ensures that both parts of the sentence are easy to read and appear in their natural order, regardless of the surrounding text or UI direction.

Using dir="auto" addresses these pain points by allowing each language segment to be displayed in its natural reading direction, significantly improving the user experience in multilingual environments.

I hope this answered your question with as much details as possible. I was the developer responsible for implementing proper RTL UIs in the now-deprecated "Mozilla's Firefox OS" project, if you need to schedule a call or a proper discussion board to explain this further just let me know.

Thanks!

@dougfabris
Copy link
Member

@anefzaoui Thank you so much for your explanation, I will share the content you bring to us with the design team and will think about how we can add some tests in the PR and guarantee the issue doesn't happen in future changes

@dougfabris
Copy link
Member

@anefzaoui In a discussion with the team we thought we should add this rule for our message composer and our text inputs as well, what do you think? There are other places we should have this rule in your opinion besides the messages?

@anefzaoui
Copy link
Contributor Author

@anefzaoui Thank you so much for your explanation, I will share the content you bring to us with the design team and will think about how we can add some tests in the PR and guarantee the issue doesn't happen in future changes

Thanks!

@anefzaoui In a discussion with the team we thought we should add this rule for our message composer and our text inputs as well, what do you think? There are other places we should have this rule in your opinion besides the messages?

Basically this is needed in any place where a user needs to type something & anywhere where users are expected to input text, since the user can type and combination of words.

Copy link

codecov bot commented Jan 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (116ad55) 49.56% compared to head (16bdce7) 58.94%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #29367      +/-   ##
===========================================
+ Coverage    49.56%   58.94%   +9.37%     
===========================================
  Files         3309     1780    -1529     
  Lines        81387    34447   -46940     
  Branches     16693     7162    -9531     
===========================================
- Hits         40338    20304   -20034     
+ Misses       36348    12587   -23761     
+ Partials      4701     1556    -3145     
Flag Coverage Δ
e2e 51.85% <ø> (-1.30%) ⬇️
e2e-api ?
unit 76.54% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@dougfabris dougfabris changed the title fix: Message Boxes Text Direction Issue for RTL and LTR fix: Automate text direction based on the language in messages Jan 11, 2024
Copy link
Member

@dougfabris dougfabris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, as your PR is targeted to messages, let's merge it. Feel free to help us to fix the missing places, but I will share this need with the team as well. Thanks a lot!

@dionisio-bot dionisio-bot bot added stat: ready to merge PR tested and approved waiting for merge and removed stat: needs QA labels Jan 11, 2024
@dionisio-bot dionisio-bot bot removed the stat: ready to merge PR tested and approved waiting for merge label Jan 11, 2024
@dougfabris dougfabris added the stat: ready to merge PR tested and approved waiting for merge label Jan 11, 2024
@kodiakhq kodiakhq bot merged commit 2f8c98f into RocketChat:develop Jan 11, 2024
42 checks passed
@anefzaoui
Copy link
Contributor Author

@dougfabris thanks!! Will do, however is there like a chat server for volunteers or a proper way to notify and recommend such features? if such is available then discussions can be much more detailed and clear than in the comments.

@dougfabris
Copy link
Member

@anefzaoui You can join us on the Open Server, in the #support channel you can raise a question if needed, also we do have a Community Forum where you can search/create a post. For feature request we do have a repository for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat: QA skipped stat: ready to merge PR tested and approved waiting for merge
Projects
Development

Successfully merging this pull request may close these issues.

Text Direction Issue: Message Boxes Do Not Handle Mixed RTL and LTR Text Correctly
2 participants