Closed Bug 1470947 Opened 6 years ago Closed 6 years ago

Visual regressions in new all tabs panel

Categories

(Firefox :: Tabbed Browser, defect, P2)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- verified
firefox63 --- fixed

People

(Reporter: mstriemer, Assigned: mstriemer)

References

Details

(Keywords: regression)

Attachments

(8 files, 4 obsolete files)

Bug 1470947 - Part 3: Ensure tab throbbers are always hidden with browser.tabs.hideThrobber
59 bytes, text/x-review-board-request
Details
throbbers-with-progress.png
124.13 KB, image/png
Details
scrollbars-selected-fixed-visible-removed.png
209.31 KB, image/png
Details
Bug 1470947 - Part 1: Remove visible tab indicator from all tabs menu
59 bytes, text/x-review-board-request
dao
: review+
Details
Bug 1470947 - Part 2: Call _tabAttrModified on busy and progress changes
59 bytes, text/x-review-board-request
dao
: review+
Details
Bug 1470947 - Part 4: Fix the selected tab colour in all tabs menu
59 bytes, text/x-review-board-request
dao
: review+
Details
Bug 1470947 - Part 5: Remove block padding from scrollbars in all tabs menu
59 bytes, text/x-review-board-request
dao
: review+
Details
bug-1470947-combined.patch
11.03 KB, patch
ritu
: approval-mozilla-beta+
Details | Diff | Splinter Review 
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
Blocks: 1446101
No longer depends on: 1446101
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.
Attachment #8987573 - Flags: review?(jaws) → review?(mconley)
(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?
Flags: needinfo?(mstriemer)
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?
Attachment #8987571 - Flags: review?(dao+bmo)
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?
Attachment #8987572 - Flags: review?(dao+bmo)
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
Attachment #8987574 - Flags: review?(dao+bmo) → review+
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!
Attachment #8987573 - Flags: review?(mconley) → review+
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.
Attachment #8987571 - Attachment is obsolete: true
Attachment #8987571 - Flags: review?(dao+bmo)
Attachment #8987572 - Attachment is obsolete: true
Attachment #8987572 - Flags: review?(dao+bmo)
Attachment #8987574 - Attachment is obsolete: true
Attachment #8987575 - Attachment is obsolete: true
Attachment #8987575 - Flags: review?(dao+bmo)
Comment on attachment 8987695 [details]
Bug 1470947 - Part 1: Remove visible tab indicator from all tabs menu

https://reviewboard.mozilla.org/r/252928/#review260334
Attachment #8987695 - Flags: review?(dao+bmo) → review+
Comment on attachment 8987696 [details]
Bug 1470947 - Part 2: Call _tabAttrModified on busy and progress changes

https://reviewboard.mozilla.org/r/252930/#review260336
Attachment #8987696 - Flags: review?(dao+bmo) → review+
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
Attachment #8987697 - Flags: review?(dao+bmo) → review+
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?
Attachment #8987698 - Flags: review?(dao+bmo) → review-
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.
Attachment #8987698 - Flags: review?(dao+bmo) → review-
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?
Attachment #8987698 - Flags: review?(dao+bmo)
Flags: needinfo?(mstriemer)
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?
Blocks: 1474352
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
Attachment #8987698 - Flags: review?(dao+bmo) → review+
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
Depends on: 1476991
Depends on: 1477199
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.
Flags: needinfo?(mjaritz)
Depends on: 1477591
Depends on: 1477594
Depends on: 1477793
No longer blocks: 1478708
Depends on: 1478708
No longer depends on: 1478708
(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
Flags: needinfo?(mjaritz)
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
Attachment #8996346 - Flags: approval-mozilla-beta?
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!
Flags: needinfo?(andrei.vaida)
Attachment #8996346 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: needinfo?(cornel.ionce)
Flags: needinfo?(bogdan.maris)
Flags: needinfo?(andrei.vaida)
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.
status-firefox62: fixed → verified
Flags: needinfo?(cornel.ionce)
Flags: needinfo?(bogdan.maris)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: