Fix RTL message alignment and add RTL regression stories#7821
Closed
sari-devv wants to merge 1 commit intosignalapp:mainfrom
Closed
Fix RTL message alignment and add RTL regression stories#7821sari-devv wants to merge 1 commit intosignalapp:mainfrom
sari-devv wants to merge 1 commit intosignalapp:mainfrom
Conversation
Fixes issue signalapp#7819 What: - Removed hardcoded `dir="ltr"` from the `.module-message__text` container in `Message.dom.tsx`. - Updated Storybook `createProps` to allow `i18n` overrides. - Added `MixedRtlAndLtrMessage` and `DeletedRtl` stories to `TimelineMessage.dom.stories.tsx`. Why: The hardcoded `dir="ltr"` attribute on the message text wrapper forced a Left-To-Right context regardless of the actual message content. Since the container uses `text-align: start`, this caused RTL messages (like Arabic or Hebrew) to incorrectly align to the left side of the bubble and broke the rendering of bidirectional punctuation. This attribute was originally added to stabilize the "Delete for everyone" layout icon positioning. However, since those strings are now correctly wrapped in `<span dir="auto">`, the hardcoded attribute on the parent is no longer necessary and was causing wide-spread RTL regressions. Technical Details: - Removing the hardcoded attribute allows the message text to properly inherit the directionality calculated by the message selectors. - The `DeletedRtl` story was added to verify that "Delete for everyone" remains stable in RTL contexts. It dynamically imports Arabic strings from `_locales/ar/messages.json` and mocks the `i18n.getIntl()` interface to simulate the production environment's usage of the `<I18n>` component. This ensures that the layout fix is verified against actual RTL text and character-order rules in automated visual tests.
|
so... what are the plans regarding this regression? this bug is now landing in production for many users, greatly reducing usability for anyone of the half a billion RTL language speakers, with switching to the LTR interface not improving the situation... |
|
This needs to be pushed ASAP. |
Contributor
|
Thank you for opening the PR and correctly identifying the issue. We ended up fixing just the "ltr" attribute, and will consider adding extra storybook as you've suggested or automated visual tests to detect RTL regressions in the future. |
Author
|
My pleasure! Do I need to close this PR now? |
Contributor
|
For now I'll close it. If we end up using the storybook updates I'll let you know. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First time contributor checklist:
Contributor checklist:
mainbranchpnpm run readyrun passes successfully (more about tests here)Description
Fixes issue #7819:
This PR restores correct Right-To-Left (RTL) message alignment in the conversation view. A previous commit accidentally hardcoded
dir="ltr"on the inner message text container, which overrode the dynamic directionality for Arabic and Hebrew messages.What:
dir="ltr"from the.module-message__textcontainer inMessage.dom.tsx.createPropsto allowi18noverrides.MixedRtlAndLtrMessageandDeletedRtlstories toTimelineMessage.dom.stories.tsx.Why:
The hard-coded
dir="ltr"attribute on the message text wrapper forced a Left-To-Right context regardless of the actual message content. Since the container usestext-align: start, this caused RTL messages (like Arabic or Hebrew) to incorrectly align to the left side of the bubble and broke the rendering of bidirectional punctuation.This attribute was originally added to stabilize the "Delete for everyone" layout icon positioning. However, since those strings are now correctly wrapped in
<span dir="auto">, the hard-coded attribute on the parent is no longer necessary and was causing wide-spread RTL regressions.Technical Details:
OS: MacOS, but the testing was done on Storybook.
DeletedRtlstory was added to verify that "Delete for everyone" remains stable in RTL contexts. It dynamically imports Arabic strings from_locales/ar/messages.jsonand mocks thei18n.getIntl()interface to simulate the production environment's usage of the<I18n>component. This ensures that the layout fix is verified against actual RTL text and character-order rules in automated visual tests.Before vs After Visual Comparison
Before
After