1915867 - [RTL][Menu redesign] Recommended extensions' LTR descriptions are cut off from the beginning

Status: RESOLVED FIXED

(Fenix :: WebExtensions, defect, P2)

Description

[Itiel]

Attachment: Screenshot

See attached screenshot, the beginning of the extensions descriptions are cut off, with extra space on the right.
I assume there's a non-logical-property margin/padding somewhere?

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:zmckenney, could you have a look please?

For more information, please visit BugBot documentation.

Comment 2

[Iorga Gabriel]

I think the problem here is that the descriptions are in English. This is the reason that they are cut from the right.

Comment 3

[William Durand [:willdurand]]

(In reply to Iorga Gabriel from comment #2)

I think the problem here is that the descriptions are in English. This is the reason that they are cut from the right.

It looks like we're currently letting Android/Compose decide on the content direction but that might not be what we want. What is the rationale for being content-dependent instead of following how the OS is configured?

Comment 4

[Zac McKenney [:zmckenney]]

We want English descriptions to be cut from the right because it doesn't make sense to cut them from the left. We also want English to be written from left to right and that's how you are supposed to display and cut it in Android. I want to circle this conversation back to the original comment with the most important part of the ticket:
the beginning of the extensions descriptions are cut off, with extra space on the right

The real requirement here is making sure the beginning of the English descriptions are visible and I'm also guessing that a padding was set to paddingLeft instead of paddingStart

Comment 5

[Iorga Gabriel]

We don't have paddingLeft in Compose. All the layout is done in Compose. The problem is that language text is different from the layout direction language of the phone. If all the text is in Hebrew, we don't have this problem. The description of Dark Reader is in Hebrew, and it looks correct.

Comment 6

[William Durand [:willdurand]]

Attachment: after-rtl.png

Thanks for the explanations. Desktop does it a bit differently IIRC.

The real requirement here is making sure the beginning of the English descriptions are visible and I'm also guessing that a padding was set to paddingLeft instead of paddingStart

It doesn't look like it's the case (or it isn't obvious). From what I can see, that has something to do with the fact that add-ons are eventually instances of the ListItem, and we might not be applied the correct style OR - at least - Compose isn't able to automatically determine the right text positioning.

A way to achieve this would be the following. Per Zac, we want to respect the direction based on the content, so we could tell Compose to style the description accordingly (we might want to do the same for extension names - I haven't checked):

diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/compose/list/ListItem.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/compose/list/ListItem.kt
index f3e9616006544..bc7df5258682a 100644
--- a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/compose/list/ListItem.kt
+++ b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/compose/list/ListItem.kt
@@ -44,6 +44,8 @@ import androidx.compose.ui.semantics.clearAndSetSemantics
 import androidx.compose.ui.semantics.role
 import androidx.compose.ui.semantics.selected
 import androidx.compose.ui.semantics.semantics
+import androidx.compose.ui.text.TextStyle
+import androidx.compose.ui.text.style.TextDirection
 import androidx.compose.ui.text.style.TextOverflow
 import androidx.compose.ui.tooling.preview.Preview
 import androidx.compose.ui.unit.Dp
@@ -805,7 +807,9 @@ private fun ListItem(
                     color = if (enabled) descriptionTextColor else FirefoxTheme.colors.textDisabled,
                     overflow = TextOverflow.Ellipsis,
                     maxLines = maxDescriptionLines,
-                    style = FirefoxTheme.typography.body2,
+                    style = FirefoxTheme.typography.body2.merge(
+                        TextStyle(textDirection = TextDirection.Content),
+                    ),
                 )
             }
         }

I attached a screenshot with the result.

:itiel, is that what you'd expect?

Comment 7

[William Durand [:willdurand]]

Attachment: after-ltr.png

Note that LTR wouldn't be affected by the sketch of patch in Comment 6.

Comment 8

[Mihai Adrian Carare [:mcarare]]

This is likely caused by an API issue: https://issuetracker.google.com/issues/272989440

The solution would be either using a typography without letter spacing or not using Ellipsis for text overflow.

Setting TextDirection based on content could also work, not sure if it would be a problem with a mix of translated and not translated descriptions.

As a note, the main menu also has this problem.

Also important: a properly translated string would not have this problem.

Comment 9

[William Durand [:willdurand]]

(In reply to Mihai Adrian Carare [:mcarare] from comment #8)

Also important: a properly translated string would not have this problem.

Extension descriptions are supplied by developers. We cannot expect translations in all locales so while the main menu has this problem, it's probably fine to wait until the upstream bug is fixed for it, but for this specific bug, we should probably do something. I don't know if my suggestion is the right one, but there is surely something to do. No ellipsis might look bad, I don't know about letter spacing. WDYT?

Comment 10

[Mihai Adrian Carare [:mcarare]]

IMO, no letter spacing does not make much visual difference, but I think that's something UX should decide on.

Comment 11

[Itiel]

(In reply to William Durand [:willdurand] from comment #6)

I attached a screenshot with the result.

:itiel, is that what you'd expect?

Yes, that looks great.

Comment 12

[William Durand [:willdurand]]

Attachment: Bug 1915867 - Fix recommended extensions' LTR descriptions in RTL mode. r?mcarare!

Comment 13

[William Durand [:willdurand]]

I submitted a patch with the textDirection, which is a good trade-off right now. It isn't perfect but it is also not changing the rendering of the text, so side-effects should be fairly limited. That said, the letterSpacing approach is far better, mainly because the truncation/alignment is perfect (which isn't the case with textDirection), but that changes the text, and potentially makes it harder to read. So that's a question for UX/a11y.

Not sure what the timeline for this menu redesign work is but I thought I'd submit a short-term fix to address the problem in this cycle. I'll start a conversation about the letter spacing with ux/a11y team independently of that.

FTR, there is the patch for the letterSpacing:

commit 3a6ed3ed03d0ad98985c4f15350504555a380176
Author: William Durand <[e-mail]>
Date:   Wed Nov 20 09:17:18 2024 +0100

    Bug 1915867 - Fix recommended extensions' LTR descriptions in RTL mode. r?mcarare!
    
    The `letterSpacing` approach changes the UI a bit but that's what gives
    the best result. The other option would be to force the `textDirection`.

diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/compose/list/ListItem.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/compose/list/ListItem.kt
index f3e9616006544..72a17bf573de3 100644
--- a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/compose/list/ListItem.kt
+++ b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/compose/list/ListItem.kt
@@ -44,10 +44,12 @@ import androidx.compose.ui.semantics.clearAndSetSemantics
 import androidx.compose.ui.semantics.role
 import androidx.compose.ui.semantics.selected
 import androidx.compose.ui.semantics.semantics
+import androidx.compose.ui.text.TextStyle
 import androidx.compose.ui.text.style.TextOverflow
 import androidx.compose.ui.tooling.preview.Preview
 import androidx.compose.ui.unit.Dp
 import androidx.compose.ui.unit.dp
+import androidx.compose.ui.unit.sp
 import mozilla.components.ui.colors.PhotonColors
 import org.mozilla.fenix.R
 import org.mozilla.fenix.compose.Divider
@@ -805,7 +807,9 @@ private fun ListItem(
                     color = if (enabled) descriptionTextColor else FirefoxTheme.colors.textDisabled,
                     overflow = TextOverflow.Ellipsis,
                     maxLines = maxDescriptionLines,
-                    style = FirefoxTheme.typography.body2,
+                    // Bug 1915867 - We must force no letter spacing to correctly truncate a LTR description that is
+                    // too long when the app in RTL mode - at least until this bug gets fixed in Compose.
+                    style = FirefoxTheme.typography.body2.merge(TextStyle(letterSpacing = 0.sp)),
                 )
             }
         }

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7e09524c9bd
Fix recommended extensions' LTR descriptions in RTL mode. r=mcarare,android-reviewers

Comment 15

[Norisz Fay [:noriszfay]]

Backed out for causing multiple wpt failures

Backout link

Push with failures

Failure log // Failure log 2 // Failure log 3

More logs can be found by clicking on each individual failure.

Edit: The backout is obsolete as it was mistakenly backed out.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7edb5ee7a3ef
Fix recommended extensions' LTR descriptions in RTL mode. r=mcarare,android-reviewers CLOSED TREE

Comment 17

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/7edb5ee7a3ef

Comment 18

[William Durand [:willdurand]]

FTR we discussed about this bug together with the a11y, content, and UX teams again. While removing the letter spacing would be a technically™ better fix for this bug, it's going to have a negative impact on users with low or blurry vision or dyslexia, etc. Therefore, we'll keep the fix landed in Comment 17, and we won't do more changes to address this bug - it's considered fixed.