1296648 - Fix direction of .ruleview-expander.theme-twisty in RTL locales

Status: VERIFIED FIXED

(DevTools :: Inspector: Rules, defect, P3)

Description

[magicp]

Attachment: rtl-ruleview-expander-theme-twisty.png

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20160819030226

Steps to reproduce:

1. Start Nightly
2. Go to any sites (e.g. about:home)
3. Open DevTools > Inspector > Rules
4. Check .ruleview-expander.theme-twisty


Actual results:

.ruleview-expander.theme-twisty has wrong direction.


Expected results:

No longer transform scaleX(-1) as Ruleview is ltr.

Comment 1

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 2

[[:Towkir] Ahmed]

I think we should not mirror the twisty at all.
In that case, we can remove these rules:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/dark-theme.css#285-289
https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/light-theme.css#297-301

And this one looks important and can be kept:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/webconsole.css#575-579

What is your opinion on this Brosset ?

Comment 3

[Patrick Brosset <:pbro>]

This used to work in Firefox 50, and fails in 51, so it's a regression.

I'm forwarding this needinfo to Brian who might know what has changed in the theme CSS that might impact this. Also ntim might also know.
In any case, they will be better persons than me to ask for feedback/reviews.

Comment 4

[Tim Nguyen :ntim]

Seems like a regression of bug 1205590.

We can't just remove the rules in light-theme.css and dark-theme.css, since this would affect other places.

I'd prefer setting transform: none on the rule view twisties with a comment explaining why.

Comment 5

[[:Towkir] Ahmed]

Attachment: twistyOnRtlLocales.patch

Hello Tim,
Hope this is fine. but it would be very nice to know how would it break things somewhere if the rules were removed. :)

Thanks

Comment 6

[Tim Nguyen :ntim]

Comment on attachment 8785961 [details] [diff] [review]
twistyOnRtlLocales.patch

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

By setting transform: none I actually meant setting it in rules.css similar to how it's currently set in webconsole.css

The current patch basically does the same thing as removing the rule, which would break the computed view, the memory tool and the perf tool twisties (I might be missing some other places too)

Comment 7

[[:Towkir] Ahmed]

Should there be any specific line number on rules.css file ? or any place is ok.
and one more thing, webconsole.css has the twisty in a .message class, 
should it be the same in here too ? or just the .theme-twisty:dir and moz dir ??

Comment 8

[Tim Nguyen :ntim]

(In reply to [:Towkir] Ahmed from comment #7)
> Should there be any specific line number on rules.css file ? or any place is
> ok.
I would say where it makes sense.
Here would be a good place: https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/rules.css#326
> and one more thing, webconsole.css has the twisty in a .message class, 
> should it be the same in here too ? or just the .theme-twisty:dir and moz
> dir ??
.ruleview-expander:-moz-locale-dir(rtl) is enough. -moz-locale-dir because the inspector is a XUL doc and .ruleview-expander to be as specific as we can.

Comment 9

[[:Towkir] Ahmed]

Attachment: rtlDirectionRuleviewExpander.patch

Here it is.

Comment 10

[Tim Nguyen :ntim]

Comment on attachment 8786095 [details] [diff] [review]
rtlDirectionRuleviewExpander.patch

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

The patch doesn't seem to work locally, the arrow is still backwards.

It seems due to the higher specificity of the other transform rule. Maybe `.ruleview-expander.theme-twisty:-moz-locale-dir(rtl)` instead?

::: devtools/client/themes/rules.css
@@ +325,5 @@
>    display: inline-block;
>  }
>  
> +.ruleview-expander:-moz-locale-dir(rtl) {
> +  /*for preventing .theme-twisty's wrong direction in rtl; Bug 1296648 */

nit: add whitespace after /*

Comment 11

[Tim Nguyen :ntim]

You can use the Force RTL add-on to test your changes: https://addons.mozilla.org/en-US/firefox/addon/force-rtl/

Comment 12

[[:Towkir] Ahmed]

Attachment: rtlDirectionThemeTwisty.patch

Hi Tim, 
Sorry for being late. Here is the updated patch.
Thanks

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 8787932 [details] [diff] [review]
rtlDirectionThemeTwisty.patch

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

Looks good to me, works fine locally as well.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/16a1a91f9269
Fix direction of .ruleview-expander.theme-twisty in RTL locales. r=ntim

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/16a1a91f9269

Comment 16

[magicp]

verified on latest Nightly (Build ID: 20160908030434). Thanks all.

Comment 17

[Tim Nguyen :ntim]

Thanks magicp!