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

Status: VERIFIED FIXED

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

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 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.

Comment 2

[Francesco Lodolo [:flod]]

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 ;-)

Comment 3

[Itiel]

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).

Comment 4

[Reza Habibi (rehb)]

(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.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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?

Comment 6

[Itiel]

(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.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 8

[Itiel]

(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.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1393274: Rename card_history_item -> webpage_item_row & friends.

Unrelated to RTL changes but these names are awful and gotta go!

Review commit: https://reviewboard.mozilla.org/r/174594/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174594/

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1393274: Align webpage item row text to right in RTL, left in LTR.

Review commit: https://reviewboard.mozilla.org/r/174596/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174596/

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1393274: Correct webpage RTL padding/margins.

Previously, I added a comment to explain why the explicit 0dp padding/margins
are necessary for RTL but since it's required everywhere padding/margins are
used, it's unsustainable and I chose not to do it here.

Review commit: https://reviewboard.mozilla.org/r/174598/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174598/

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 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.

Comment 14

[Itiel]

(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 15

[Chenxia Liu [:liuche]]

Comment on attachment 8902887 [details]
Bug 1393274: Rename card_history_item -> webpage_item_row & friends.

https://reviewboard.mozilla.org/r/174594/#review179810

Comment 16

[Chenxia Liu [:liuche]]

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

Comment 17

[Chenxia Liu [:liuche]]

Comment on attachment 8902889 [details]
Bug 1393274: Correct webpage RTL padding/margins.

https://reviewboard.mozilla.org/r/174598/#review179814

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Screenshot post-patch: walla RTL

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Screenshot post-patch: walla LTR

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
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 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

These caused android lint failures like https://treeherder.mozilla.org/logviewer.html#?job_id=127502403&repo=autoland

Backed out in https://hg.mozilla.org/integration/autoland/rev/250d13bd254cb6aae1caf2d5973c16846027e8e8

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1393274 - bustage: Add gravity=start where textAlignment to appease lint.

Lint passes locally.

Review commit: https://reviewboard.mozilla.org/r/175082/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/175082/

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8902887 [details]
Bug 1393274: Rename card_history_item -> webpage_item_row & friends.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174594/diff/1-2/

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8902888 [details]
Bug 1393274: Align webpage item row text to right in RTL, left in LTR.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174596/diff/1-2/

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8902889 [details]
Bug 1393274: Correct webpage RTL padding/margins.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174598/diff/1-2/

Comment 27

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 28

[Pulsebot]

Pushed by [e-mail]:
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

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e8bc46732fe1
https://hg.mozilla.org/mozilla-central/rev/2a39f7aa9629
https://hg.mozilla.org/mozilla-central/rev/a1a1f209f825
https://hg.mozilla.org/mozilla-central/rev/63b87e31320d

Comment 30

[Itiel]

Looking good on latest Nightly :)
Removing Michael's NI.

Comment 31

[Bogdan Surd, QA [:bsurd, NI]]

It's properly displayed in beta as well. Marking as verified.