There were some regressions noted in bug 1446101, I'm combining them here. * Tab throbbers are sometimes missing * Tab throbbers are shown regardless of browser.tabs.hideThrobber * The visible tabs indicator should be removed * The selected tab indicator is missing * There is vertical padding around the scroll bars

Comment on attachment 8987573 [details] Bug 1470947 - Part 3: Ensure tab throbbers are always hidden with browser.tabs.hideThrobber https://reviewboard.mozilla.org/r/252818/#review259334 This pref is only here for a Shield study baseline and isn't something that we are planning on shipping. I'm redirecting this to mconley since it is his Shield study.

(In reply to (away 7/1-7/7) Jared Wein [:jaws] (please needinfo? me) from comment #8) > Comment on attachment 8987573 [details] > Bug 1470947 - Part 3: Ensure tab throbbers are always hidden with > browser.tabs.hideThrobber > > https://reviewboard.mozilla.org/r/252818/#review259334 > > This pref is only here for a Shield study baseline and isn't something that > we are planning on shipping. I'm redirecting this to mconley since it is his > Shield study. why it's super useful.

there should not be blank space before tab title if site/fav icons are disabled just more title info. you missed that, screenshots look great. can this make into fx62?

Comment on attachment 8987571 [details] Bug 1470947 - Part 1: Remove visible tab indicator from all tabs menu https://reviewboard.mozilla.org/r/252814/#review259378 ::: browser/themes/shared/tabs.inc.css (Diff revision 1) > > .all-tabs-item[selected] { > box-shadow: inset 4px 0 var(--blue-40); > } > > -.all-tabs-item[tabIsVisible] { Can we stop setting the tabIsVisible attribute?

Comment on attachment 8987572 [details] Bug 1470947 - Part 2: Call _tabAttrModified on busy and progress changes https://reviewboard.mozilla.org/r/252816/#review259380 ::: browser/base/content/tabbrowser.js:4571 (Diff revision 1) > this.mTab.setAttribute("bursting", "true"); > } > > gBrowser._tabAttrModified(this.mTab, ["busy"]); > } > this.mTab.removeAttribute("progress"); Don't you need to call _tabAttrModified here too?

Comment on attachment 8987574 [details] Bug 1470947 - Part 4: Fix the selected tab colour in all tabs menu https://reviewboard.mozilla.org/r/252820/#review259382

Comment on attachment 8987573 [details] Bug 1470947 - Part 3: Ensure tab throbbers are always hidden with browser.tabs.hideThrobber https://reviewboard.mozilla.org/r/252818/#review259390 ::: browser/base/content/browser.css:209 (Diff revision 1) > > %ifdef NIGHTLY_BUILD > @supports -moz-bool-pref("browser.tabs.hideThrobber") { > .tab-throbber, > .tab-throbber-fallback { > - display: none; > + display: none !important; Normally, I find !important in our CSS as symptomatic of a deeper organizational problem in the code - but as this is strictly for debugging purposes only on Nightly, I think this fine. Thanks!

Comment on attachment 8987571 [details] Bug 1470947 - Part 1: Remove visible tab indicator from all tabs menu https://reviewboard.mozilla.org/r/252814/#review259378 > Can we stop setting the tabIsVisible attribute? This was the only external use of `onPopulate`, so I removed that too.

Comment on attachment 8987695 [details] Bug 1470947 - Part 1: Remove visible tab indicator from all tabs menu https://reviewboard.mozilla.org/r/252928/#review260334

Comment on attachment 8987696 [details] Bug 1470947 - Part 2: Call _tabAttrModified on busy and progress changes https://reviewboard.mozilla.org/r/252930/#review260336

Comment on attachment 8987697 [details] Bug 1470947 - Part 4: Fix the selected tab colour in all tabs menu https://reviewboard.mozilla.org/r/252932/#review260340

Comment on attachment 8987698 [details] Bug 1470947 - Part 5: Remove block padding from scrollbars in all tabs menu https://reviewboard.mozilla.org/r/252934/#review260342 ::: browser/themes/shared/customizableui/panelUI.inc.css:193 (Diff revision 1) > + * however it will have too much top/bottom padding causing the > + * scrollbars to be inset. Remove the padding with negative > + * margin so we stay at 6px padding and the scroll bars reach > + * right to the top/bottom. > + */ > + margin: -6px 0; Can we just drop the padding from the outer panel-subview-body instead?

Comment on attachment 8987698 [details] Bug 1470947 - Part 5: Remove block padding from scrollbars in all tabs menu https://reviewboard.mozilla.org/r/252934/#review260342 > Can we just drop the padding from the outer panel-subview-body instead? So this comment wasn't entirely correct. I revised it to mention toolbarseparators which were causing the issue at the top of this list. Removing the padding from the outer subview body would make the padding incorrect for the toolbarbutton at the top of the outer subview, but it would fix the bottom padding here. I added a new class to remove this padding instead of always doing it for nested `.panel-subview-body` elements.

Comment on attachment 8987698 [details] Bug 1470947 - Part 5: Remove block padding from scrollbars in all tabs menu https://reviewboard.mozilla.org/r/252934/#review263084 ::: browser/themes/shared/customizableui/panelUI.inc.css:192 (Diff revision 2) > + /* When there's an inner subview body it will scroll but the padding > + * adds up with toolbarseparator elements. Remove the padding on the > + * subview body with negative margin to keep the visual padding > + * correct and have scrollbars reach to the top of the view. > + */ > + margin: -6px 0; I'd prefer if you actually removed the relevant paddings from the toolbarseparator and the parent, i.e. set it to 0 rather than compensating it with negative margin.

Comment on attachment 8987698 [details] Bug 1470947 - Part 5: Remove block padding from scrollbars in all tabs menu https://reviewboard.mozilla.org/r/252934/#review263084 > I'd prefer if you actually removed the relevant paddings from the toolbarseparator and the parent, i.e. set it to 0 rather than compensating it with negative margin. Okay, it's been removed from the parent and toolbarseparator.

Comment on attachment 8987698 [details] Bug 1470947 - Part 5: Remove block padding from scrollbars in all tabs menu https://reviewboard.mozilla.org/r/252934/#review263354 ::: browser/base/content/browser-allTabsMenu.inc.xul:21 (Diff revision 3) > <toolbarbutton id="allTabsUndoCloseButton" > class="undo-close-tab subviewbutton subviewbutton-iconic" > label="&undoCloseTab.label;" > key="key_undoCloseTab" > command="History:UndoCloseTab"/> > - <toolbarseparator/> > + <toolbarseparator class="container-tabs-separator"/> Can you file a followup on making this an id instead of a class? Or just fix it here while you're at it. ::: browser/base/content/browser-allTabsMenu.inc.xul:26 (Diff revision 3) > - <toolbarseparator/> > + <toolbarseparator class="container-tabs-separator"/> > <toolbarbutton class="container-tabs-button subviewbutton subviewbutton-nav" > closemenu="none" > oncommand="PanelUI.showSubView('allTabsMenu-containerTabsView', this);" > label="&newUserContext.label;"/> > - <toolbarseparator class="container-tabs-separator"/> > + <toolbarseparator class="hidden-tabs-separator"/> ditto ::: browser/themes/shared/tabs.inc.css:763 (Diff revision 3) > + and the bottom padding from the outer subview so that they don't add up. */ > +#allTabsMenu-allTabsView > .panel-subview-body { > + padding-bottom: 0; > +} > + > +#allTabsMenu-allTabsView > .panel-subview-body > toolbarseparator:last-of-type { Set an id on this separator instead of using :last-of-type?

Comment on attachment 8987698 [details] Bug 1470947 - Part 5: Remove block padding from scrollbars in all tabs menu https://reviewboard.mozilla.org/r/252934/#review263356 ::: browser/themes/shared/tabs.inc.css:758 (Diff revision 3) > max-width: 42em; > } > > +/* There is an inner panel-subview-body for the list of tabs which will have > + top/bottom padding. Remove the bottom margin from the last toolbarseparator > + and the bottom padding from the outer subview so that they don't add up. */ *sigh* Thanks, reviewboard, for eating my comment. Let's try again. I don't think the problem is that the padding and margin add up, but that we don't want any of that outside of the scrollable area. Can you clarify?

Comment on attachment 8987698 [details] Bug 1470947 - Part 5: Remove block padding from scrollbars in all tabs menu https://reviewboard.mozilla.org/r/252934/#review264300

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 1a051fdad5cc92b5d55da6f7d3eb6620a3f7ff87 -d 86eaab5999fb: rebasing 473032:1a051fdad5cc "Bug 1470947 - Part 1: Remove visible tab indicator from all tabs menu r=dao" merging browser/base/content/browser-allTabsMenu.js merging browser/themes/shared/tabs.inc.css warning: conflicts while merging browser/themes/shared/tabs.inc.css! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

Pushed by mstriemer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/618784ce43c7 Part 1: Remove visible tab indicator from all tabs menu r=dao https://hg.mozilla.org/integration/autoland/rev/82b3e2662673 Part 2: Call _tabAttrModified on busy and progress changes r=dao https://hg.mozilla.org/integration/autoland/rev/d1cc03b140c5 Part 3: Ensure tab throbbers are always hidden with browser.tabs.hideThrobber r=mconley https://hg.mozilla.org/integration/autoland/rev/c4adb0c315e5 Part 4: Fix the selected tab colour in all tabs menu r=dao https://hg.mozilla.org/integration/autoland/rev/2b968133583f Part 5: Remove block padding from scrollbars in all tabs menu r=dao

https://hg.mozilla.org/mozilla-central/rev/618784ce43c7 https://hg.mozilla.org/mozilla-central/rev/82b3e2662673 https://hg.mozilla.org/mozilla-central/rev/d1cc03b140c5 https://hg.mozilla.org/mozilla-central/rev/c4adb0c315e5 https://hg.mozilla.org/mozilla-central/rev/2b968133583f

Can the current tab highlight be full line instead of just a blue line at the start? It's difficult to find at once the current tab now that the thin scrollbar and highlight is gone.

(In reply to Firefox_Ninja from comment #48) > Can the current tab highlight be full line instead of just a blue line at > the start? > It's difficult to find at once the current tab now that the thin scrollbar > and highlight is gone. The reason it is hard to find is that the selected tab text is not bold as it should be. see https://bugzilla.mozilla.org/show_bug.cgi?id=1478708

This patch is rebased onto beta and squashed with fixes for bug 1477199 (rebase problem) and bug 1477793 (using wrong id for insertion). Approval Request Comment [Feature/Bug causing the regression]: bug 1446101 [User impact if declined]: There are some styling issues in the all tabs menu which can make it difficult to use [Is this code covered by automated tests?]: No [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: [List of other uplifts needed for the feature/fix]: [Is the change risky?]: No [Why is the change risky/not risky?]: Verified on Nightly and by myself locally on Beta [String changes made/needed]: None

Comment on attachment 8996346 [details] [diff] [review] bug-1470947-combined.patch Improves all tabs menu usability, has stabilized in Nightly for 2 weeks, Beta62+ NI Andrei for SV regression testing. Thanks!

(In reply to Ritu Kothari (:ritu) from comment #51) > NI Andrei for SV regression testing. Thanks! Roger that. Forwarding ni? to Cornel and Bogdan, to make sure this is added to Release QA's priorities for this week.

https://hg.mozilla.org/releases/mozilla-beta/rev/73061be36d6f https://hg.mozilla.org/releases/mozilla-beta/rev/cc5cd2afe96f https://hg.mozilla.org/releases/mozilla-beta/rev/a8ea40cfeaf2 https://hg.mozilla.org/releases/mozilla-beta/rev/4d020a78c00f https://hg.mozilla.org/releases/mozilla-beta/rev/c583095f5b08

We included an exploratory session around the fixes from this bug along tests for toolbars and window controls during validation of 62.0b14 and no new issues were found. Note that *browser.tabs.hideThrobber* does not work in Beta so we couldn't verify that area. Testing was done across platforms (Windows 7, Windows 8.1, macOS 10.10 and Ubuntu 16.04) using 62.0b14.