Closed Bug 1309856 Opened 8 years ago Closed 8 years ago

Fix bookmarks twisty direction in RTL locales on Mac OS X

Categories

(Firefox :: Theme, defect, P3)

48 Branch
All
macOS
defect

Tracking

()

VERIFIED FIXED
Firefox 52
Tracking Status
firefox49 --- wontfix
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- verified

People

(Reporter: magicp.jp, Assigned: stefanh)

References

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

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.
Component: Untriaged → Bookmarks & History
OS: Unspecified → Mac OS X
Hardware: Unspecified → All
Priority: -- → P3
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.
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.
Attached 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)
(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').
Flags: needinfo?(shorlander)
Attached patch 1309856.2.diffSplinter Review
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 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 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+
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.
JFTR, I pushed the patch as-is, since I couldn't reduce the number of rules.
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.
(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...
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)
Blocks: 1313848
(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
Flags: needinfo?(stefanh)
https://hg.mozilla.org/mozilla-central/rev/9c1b8232382a
https://hg.mozilla.org/mozilla-central/rev/5c834894e4b4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Verified fixed in latest Nightly build () on Mac OS X 10.10.5.
How about uplift to Dev. Edition and Beta?
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?
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.