1309856 - Fix bookmarks twisty direction in RTL locales on Mac OS X

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P3)

Description

[magicp]

Attachment: fix-bookmarks-twisty-direction-in-rtl-on-mac.png

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.

Comment 1

[magicp]

Last good revision: fe11700b6cdfdb0fca3a9b2a54bf6d339792df24
First bad revision: 196a2b799ea00257f8e172f3063b94f05d99b494
Pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=fe11700b6cdfdb0fca3a9b2a54bf6d339792df24&tochange=196a2b799ea00257f8e172f3063b94f05d99b494

Comment 2

[Andrew Overholt [:overholt]]

shorlander, is this a simple fix?

Comment 3

[Stephen Horlander [: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.

Comment 4

[Andrew Overholt [:overholt]]

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

Comment 5

[Stephen Horlander [: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.

Comment 6

[Stefan [:stefanh]]

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.

Comment 7

[Stefan [:stefanh]]

Attachment: 1309856.1.diff

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

Comment 8

[Stefan [:stefanh]]

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

Comment 9

[Stefan [:stefanh]]

Attachment: 1309856.2.diff

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?

Comment 10

[:Gijs (he/him)]

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.

Comment 11

[Marco Bonardo [:mak]]

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

[:Gijs (he/him)]

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.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c1b8232382a
Make Mac twistys in places window and sidebar rtl-friendly. r=Gijs.

Comment 14

[Stefan [:stefanh]]

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c834894e4b4
Add some style rules that I missed in the previous patch.

Comment 16

[Stefan [:stefanh]]

(In reply to Pulsebot from comment #15)
> Pushed by [e-mail]:
> 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

[:Gijs (he/him)]

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.

Comment 18

[Stefan [:stefanh]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9c1b8232382a
https://hg.mozilla.org/mozilla-central/rev/5c834894e4b4

Comment 20

[magicp]

Attachment: verified-fixed-screenshot 2016-10-31 8.34.08.png

Verified fixed in latest Nightly build () on Mac OS X 10.10.5.

Comment 21

[magicp]

How about uplift to Dev. Edition and Beta?

Comment 22

[:Gijs (he/him)]

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.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 24

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/870e0c919cf93aac64fe79c5e5345a89b359dc47
https://hg.mozilla.org/releases/mozilla-aurora/rev/5de3d84cb4c1fc8584ae259d04d10f524f2b104f

Comment 25

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

https://hg.mozilla.org/releases/mozilla-beta/rev/aa20e02e9f812f88c46b48ab0e3bac58e0fd0183
https://hg.mozilla.org/releases/mozilla-beta/rev/f2b9a23e83243c285c7f43195fe4bd3bbfdca980

https://hg.mozilla.org/releases/mozilla-release/rev/aa20e02e9f812f88c46b48ab0e3bac58e0fd0183
https://hg.mozilla.org/releases/mozilla-release/rev/f2b9a23e83243c285c7f43195fe4bd3bbfdca980