Correctly align messages in right to left languages #5453
Conversation
Signed-off-by: Aaron Raimist <[email protected]>
res/css/views/rooms/_EventTile.scss
Outdated
| @@ -297,6 +297,8 @@ $left-gutter: 64px; | |||
|
|
|||
| /* De-zalgoing */ | |||
| .mx_EventTile_body { | |||
| text-align: start; | |||
| display: block; | |||
There was a problem hiding this comment.
Rather than changing mx_EventTile_body elements (which are spans) to display: block, it would be more straightforward to instead make them divs and get this for free.
It would also make sense to me to change mx_EventTile_content elements to divs as well, and remove their display: block style, so we are consistently using div vs. span for their intended meaning.
There was a problem hiding this comment.
Okay I've changed them over to divs. This mostly works except it breaks the layout for edited messages and /me messages. I'm not sure how to fix this.
There was a problem hiding this comment.
Ah, hmm... 😖 Okay, I'll have to think about how we'd want to handle the edited case.
There was a problem hiding this comment.
what is the issue with edited case?
There was a problem hiding this comment.
See the screenshot above. The (edited) label is in the wrong place.
There was a problem hiding this comment.
why not set its display to block and then text-align to start?
Signed-off-by: Aaron Raimist <[email protected]>
Signed-off-by: Aaron Raimist <[email protected]>
|
It is needed to add |
|
Yes that already exists. The problem is it doesn’t do anything on a span element. |
I saw it just now on element desktop. I just modified the css with dev tools and the initial result is fine: .mx_EventTile_body {
overflow-y: hidden; /* already exists */
text-align: start;
display: block;
}
|
|
I have noticed issue with lists which can be resolved using this css: result:
|
|
Quotation fix: .markdown-body blockquote {
margin: 0 0 16px;
padding: 0 15px;
color: #777;
/* border-left: 4px solid #ddd; */
border-inline-start: 4px solid #ddd;
}before:
after:
|
|
This is still in my mental queue to think about, but it may take a bit of time / some conversations with Design to work out exactly what we want here. |
|
Let me know if I can help. |
|
What is the update? What is preventing this PR to be finalized and merged? |
|
I still need to work out an approach that handles edited markers and such, and then review that with Design. I haven't had time to do that yet. |
May you share what specific issue is there? One (maybe me) might help |
|
I have unfortunately failed to find the proper time to think about this... 😓 I'll send this back to the general queue in hopes that it might lead to new input here. |
May you please share what is there to think? Share the issue so that others might be able to offer some help. |
|
@ahangarha its already in a thread above. |
|
I'm obviously not familiar with RTL languages, but would it be awful to keep the |
If this is the only remaining issue, I will share a solution. Just please list all the remaining issue. |
|
@turt2live My understanding is ideally it would be on the left but if we could at least get the message oriented correctly and have the (edited) in the wrong spot that would still be much better than how it currently works. |
|
At the moment, the app currently (without the change here) shows the "(edited)" label inline with the message text. The "easy" fix to apply RTL to messages also causes them to become a block elements, which moves the "(edited)" label to the next line. I believe the main options to move forward are either:
|
|
As far as I see, it can be tweaked to follow bidi algorithm. So, if it is Latin, it remains on left and if gets translated to Persian (or other RTL languages) it moves to right. I think this is the right behavior. If you want to force it to follow the direction of the related text, it is doable but there should be significant change in the layout. Yet to be frank, this is a minor issue as compared to the situation we (RTL people) face with. I would prefer to get the current improved version and then in next version improve this little minor issue. Is there any build based on the current modifications so that I can check different scenarios? |
See the deploy preview, you can find the latest link in the Checks section of the PR. |
How much wrong is that place? It is just going to the next line. Is it a big issue? bigger than messing up with mixed RTL/LTR text? I don't think so. And frankly I think we should move that entirely out of that place but that is a different issue. The fact is that Element has a big issue when it comes to RTL text. We can fix this big issue which would produce a little visual issue for only edited message. I think logic says lets fix the bigger problem now and fix the smaller later. We shouldn't let the smaller issue prevent us from fixing the bigger issue. Am I wrong? |
|
I completly agree with @ahangarha in this case. Making Element usable in BiDi is more important than a minor visual issue. Anyway I think the current edited sign behaviour is already a bad UX decision and we should find another approach for indicating a message is edited. |
|
As a RTL user, if this "edited" sign thing is the only reason that prevents me to use my language in messages properly, I really don't care about it.So I totally agree with @ahangarha and @danialbehzadi . |
|
@ahangarha @danialbehzadi @SeedPuller Since a couple of days I was trying to edit styles to make UI compatible with RTL languages. Please check element-hq/element-web#14520 (comment) for a short video of UI in Hebrew (as UI is not available in languages of Arabic alphabet yet). |
|
Thank you for your work but the issue is not to make overall layout RTL. We need it for sure but we need to show messages in correct direction regardless of the global language and direction.
در ۳ آوریل ۲۰۲۲ ۹:۵۴:۵۶ (GMT+03:00)، Suguru Hirahara ***@***.***> نوشت:
***@***.*** @danialbehzadi @SeedPuller Since a couple of days I was trying to edit styles to make UI compatible with RTL languages. Please check element-hq/element-web#14520 (comment) for a short video of UI in Hebrew (as UI is not available in languages of Arabic alphabet yet).
…
--
Reply to this email directly or view it on GitHub:
#5453 (comment)
You are receiving this because you were mentioned.
Message ID: ***@***.***>
|
|
@ahangarha From my understanding
Mind the place of the two |
|
By breaking I think they mean it is not inline anymore. To me it is not a problem at all and even if it is, it is not something to block such an important change. |
It is going to be taken care of 😉 |
|
Hello |
MadLittleMods
added
Z-Community-PR
|
@luixxiul do you have a PR that supercedes this one, or are you working on this? |
|
Right now, not really working on this. |
|
This came up in our weekly review as the oldest open non-draft PR! If someone wanted to help it get over the line there would be kudos! |
|
This PR adds potentially really valuable functionality to Element Web, but it is currently blocked because it breaks layout in non-RTL contexts (see this comment. It also need updating to resolve merge conflicts with the latest code. Is anyone willing to step up to fix the remaining problems so we can merge this PR? If not, I will convert this to draft to indicate that it is not ready, and not under active development. Lots of people will thank you if you give this a try! |
|
Marking this as draft because it needs changes and no-one is actively working on it. If you want to work on it, please do, and mark as Ready for Review when you have address the changes needed. Feel free to discuss what is needed here or in #element-dev:matrix.org Matrix room. |
If you check the history, there has been people who wanted to help. There are many questions about the importance of the break and almost there is no answer to it. A big bug is not being fixed because it introduces another much much smaller bug. I would suggest to set a meeting (or some other effective way of communication) to discuss this issue. |
|
Hi @ahangarha , thank you for your patience with this! ahangarha:
My understanding of the situation is that this change introduces a bug. Quoting aaronraimist:
As for how important it is: the answer is "important enough to need fixing before we can merge this change".
I'm not sure whether that is needed here: if someone fixes the bug (and brings this change up-to-date, which I'm afraid will probably be harder) then we will review it and expect to merge it. |
|
Sounds good. Though I think it is more than my current knowledge, but I try to see if I can fix the conflict. I might be able to do it on this weekend. Let's see. At least I try :) |
|
Awesome, good luck! |
|
I made a quick implementation at #9026. Feedback for https://pr9026--matrix-react-sdk.netlify.app/ is appreciated. |
Inspired by #5453 but hopefully with the edited marker in the right place. This is a PoC: types aren't correct and the style needs pulling out to a class. Plus it would probably need more visual tests added. If this looks acceptable, I can make these changes.
|
I've had a go at a version of this PR that keeps the 'edited' marker in the correct place at #12837 |
|
#12837 is merged which should fix this! 🤞 |
* Fix alignment of RTL messages Inspired by #5453 but hopefully with the edited marker in the right place. This is a PoC: types aren't correct and the style needs pulling out to a class. Plus it would probably need more visual tests added. If this looks acceptable, I can make these changes. * Fix spacing between text and edited annotation * Update snapshot * Update more snapshots * More snapshots * More more snapshots * Split out style * Fix emotes This will cause them always be right-justified if the display name is rtl. * Add playwright test for ltr/rtl message rendering * Better snapshots * Await on message sending * Better waiting, hopefully * Old snapshot files * Really hopefully fixed screenshots this time * Don't include the message action bar in the screenshots
Fixes element-hq/element-web#4771
Here's what your changelog entry will look like:
🐛 Bug Fixes