Closed Bug 1288996 Opened 3 years ago Closed 3 years ago

Replace float: inline-start before core fully support

Categories

(DevTools :: Inspector, enhancement, P1)

enhancement

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Iteration:
50.4 - Aug 1
Tracking Status
firefox50 --- fixed

People

(Reporter: steveck, Assigned: steveck)

References

Details

(Whiteboard: [reserve-html])

Attachments

(2 files)

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.
Whiteboard: [devtools-html] [triage]
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.
Attachment #8774290 - Flags: feedback?(ntim.bugs)
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.
Attachment #8774290 - Flags: feedback?(ntim.bugs) → feedback+
(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.
Attachment #8774628 - Flags: review?(ntim.bugs) → review+
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!
Thanks for the review and suggestions!
Keywords: checkin-needed
Status: NEW → ASSIGNED
Flags: qe-verify+
Priority: -- → P3
QA Contact: alexandra.lucinet
Whiteboard: [devtools-html] [triage] → [reserve-html]
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/23da9dd0a0f3
Replace float: inline-start before core fully support. r=ntim
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/23da9dd0a0f3
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Iteration: --- → 50.4 - Aug 1
Priority: P3 → P1
QA Contact: adalucin → cristian.comorasu
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!
Flags: needinfo?(schung)
(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.
Flags: needinfo?(schung)
Removing QE flag based on comment #10.
Flags: qe-verify+ → qe-verify-
QA Contact: cristian.comorasu
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.