1287391 - Wrong sidebar tabs direction in RTL locales with Firebug theme

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P1)

Description

[magicp]

Attachment: inspector-sidebar-tabs-direction.png

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

Steps to reproduce:

1. Start Nightly in RTL locales
2. Open DevTools > Inspector with Firebug theme
3. Check sidebar tabs


Actual results:

Tabs direction was regressed from RTL to LTR.

Regression range:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=a8ab215f15d3a875f964663a6011b5a6cba67919&tochange=453c308dcab1e3236fde0c4ee2e6d0d3d2d60dae


Expected results:

Tabs direction should be from right to left in RTL locales.

Comment 1

[Steve Chung [:steveck]]

It seems like the sidebar navigation in firebug theme apply block display with float left instead of flex in other themes. We can either apply the flex for firebug theme as well, or add float right in RTL.

Comment 2

[Steve Chung [:steveck]]

Attachment: bug-1287391.patch

Hi Patrick, I think we should leverage flex for RTL issue and adjust the tab width by flex-grow, but please let me know if you prefer using float right in the firebug theme or other solutions, thanks.

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8772683 [details] [diff] [review]
bug-1287391.patch

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

::: devtools/client/shared/components/tabs/tabbar.css
@@ +48,5 @@
>  }
>  
>  /* Firebug theme doesn't stretch the tabs. */
> +.theme-firebug .tabs .tabs-navigation .tabs-menu-item {
> +  flex-grow: 0;

I think this is a very good solution.
The only other thing I can think of, which I think is equally acceptable, is to get rid of "left" in "float:left;" and use the corresponding logical property instead: "float:inline-start;".

We should really take the habit of using these logical properties now that they are available to us. They make dealing with RTL locales so much easier.

Up to you which solution you choose, but I'll R+ any of these 2.

Comment 4

[Steve Chung [:steveck]]

(In reply to Patrick Brosset <:pbro> from comment #3)

> The only other thing I can think of, which I think is equally acceptable, is
> to get rid of "left" in "float:left;" and use the corresponding logical
> property instead: "float:inline-start;".
> 
> Up to you which solution you choose, but I'll R+ any of these 2.

Yeah thanks for the reminder, using float:inline-start is better than specific direction. I'll also apply this in the patch. Even the float will not affect the flex display tabs, but it's to make sure other tabs 
that are not under tabs-navigation displays correctly without RTL issue.

Comment 5

[Steve Chung [:steveck]]

Attachment: Bug 1287391 - Wrong sidebar tabs direction in RTL locales with Firebug theme.

Review commit: https://reviewboard.mozilla.org/r/65808/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65808/

Comment 6

[Steve Chung [:steveck]]

I didn't trigger try build because it neither change the dom structure nor js logic, but please let me know if it's still necessary.

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8773135 [details]
Bug 1287391 - Wrong sidebar tabs direction in RTL locales with Firebug theme.

https://reviewboard.mozilla.org/r/65808/#review62820

Looks good, thanks! No need for a try build for this.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db0243f47625
Wrong sidebar tabs direction in RTL locales with Firebug theme. r=pbro

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/db0243f47625

Comment 10

[Tim Nguyen :ntim]

Steve, float: inline-start is only enabled on Nightly builds, see 1253919, which means this fix won't work for the release channel. Can you file a bug to fix that ? Thanks!

Comment 11

[Steve Chung [:steveck]]

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #10)
> Steve, float: inline-start is only enabled on Nightly builds, see 1253919,
> which means this fix won't work for the release channel. Can you file a bug
> to fix that ? Thanks!

Follow up created in bug 1288996, thanks!

Comment 12

[Ada [:adalucinet]]

Reproduced with Nightly from 2016-07-19 RTL build.
Verified fixed with latest 50.0a1, across platforms [1] - tabs direction is the correct one for RTL builds, but tab names are no longer translated → https://i.imgur.com/jrahnnK.png
Steve, is this behavior expected for now?

[1] Windows 10 64-bit, Ubuntu 16.04 64-bit and Mac OS X 10.11

Comment 13

[Tim Nguyen :ntim]

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #12)
> Reproduced with Nightly from 2016-07-19 RTL build.
> Verified fixed with latest 50.0a1, across platforms [1] - tabs direction is
> the correct one for RTL builds, but tab names are no longer translated →
> https://i.imgur.com/jrahnnK.png
> Steve, is this behavior expected for now?
> 
> [1] Windows 10 64-bit, Ubuntu 16.04 64-bit and Mac OS X 10.11

I don't think it's specifically a regression of this bug since it leaves the strings untouched. Probably a regression of bug 1259819, since it moves the strings from a dtd file to a properties file.

Comment 14

[Steve Chung [:steveck]]

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #13)
> (In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #12)
> > Reproduced with Nightly from 2016-07-19 RTL build.
> > Verified fixed with latest 50.0a1, across platforms [1] - tabs direction is
> > the correct one for RTL builds, but tab names are no longer translated →
> > https://i.imgur.com/jrahnnK.png
> > Steve, is this behavior expected for now?
> > 
> > [1] Windows 10 64-bit, Ubuntu 16.04 64-bit and Mac OS X 10.11
> 
> I don't think it's specifically a regression of this bug since it leaves the
> strings untouched. Probably a regression of bug 1259819, since it moves the
> strings from a dtd file to a properties file.

Thanks for the explanation, I don't think the patch will affect the string localization either and the strings were not translated before this patch. Maybe we need to file another bug for the regression?

Comment 15

[Ada [:adalucinet]]

Thanks for the replies!
Logged bug 1292104 for this regression. Marking here accordingly.