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

VERIFIED FIXED in Firefox 56

Status

()

Firefox for Android
Theme and Visual Design
P1
normal
Rank:
1
VERIFIED FIXED
6 months ago
3 days ago

People

(Reporter: ItielMaN, Assigned: liuche)

Tracking

(Blocks: 2 bugs, {rtl})

unspecified
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [MobileAS] 1.26)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

6 months ago
Created attachment 8834438 [details]
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.
(Reporter)

Updated

6 months ago
Summary: [RTL] Separators in Activity Stream items' context menu should be align to the left → [RTL] Separators in Activity Stream items' context menu should be aligned to the left

Comment 1

6 months ago
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.
(Reporter)

Comment 2

6 months ago
(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

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

Updated

17 days ago
Priority: -- → P2
Whiteboard: [MobileAS]

Updated

12 days ago
Rank: 1
Whiteboard: [MobileAS] → [MobileAS] 1.26
(Assignee)

Updated

8 days ago
Assignee: nobody → liuche
Comment hidden (mozreview-request)
(Assignee)

Comment 5

8 days ago
Created attachment 8888608 [details]
Screenshot: rtl aligned dividers

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

Comment 6

8 days ago
mozreview-review
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.
Attachment #8888607 - Flags: review?(michael.l.comella) → review+
Iteration: --- → 1.26
Priority: P2 → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

5 days ago
Pushed by cliu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/080aa07d32df
Add rtl divider resource. r=mcomella
https://hg.mozilla.org/mozilla-central/rev/080aa07d32df
Status: NEW → RESOLVED
Last Resolved: 4 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
(Reporter)

Comment 11

3 days ago
LGTM.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.