962284 - Metro Tab strip overflow layout is wrong in RTL locales

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Theme, defect, P2)

Description

[Matt Brubeck (:mbrubeck)]

When running in a right-to-left locale (bug 851165), if there are enough tabs open to make the tab strip scrollable, the tabs do not line up correctly with the vertical separators (bug 948778).

Comment 1

[Marina Samuel [:emtwo]]

Attachment: v1: Adjust tabstrip css in rtl locales

Oops, I stole a mentor-able bug O_o

Comment 2

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8368220 [details] [diff] [review]
v1: Adjust tabstrip css in rtl locales

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

This fixes the main issue I reported, plus some that I hadn't noticed.  But there seem to be some remaining issues, and I don't understand all of the styles.  (They might just need some comments to explain them.)

::: browser/metro/theme/browser.css
@@ +55,5 @@
>  }
>  
> +.tabs-scrollbox > .scrollbutton-up:-moz-locale-dir(rtl),
> +.tabs-scrollbox > .scrollbutton-down:-moz-locale-dir(rtl) {
> +   transform: scaleX(-1);

The arrows now look correct, but they still behave wrong (sometimes?).  I'm not sure exactly why... do we need to set a -moz-box-direction: reverse somewhere?

@@ +59,5 @@
> +   transform: scaleX(-1);
> +}
> +
> +.tabs-scrollbox > .scrollbutton-up:not([disabled]):not([collapsed]):-moz-locale-dir(rtl)::after {
> +  left: @tabs_scrollarrow_width@ !important;

With this patch applied, the separators now appear correctly in RTL when using the mouse (inputmode="precise") but I don't see them when using RTL+touch.  (In LTR locales they are visible with both precise and imprecise input.)  Again, it's not clear to me why this isn't working correctly.

@@ +64,5 @@
> +}
> +
> +.tabs-scrollbox > .scrollbutton-down:not([disabled]):not([collapsed]):-moz-locale-dir(rtl)::before {
> +  right: auto;
> +  left: 0px !important;

Do you know whether/why the !important is necessary?

Comment 3

[Marina Samuel [:emtwo]]

Attachment: v2: Adjust tabstrip css in rtl locales

> The arrows now look correct, but they still behave wrong (sometimes?).

Was there any other incorrect behaviour that you saw besides the lack of separators when using imprecise input?

When I wrote that patch, I looked for where the lines for :before and :after were being drawn then adjusted the "left" property accordingly because with scaleX() I could not make sense of why the :before and :after lines were where they were.

I have another patch here without scaleX() which I think should be more clear and it does not have the missing separator issue.

If you have any ideas of why scaleX() might have been causing issues please let me know. Thanks!

Comment 4

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8368629 [details] [diff] [review]
v2: Adjust tabstrip css in rtl locales

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

(In reply to Marina Samuel [:emtwo] from comment #3)
> Was there any other incorrect behaviour that you saw besides the lack of
> separators when using imprecise input?

Yes - when clicking on the arrow buttons, the scrollbox would sometimes scroll in the correct direction and sometimes in the opposite direction.

> I have another patch here without scaleX() which I think should be more
> clear and it does not have the missing separator issue.

Yes, this looks perfect and also seems to fix the behavior of the buttons noted above.

Comment 5

[Marina Samuel [:emtwo]]

https://hg.mozilla.org/integration/fx-team/rev/438cab96ec84

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/438cab96ec84

Comment 7

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.3; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0

Verified as fixed on Windows 8.1 using the Arabic and Hebrew locales with latest Nightly (build ID: 20140203030203).