Fix bookmarks twisty direction in RTL locales on Mac OS X

VERIFIED FIXED in Firefox 50

Status

()

defect
P3
normal
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: magicp.jp, Assigned: stefanh)

Tracking

({regression})

48 Branch
Firefox 52
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 wontfix, firefox50 fixed, firefox51 fixed, firefox52 verified)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

3 years ago
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20161012143919

Steps to reproduce:

1. Start Nightly in RTL locales (e.g. Arabic) on Mac OS X
2. View > Sidebar > Bookmarks
3. Confirm twisty direction


Actual results:

bookmarks twisty direction is wrong in RTL locales


Expected results:

In RTL locales, twisty should be reversed.
Reporter

Updated

3 years ago
Component: Untriaged → Bookmarks & History
OS: Unspecified → Mac OS X
Hardware: Unspecified → All
Priority: -- → P3
Reporter

Comment 1

3 years ago
Last good revision: fe11700b6cdfdb0fca3a9b2a54bf6d339792df24
First bad revision: 196a2b799ea00257f8e172f3063b94f05d99b494
Pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=fe11700b6cdfdb0fca3a9b2a54bf6d339792df24&tochange=196a2b799ea00257f8e172f3063b94f05d99b494
Blocks: 680256
Has Regression Range: --- → yes
Has STR: --- → yes
Version: unspecified → 49 Branch
Keywords: regression
shorlander, is this a simple fix?
Flags: needinfo?(shorlander)
(In reply to Andrew Overholt [:overholt] from comment #2)
> shorlander, is this a simple fix?

Not as simple as I would like… The usual transform: scaleX(-1) doesn't appear to work in XUL trees. Will need to add RTL images and rules specifically I think.
Flags: needinfo?(shorlander)
(In reply to Stephen Horlander [:shorlander] from comment #3)
> (In reply to Andrew Overholt [:overholt] from comment #2)
> > shorlander, is this a simple fix?
> 
> Not as simple as I would like… The usual transform: scaleX(-1) doesn't
> appear to work in XUL trees. Will need to add RTL images and rules
> specifically I think.

Is that something we'd do and uplift to beta? I'm just wondering if we should ship 50 with this or hold it and wait for a fix.
Flags: needinfo?(shorlander)
(In reply to Andrew Overholt [:overholt] from comment #4)
> (In reply to Stephen Horlander [:shorlander] from comment #3)
> > (In reply to Andrew Overholt [:overholt] from comment #2)
> > > shorlander, is this a simple fix?
> > 
> > Not as simple as I would like… The usual transform: scaleX(-1) doesn't
> > appear to work in XUL trees. Will need to add RTL images and rules
> > specifically I think.
> 
> Is that something we'd do and uplift to beta? I'm just wondering if we
> should ship 50 with this or hold it and wait for a fix.

I will put together a patch and see if we can uplift. We are already shipping the regression on release.
Assignee

Comment 6

3 years ago
I have a patch for this. The only thing that I don't seem to be able to fix is syncedtabs/sidebar.css. That sidebar is in html, but ":dir(rtl)" doesn't seem to work.
Assignee

Comment 7

3 years ago
Posted patch 1309856.1.diff (obsolete) — Splinter Review
I don't know if you already have started to work on this, but I'm sort of curious if you know why the sidebar.css changes doesn't work. That said, since the syncedtabs sidebar doesn't pick up any rtl it might not be me...
Attachment #8805247 - Flags: feedback?(shorlander)
Assignee

Comment 8

3 years ago
(In reply to Stefan [:stefanh] from comment #7)
> I don't know if you already have started to work on this, but I'm sort of
> curious if you know why the sidebar.css changes doesn't work. That said,
> since the syncedtabs sidebar doesn't pick up any rtl it might not be me...

Nevermind, it does seems to work... I installed the "Force RTL" extension and it looks fine now (I started testing it with just setting 'intl.uidirection.en-US' to 'rtl').
Assignee

Updated

3 years ago
Flags: needinfo?(shorlander)
Assignee

Comment 9

3 years ago
It's probably obvious (it's late here), but right now I fail to understand why I need all 4 rules in sidebar.css. It's html, but that shouldn't matter, should it?
Assignee: nobody → stefanh
Attachment #8805247 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8805247 - Flags: feedback?(shorlander)
Attachment #8805266 - Flags: review?(gijskruitbosch+bugs)

Comment 10

3 years ago
Comment on attachment 8805266 [details] [diff] [review]
1309856.2.diff

Review of attachment 8805266 [details] [diff] [review]:
-----------------------------------------------------------------

I think the twisty containers have their own cell, right? Can't we just transform() them to invert the X axis when using RTL? That would limit the number of "duplicate" rules we need here.
Attachment #8805266 - Flags: review?(gijskruitbosch+bugs)
treeviews don't support most of the css rules, I really think they don't support transform at all.
Some time ago I added opacity support, but that was trivial (https://hg.mozilla.org/mozilla-central/rev/ed60c3abdb71).
Maybe you could similarly add support for a very trivial scale(-1) transform and just paint a reversed image.

Comment 12

3 years ago
Comment on attachment 8805266 [details] [diff] [review]
1309856.2.diff

Review of attachment 8805266 [details] [diff] [review]:
-----------------------------------------------------------------

I wouldn't want to add CSS transform support to tree cells, that doesn't sound like fun. Let's just take this, then.

::: browser/themes/osx/syncedtabs/sidebar.css
@@ +53,5 @@
>  }
>  
> +.item.client .item-twisty-container:dir(rtl) {
> +  background-image: url("chrome://global/skin/tree/arrow-disclosure.svg#arrow-disclosure-expanded");
> +}

I expect the reason for needing multiple rules is the ordering of rules. You might be able to reduce the number of rules by leaving the collapsed selectors (which AIUI are the only ones you really want) inbetween the selectors above, instead of at the end.
Attachment #8805266 - Flags: review+

Comment 13

3 years ago
Pushed by stefanh@inbox.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c1b8232382a
Make Mac twistys in places window and sidebar rtl-friendly. r=Gijs.
Assignee

Comment 14

3 years ago
JFTR, I pushed the patch as-is, since I couldn't reduce the number of rules.

Comment 15

3 years ago
Pushed by stefanh@inbox.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c834894e4b4
Add some style rules that I missed in the previous patch.
Assignee

Comment 16

3 years ago
(In reply to Pulsebot from comment #15)
> Pushed by stefanh@inbox.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/5c834894e4b4
> Add some style rules that I missed in the previous patch.

Turns out that I needed rtl rules for the "open" twistys in places.css and organizer.css...

Comment 17

3 years ago
Per IRC, ni to file a followup to see if using the 'closed' atom can avoid some of the repetition by making rules for the closed state not apply to items in the open state and vice versa.
Flags: needinfo?(stefanh)
Assignee

Updated

3 years ago
Blocks: 1313848
Assignee

Comment 18

3 years ago
(In reply to :Gijs Kruitbosch from comment #17)
> Per IRC, ni to file a followup to see if using the 'closed' atom can avoid
> some of the repetition by making rules for the closed state not apply to
> items in the open state and vice versa.

Filed bug 1313848 which I will look at in a few days.
Component: Bookmarks & History → Theme
Assignee

Updated

3 years ago
Flags: needinfo?(stefanh)

Comment 19

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9c1b8232382a
https://hg.mozilla.org/mozilla-central/rev/5c834894e4b4
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Reporter

Comment 20

3 years ago
Verified fixed in latest Nightly build () on Mac OS X 10.10.5.
Reporter

Comment 21

3 years ago
How about uplift to Dev. Edition and Beta?

Comment 22

3 years ago
Comment on attachment 8805266 [details] [diff] [review]
1309856.2.diff

Approval Request Comment
[Feature/regressing bug #]: bug 680256
[User impact if declined]: regresses twisty appearance for RTL users in various trees
[Describe test coverage new/current, TreeHerder]: nope, CSS only
[Risks and why]: low, CSS-only changes
[String/UUID change made/needed]: no

NB: please uplift all commits for this bug.
Attachment #8805266 - Flags: approval-mozilla-beta?
Attachment #8805266 - Flags: approval-mozilla-aurora?
Status: RESOLVED → VERIFIED
Comment on attachment 8805266 [details] [diff] [review]
1309856.2.diff

CSS only, low risk, poor experience for RTL users, fix was verified on Nightly, Aurora51+, Beta50+
Attachment #8805266 - Flags: approval-mozilla-beta?
Attachment #8805266 - Flags: approval-mozilla-beta+
Attachment #8805266 - Flags: approval-mozilla-aurora?
Attachment #8805266 - Flags: approval-mozilla-aurora+
Version: 49 Branch → 48 Branch
You need to log in before you can comment on or make changes to this bug.