1337407 - [RTL] Separators in Activity Stream items' context menu should be aligned to the left

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect, P1)

Description

[Itiel]

Attachment: Separators aligned to the right

STR:
1. Install Switchboard Experiments add-on on the latest Nightly
2. In about:experiments, tap on Activity stream
3. Settings -> Advanced -> Enable Activity stream
4. Open a New Tab
5. Click on the 3 dots of one of the suggested items
6. Observe the separators

AR:
The separators are aligned to the right.

ER:
They should be aligned to the left.

See attached screenshot.

Comment 1

[Tomer Cohen :tomer]

I am not sure if URLs should be right-aligned. I'd prefer having the URL as well as the page title displayed according to their language, as in dir=auto.

Comment 2

[Itiel]

(In reply to Tomer Cohen :tomer from comment #1)
> I am not sure if URLs should be right-aligned. I'd prefer having the URL as
> well as the page title displayed according to their language, as in dir=auto.

I was talking about the separators, not URLs :)

Comment 3

[Tomer Cohen :tomer]

Sorry. Now that I see it, I am sure you're right.

Comment 4

[Chenxia Liu [:liuche]]

Attachment: Bug 1337407 - Add rtl divider resource.

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

Comment 5

[Chenxia Liu [:liuche]]

Attachment: Screenshot: rtl aligned dividers

Added a rotated drawable in the ldrtl (layout direction right to left) dir.

Comment 6

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

Comment on attachment 8888607 [details]
Bug 1337407 - Add rtl divider resource.

https://reviewboard.mozilla.org/r/159610/#review165352

::: mobile/android/app/src/main/res/drawable-ldltr/as_contextmenu_divider.xml:1
(Diff revision 1)
> +../drawable/as_rtl_contextmenu_divider.xml

nit: that file should be called `as_ltr_contextmenu_divider` because it's the ltr configuration.

That being said, I'd actually prefer `as_contextmenu_divider_helper` or similar, because it's otherwise unclear that `as_contextmenu_divider` is the one that's intended to be used and `as_contextmenu_divider_helper` only exists because it's impossible to define a bidirectional drawable without it. Maybe add a comment to `as_contextmenu_divider_helper` to that effect too.

Comment 7

[Chenxia Liu [:liuche]]

Comment on attachment 8888607 [details]
Bug 1337407 - Add rtl divider resource.

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

Comment 8

[Chenxia Liu [:liuche]]

Comment on attachment 8888607 [details]
Bug 1337407 - Add rtl divider resource.

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/080aa07d32df
Add rtl divider resource. r=mcomella

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/080aa07d32df

Comment 11

[Itiel]

LGTM.