1288996 - Replace float: inline-start before core fully support

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P1)

Description

[Steve Chung [:steveck]]

In bug 1287391, we apply the float: inline-start for the locale direction related issue, but it only enabled in nightly. We should resume the float left first and apply again after bug 1253919.
Just a reminder: It will NOT affect the inspector sidebar tab's layout, because the direction is actually decided by the flexbox instead of float. We resume the float because of the tabs other than inspector sidebar's tabbar.

Comment 1

[Steve Chung [:steveck]]

Attachment: bug-1288996.patch

Not sure if we should take care of the rtl tab case, because tabs.css it seems only applied for sidebar right now. Maybe we should remove float in tabs instead.

Comment 2

[Tim Nguyen :ntim]

Comment on attachment 8774290 [details] [diff] [review]
bug-1288996.patch

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

::: devtools/client/shared/components/tabs/tabs.css
@@ +19,5 @@
>  .tabs .tabs-menu-item {
> +  float: left;
> +}
> +
> +.tabs .tabs-menu-item:-moz-locale-dir(rtl) {

.tabs .tabs-menu-item:dir(rtl),
.tabs .tabs-menu-item:-moz-locale-dir(rtl)

It would be better to use this selector, so it works on both HTML and XUL docs.

Comment 3

[Tim Nguyen :ntim]

(In reply to Steve Chung [:steveck] from comment #1)
> Created attachment 8774290 [details] [diff] [review]
> bug-1288996.patch
> 
> Not sure if we should take care of the rtl tab case, because tabs.css it
> seems only applied for sidebar right now. Maybe we should remove float in
> tabs instead.
I prefer removing the float completely if that fixes RTL.

Comment 4

[Steve Chung [:steveck]]

Attachment: Bug 1288996 - Replace float: inline-start before core fully support.

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

Comment 5

[Tim Nguyen :ntim]

Comment on attachment 8774628 [details]
Bug 1288996 - Replace float: inline-start before core fully support.

https://reviewboard.mozilla.org/r/67082/#review64412

Cleans up the code and fixes the RTL issue. Thanks!

Comment 6

[Steve Chung [:steveck]]

Thanks for the review and suggestions!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/23da9dd0a0f3
Replace float: inline-start before core fully support. r=ntim

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/23da9dd0a0f3

Comment 9

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

Hi!
Can you please give me some details on how I should manually verify this bug? (should I search in Browser Content Toolbox?)
Thank you!

Comment 10

[Steve Chung [:steveck]]

(In reply to Cristian Comorasu from comment #9)
> Hi!
> Can you please give me some details on how I should manually verify this
> bug? (should I search in Browser Content Toolbox?)
> Thank you!

I'm afraid that it's unlikely to be verified, because this patch simply remove the unused style to avoid any further misuse in the future. You won't see any difference before/after this patch in the browser actually.

Comment 11

[Marco Mucci [:MarcoM]]

Removing QE flag based on comment #10.