Closed Bug 1478708 Opened Last year Closed Last year

selected tab is not marked bold in new all tabs panel

Categories

(Firefox :: Tabbed Browser, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- verified
firefox63 --- verified

People

(Reporter: designakt, Assigned: mstriemer)

References

Details

(Keywords: regression)

Attachments

(4 files)

To easily identify the selected tab in the all tabs panel, it should be made bold, as in the previous menu.

See attachment and https://mozilla.invisionapp.com/share/KXEUSPHHN#/279576474_Project_Jazz_-_Tab_Hiding_-_Tabs_Hidden_-_Menu_Open_-_Extended
Blocks: 1470947
No longer depends on: 1470947
Blocks: 1446101
No longer blocks: 1470947
Flags: needinfo?(mstriemer)
Keywords: regression
Priority: -- → P2
Assignee: nobody → mstriemer
Flags: needinfo?(mstriemer)
Comment on attachment 8995672 [details]
Bug 1478708 - Part 2: Selected tab text is bold in all tabs menu

https://reviewboard.mozilla.org/r/260066/#review267124

::: browser/themes/shared/tabs.inc.css:803
(Diff revision 1)
>  
>  .all-tabs-item[selected] {
>    box-shadow: inset 4px 0 var(--blue-40);
>  }
>  
> +.all-tabs-item[selected] > .all-tabs-button > .toolbarbutton-text {

Why target .toolbarbutton-text specifically? Can't you just set font-weight on .all-tabs-item[selected] and be done with it?
Attachment #8995672 - Flags: review?(dao+bmo)
Comment on attachment 8995672 [details]
Bug 1478708 - Part 2: Selected tab text is bold in all tabs menu

Unfortunately there's a rule that sets `.subviewbutton:not(.panel-subview-footer) > .toolbarbutton-text` to `font: menu` [1] which resets the font-weight.

[1] https://searchfox.org/mozilla-central/rev/033d45ca70ff32acf04286244644d19308c359d5/browser/themes/shared/customizableui/panelUI.inc.css#958
Attachment #8995672 - Flags: review?(dao+bmo)
Comment on attachment 8995672 [details]
Bug 1478708 - Part 2: Selected tab text is bold in all tabs menu

(In reply to Mark Striemer [:mstriemer] from comment #3)
> Comment on attachment 8995672 [details]
> Bug 1478708 - Bold selected tab in all tabs menu
> 
> Unfortunately there's a rule that sets
> `.subviewbutton:not(.panel-subview-footer) > .toolbarbutton-text` to `font:
> menu` [1] which resets the font-weight.
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> 033d45ca70ff32acf04286244644d19308c359d5/browser/themes/shared/
> customizableui/panelUI.inc.css#958

Can you remove .toolbarbutton-text / .menu-text / .menu-iconic-text from these selectors?
Flags: needinfo?(mstriemer)
Attachment #8995672 - Flags: review?(dao+bmo)
I reduced that rule's selector down quite a bit. It's now `.subviewbutton, .subviewbutton-nav, .addon-banner-item`.

I didn't want to get into the details of `.addon-banner-item`, the `.panel-banner-item` sets the font on an element within it so I just left the `.addon-banner-item` selector in there.

I needed to include `.subviewbutton-nav` because there are some buttons (I think it was bookmarks and library) that only have the subviewbutton-nav class. Adding the subviewbutton class breaks hover over these buttons when they're in the toolbar. Seems to me like the subviewbutton-nav class should be added when it is in a subview, but I didn't want this patch to get too big.
Flags: needinfo?(mstriemer)
Comment on attachment 8996444 [details]
Bug 1478708 - Part 1: Reduce specificity for toolbarbutton font

https://reviewboard.mozilla.org/r/260528/#review268208

::: browser/themes/shared/customizableui/panelUI.inc.css:952
(Diff revision 1)
>  .subviewbutton > .menu-accel-container > .menu-accel {
>    margin-inline-end: 0 !important; /* to override menu.css on Windows */
>  }
>  
> -#widget-overflow-fixed-list .toolbarbutton-1 > .toolbarbutton-text,
> -#widget-overflow-list .toolbarbutton-1 > .toolbarbutton-text,
> +.subviewbutton,
> +.subviewbutton-nav,

Why did you drop #widget-overflow-fixed-list .toolbarbutton-1 and #widget-overflow-list .toolbarbutton-1 from this rule?

::: browser/themes/shared/customizableui/panelUI.inc.css:1096
(Diff revision 1)
>    color: var(--panel-disabled-color);
>  }
>  
>  .subview-subheader,
>  .panel-subview-footer {
>    font: menu;

Can we remove or combine this with the other rule?
Comment on attachment 8996444 [details]
Bug 1478708 - Part 1: Reduce specificity for toolbarbutton font

https://reviewboard.mozilla.org/r/260528/#review268208

> Can we remove or combine this with the other rule?

Searching the codebase it looks like all of the .panel-subview-footer elements also have .subviewbutton, but the .subview-subheader elements don't. I moved the latter selector with the others.
Comment on attachment 8996444 [details]
Bug 1478708 - Part 1: Reduce specificity for toolbarbutton font

https://reviewboard.mozilla.org/r/260528/#review268282

::: browser/themes/shared/customizableui/panelUI.inc.css:952
(Diff revision 1)
>  .subviewbutton > .menu-accel-container > .menu-accel {
>    margin-inline-end: 0 !important; /* to override menu.css on Windows */
>  }
>  
> -#widget-overflow-fixed-list .toolbarbutton-1 > .toolbarbutton-text,
> -#widget-overflow-list .toolbarbutton-1 > .toolbarbutton-text,
> +.subviewbutton,
> +.subviewbutton-nav,

I had checked a handfull of items in there and they all seemed to have .subviewbutton or .subviewbutton-nav. Looks like that isn't guaranteed though, so I added it back.
Comment on attachment 8996444 [details]
Bug 1478708 - Part 1: Reduce specificity for toolbarbutton font

https://reviewboard.mozilla.org/r/260528/#review268424
Attachment #8996444 - Flags: review?(dao+bmo) → review+
Comment on attachment 8995672 [details]
Bug 1478708 - Part 2: Selected tab text is bold in all tabs menu

https://reviewboard.mozilla.org/r/260066/#review268426

::: browser/themes/shared/tabs.inc.css:803
(Diff revision 3)
>  
>  .all-tabs-item[selected] {
>    box-shadow: inset 4px 0 var(--blue-40);
>  }
>  
> +.all-tabs-item[selected] > .all-tabs-button {

Why not just .all-tabs-item[selected]?
Attachment #8995672 - Flags: review?(dao+bmo)
Comment on attachment 8996444 [details]
Bug 1478708 - Part 1: Reduce specificity for toolbarbutton font

https://reviewboard.mozilla.org/r/260528/#review268490

::: browser/themes/shared/customizableui/panelUI.inc.css:955
(Diff revision 2)
>  
>  #widget-overflow-fixed-list .toolbarbutton-1 > .toolbarbutton-text,
>  #widget-overflow-list .toolbarbutton-1 > .toolbarbutton-text,
> -.subviewbutton:not(.panel-subview-footer) > .toolbarbutton-text,
> -.addon-banner-item > .toolbarbutton-text,
> -/* Bookmark items need a more specific selector. */
> +.addon-banner-item,
> +.subviewbutton,
> +.subviewbutton-nav,

Aren't .subviewbutton-nav also .subviewbutton ? If so, this selector is redundant.
(In reply to Tim Nguyen :ntim from comment #15)
> Comment on attachment 8996444 [details]
> Bug 1478708 - Part 1: Reduce specificity for toolbarbutton font
> 
> https://reviewboard.mozilla.org/r/260528/#review268490
> 
> ::: browser/themes/shared/customizableui/panelUI.inc.css:955
> (Diff revision 2)
> >  
> >  #widget-overflow-fixed-list .toolbarbutton-1 > .toolbarbutton-text,
> >  #widget-overflow-list .toolbarbutton-1 > .toolbarbutton-text,
> > -.subviewbutton:not(.panel-subview-footer) > .toolbarbutton-text,
> > -.addon-banner-item > .toolbarbutton-text,
> > -/* Bookmark items need a more specific selector. */
> > +.addon-banner-item,
> > +.subviewbutton,
> > +.subviewbutton-nav,
> 
> Aren't .subviewbutton-nav also .subviewbutton ? If so, this selector is
> redundant.

Nevermind, I read comment 7 :)
Comment on attachment 8995672 [details]
Bug 1478708 - Part 2: Selected tab text is bold in all tabs menu

https://reviewboard.mozilla.org/r/260066/#review268426

> Why not just .all-tabs-item[selected]?

The `.subviewbutton { font: menu; }` rule will reset the font-weight to normal on the toolbarbutton.
Attachment #8995672 - Flags: review?(dao+bmo)
(In reply to Mark Striemer [:mstriemer] from comment #17)
> Comment on attachment 8995672 [details]
> Bug 1478708 - Part 2: Selected tab text is bold in all tabs menu
> 
> https://reviewboard.mozilla.org/r/260066/#review268426
> 
> > Why not just .all-tabs-item[selected]?
> 
> The `.subviewbutton { font: menu; }` rule will reset the font-weight to
> normal on the toolbarbutton.

*Sigh* Okay.

I'll look into doing some further cleanup in a followup.
Comment on attachment 8995672 [details]
Bug 1478708 - Part 2: Selected tab text is bold in all tabs menu

https://reviewboard.mozilla.org/r/260066/#review268694
Attachment #8995672 - 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 4223dbf969e471ea28553214c61b43f7df66a309 -d 51c01db0c02f: rebasing 476958:4223dbf969e4 "Bug 1478708 - Part 1: Reduce specificity for toolbarbutton font r=dao"
rebasing 476959:024a87ca8aee "Bug 1478708 - Part 2: Selected tab text is bold in all tabs menu r=dao" (tip)
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)
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 6e61d3cdc3f67fcc39ee17623bc8256c4aaa5a19 -d d0fe7d0b2a28: rebasing 477025:6e61d3cdc3f6 "Bug 1478708 - Part 1: Reduce specificity for toolbarbutton font r=dao"
rebasing 477026:30c368989f7e "Bug 1478708 - Part 2: Selected tab text is bold in all tabs menu r=dao" (tip)
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/6012ea7adf57
Part 1: Reduce specificity for toolbarbutton font r=dao
https://hg.mozilla.org/integration/autoland/rev/ef59bc4353ff
Part 2: Selected tab text is bold in all tabs menu r=dao
https://hg.mozilla.org/mozilla-central/rev/6012ea7adf57
https://hg.mozilla.org/mozilla-central/rev/ef59bc4353ff
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Attached image Bug1478708.gif
I was able to reproduce this issue on Firefox 62.0b15 (20180806191531) under Win 7 64-bit and Mac OS X 10.13.3.

This issue is verified as fixed on Firefox 63.0a1(20180807220134) under Win 7 64-bit and Mac OS X 10.13.3.

Please see the attached video.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Blocks: 1481754
Want to request uplift to beta? Since it's a new issue in 62 it would be nice to ship without the problem.
Flags: needinfo?(mstriemer)
Comment on attachment 8995672 [details]
Bug 1478708 - Part 2: Selected tab text is bold in all tabs menu

Approval Request Comment
[Feature/Bug causing the regression]: bug 1446101
[User impact if declined]: It is more difficult to find the active tab in the all tabs menu
[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?]: CSS only changes
[String changes made/needed]:
Flags: needinfo?(mstriemer)
Attachment #8995672 - Flags: approval-mozilla-beta?
Comment on attachment 8995672 [details]
Bug 1478708 - Part 2: Selected tab text is bold in all tabs menu

CSS fix for all tabs menu, OK to uplift for beta 18.
Attachment #8995672 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I was able to reproduce this issue on Firefox 62.0b7(20180709172241) under Win 10x64, Mac OS X 10.13 and Ubuntu 18.04

This issue is verified as FIXED on Firefox 62.0b18(20180816151750) under Win 7 64-bit, Mac OS X 10.13 and Ubuntu 18.04
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.