Closed Bug 1393274 Opened 2 years ago Closed 2 years ago

[RTL] Top sites highlight item does not support rtl correctly

Categories

(Firefox for Android :: Awesomescreen, defect, P1)

All
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 57
Iteration:
1.29
Tracking Status
firefox57 --- verified

People

(Reporter: mcomella, Assigned: mcomella)

References

(Blocks 1 open bug)

Details

(Whiteboard: [mobileAS])

Attachments

(8 files)

(In reply to Michael Comella (:mcomella) from bug 1385934 comment #2)
> However, the highlights items is not correct:
> - the page domain text should be right-aligned
> - "Visited" should be right-aligned to the icon.

See screenshot: https://bug1385934.bmoattachments.org/attachment.cgi?id=8900506
Attached image Screenshot: arabic
(In reply to Michael Comella (:mcomella) from comment #0)
> (In reply to Michael Comella (:mcomella) from bug 1385934 comment #2)
> > However, the highlights items is not correct:
> > - the page domain text should be right-aligned
> > - "Visited" should be right-aligned to the icon.

Actually, it looks like the text alignment depends on the text locale. With my device in Arabic (Nightly b/c local builds aren't multi locale by default), the text is right-aligned and the issues I mentioned are not a problem.

Flod, in my screenshot, at the bottom of the screen, should the domain name (ascii) be right-aligned instead? (I wonder we could do if it should be right-aligned...).

For my own observation, the URL bar is always left aligned since it displays ascii text.
Flags: needinfo?(francesco.lodolo)
Summary: [RTL] Top sites highlight item is does not support rtl correctly → [RTL] Top sites highlight item does not support rtl correctly
Uhm, CCing someone who understands RTL better than me. 

I'm pretty sure it should be right aligned, but let's see if any of them can confirm it. The text itself is going to be LTR, since it's Latin characters, but that doesn't change the fact that UI is read from the right.

P.S: For future reference, Delphine is the l10n person in charge of Fennec (and mobile products in general). Since she's on PTO for the next 3 weeks, it doesn't change much in this case, since I'm her fallback ;-)
Flags: needinfo?(francesco.lodolo)
Usually I'd say it should be aligned according to it's text direction (bidi, as it is implemented right now), it'd be confusing if the RTL "Visited" text and the page's title are would be aligned to the right, and the domain name to the left (as the domain name would almost always be LTR)
It'd probably get worse on landscape mode, or on tablets where there would be a considerably big spacing between them.

That's why we can avoid that if all 3 items would automatically be aligned to the right (though, bidi should still be applied to the text itself. See attachment 8817617 [details], expected2 sector).
(In reply to Michael Comella (:mcomella) from comment #1)
> Created attachment 8900511 [details]
> Screenshot: arabic
> 
> (In reply to Michael Comella (:mcomella) from comment #0)
> > (In reply to Michael Comella (:mcomella) from bug 1385934 comment #2)
> > > However, the highlights items is not correct:
> > > - the page domain text should be right-aligned
> > > - "Visited" should be right-aligned to the icon.
> 
> Actually, it looks like the text alignment depends on the text locale. With
> my device in Arabic (Nightly b/c local builds aren't multi locale by
> default), the text is right-aligned and the issues I mentioned are not a
> problem.
> 
> Flod, in my screenshot, at the bottom of the screen, should the domain name
> (ascii) be right-aligned instead? (I wonder we could do if it should be
> right-aligned...).
> 
> For my own observation, the URL bar is always left aligned since it displays
> ascii text.

Hi,

Regarding Flod and ItielMaN comments, it is more clear when we use left direction for the URL texts rather than right direction.
The main reason is the way we read the Latin text. If you change a Latin direction to the right side it leads to confusion for the RTL users.
Iteration: 1.28 → 1.29
ItielMan, what about the case where the system locale is an RTL language but the domain & page title are in latin script? e.g. a device in Arabic that visits an article on bbc.co.uk? My intuition says we should align the page title naturally (e.g. right for RTL, left for LTR) and match that alignment for the page domain. The source of the highlight (e.g. "Bookmarked", "Visited") should always be aligned to natural text direction which will be the natural direction for the locale. What do you think?
Flags: needinfo?(itiel_yn8)
(In reply to Michael Comella (:mcomella) from comment #5)
> My intuition says we should align the page title naturally
> (e.g. right for RTL, left for LTR) and match that alignment
> for the page domain.

... and in such scenario, a domain name in one entry in the (top sites) list would be aligned to the right, and another to the left, even though both have the same directionality (LTR). It will cause inconsistency.
Besides, it will still result in big spacing between the domain+page title and the source of the highlight ("Bookmarked" and "Visited"). Imagine how it will look like on (probably big) tablets.. users could get confused by this.
Flags: needinfo?(itiel_yn8)
To be explicitly state my understanding, we should left-align domain, page title, and source (e.g. "visited") in LTR locales and right-align domain, page title, and source in RTL locales, no matter the source language (e.g. page title ). The reason for this is to avoid large gaps between related text.

The text direction (i.e. the way you read the text) should always remain the same as the source language (e.g. we shouldn't write "apple" as "elppa"). For punctuation, we should be careful to follow text direction, as in the link in comment 3.

---

From an implementation perspective, I think we can solve text alignment by setting textAlignment="start" on all three fields and the text direction should just work.
(In reply to Michael Comella (:mcomella) from comment #7)
> To be explicitly state my understanding, we should left-align domain, page
> title, and source (e.g. "visited") in LTR locales and right-align domain,
> page title, and source in RTL locales, no matter the source language (e.g.
> page title ). The reason for this is to avoid large gaps between related
> text.
> 
> The text direction (i.e. the way you read the text) should always remain the
> same as the source language (e.g. we shouldn't write "apple" as "elppa").
> For punctuation, we should be careful to follow text direction, as in the
> link in comment 3.

Precisely. Theoretically, if all 3 items (domain, page title, and source) would always be LTR text (or always RTL), I wouldn't be against aligning them according to their direction as it would be consistent.

See an example, taken from Google Chrome's settings:
https://i.imgur.com/UPbG4VE.png
Imagine if the "JavaScript" and "Flash" text would have been aligned according to their directionality- it'd simply look bad (and of course break the consistency of the list). The user may think something's missing above the Hebrew text, until he'd notice the text in the left. I think that's the same in our case.
Duplicate of this bug: 1388168
Unfortunately, I don't have a multi-locale dev build so here's a screenshot for the arabic locale with en-US strings. However, I'm confident this will work with the arabic strings. NI self to verify on nightly.
Flags: needinfo?(michael.l.comella)
(In reply to Michael Comella (:mcomella) from comment #13)
> Created attachment 8902896 [details]
> Screenshot post-patch: Arabic locale, no l10n strings
> 
> Unfortunately, I don't have a multi-locale dev build so here's a screenshot
> for the arabic locale with en-US strings. However, I'm confident this will
> work with the arabic strings. NI self to verify on nightly.

Can you upload a new screenshot, this time bookmark the following website?
https://www.walla.co.il/
Of course, make sure it appears in the Highlights section before taking a screenshot. Thanks!
Comment on attachment 8902887 [details]
Bug 1393274: Rename card_history_item -> webpage_item_row & friends.

https://reviewboard.mozilla.org/r/174594/#review179810
Attachment #8902887 - Flags: review?(liuche) → review+
Comment on attachment 8902888 [details]
Bug 1393274: Align webpage item row text to right in RTL, left in LTR.

https://reviewboard.mozilla.org/r/174596/#review179812
Attachment #8902888 - Flags: review?(liuche) → review+
Comment on attachment 8902889 [details]
Bug 1393274: Correct webpage RTL padding/margins.

https://reviewboard.mozilla.org/r/174598/#review179814
Attachment #8902889 - Flags: review?(liuche) → review+
(In reply to Itiel from comment #14)
> Can you upload a new screenshot, this time bookmark the following website?
> https://www.walla.co.il/

This looks correct to me!
Pushed by michael.l.comella@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/03063f77c3f5
Rename card_history_item -> webpage_item_row & friends. r=liuche
https://hg.mozilla.org/integration/autoland/rev/29e8bf6a34c1
Align webpage item row text to right in RTL, left in LTR. r=liuche
https://hg.mozilla.org/integration/autoland/rev/52e7fa268a65
Correct webpage RTL padding/margins. r=liuche
Comment on attachment 8903290 [details]
Bug 1393274 - bustage: Add gravity=start where textAlignment to appease lint.

https://reviewboard.mozilla.org/r/175082/#review180144

r=trivial
Attachment #8903290 - Flags: review?(michael.l.comella) → review+
Pushed by michael.l.comella@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e8bc46732fe1
Rename card_history_item -> webpage_item_row & friends. r=liuche
https://hg.mozilla.org/integration/autoland/rev/2a39f7aa9629
Align webpage item row text to right in RTL, left in LTR. r=liuche
https://hg.mozilla.org/integration/autoland/rev/a1a1f209f825
Correct webpage RTL padding/margins. r=liuche
https://hg.mozilla.org/integration/autoland/rev/63b87e31320d
bustage: Add gravity=start where textAlignment to appease lint. r=mcomella
Looking good on latest Nightly :)
Removing Michael's NI.
Status: RESOLVED → VERIFIED
Flags: needinfo?(michael.l.comella)
It's properly displayed in beta as well. Marking as verified.
You need to log in before you can comment on or make changes to this bug.