1478708 - selected tab is not marked bold in new all tabs panel

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect, P1)

Description

[Markus Jaritz [:designakt] (UX)]

Attachment: New-All-Tabs-Panel---Selected-Bold.gif

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

Comment 1

[Mark Striemer [:mstriemer]]

Attachment: Bug 1478708 - Part 2: Selected tab text is bold in all tabs menu

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

Comment 2

[Dão Gottwald [:dao]]

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?

Comment 3

[Mark Striemer [:mstriemer]]

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

Comment 4

[Dão Gottwald [:dao]]

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?

Comment 5

[Mark Striemer [:mstriemer]]

Attachment: Bug 1478708 - Part 1: Reduce specificity for toolbarbutton font

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

Comment 6

[Mark Striemer [:mstriemer]]

Comment on attachment 8995672 [details]
Bug 1478708 - Part 2: Selected tab text is bold in all tabs menu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260066/diff/1-2/

Comment 7

[Mark Striemer [:mstriemer]]

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.

Comment 8

[Dão Gottwald [:dao]]

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 9

[Mark Striemer [:mstriemer]]

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 10

[Mark Striemer [:mstriemer]]

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 11

[Mark Striemer [:mstriemer]]

Comment on attachment 8996444 [details]
Bug 1478708 - Part 1: Reduce specificity for toolbarbutton font

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260528/diff/1-2/

Comment 12

[Mark Striemer [:mstriemer]]

Comment on attachment 8995672 [details]
Bug 1478708 - Part 2: Selected tab text is bold in all tabs menu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260066/diff/2-3/

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8996444 [details]
Bug 1478708 - Part 1: Reduce specificity for toolbarbutton font

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

Comment 14

[Dão Gottwald [:dao]]

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]?

Comment 15

[Tim Nguyen :ntim]

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.

Comment 16

[Tim Nguyen :ntim]

(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 17

[Mark Striemer [: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/#review268426

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

The `.subviewbutton { font: menu; }` rule will reset the font-weight to normal on the toolbarbutton.

Comment 18

[Dão Gottwald [:dao]]

(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 19

[Dão Gottwald [:dao]]

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

Comment 20

[Mozilla Autoland]

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)

Comment 21

[Mark Striemer [:mstriemer]]

Comment on attachment 8996444 [details]
Bug 1478708 - Part 1: Reduce specificity for toolbarbutton font

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260528/diff/2-3/

Comment 22

[Mark Striemer [:mstriemer]]

Comment on attachment 8995672 [details]
Bug 1478708 - Part 2: Selected tab text is bold in all tabs menu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260066/diff/3-4/

Comment 23

[Mozilla Autoland]

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)

Comment 24

[Mark Striemer [:mstriemer]]

Comment on attachment 8996444 [details]
Bug 1478708 - Part 1: Reduce specificity for toolbarbutton font

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260528/diff/3-4/

Comment 25

[Mark Striemer [:mstriemer]]

Comment on attachment 8995672 [details]
Bug 1478708 - Part 2: Selected tab text is bold in all tabs menu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260066/diff/4-5/

Comment 26

[Pulsebot]

Pushed by [e-mail]:
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

Comment 27

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/6012ea7adf57
https://hg.mozilla.org/mozilla-central/rev/ef59bc4353ff

Comment 28

[CosminB]

Attachment: 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.

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

Want to request uplift to beta? Since it's a new issue in 62 it would be nice to ship without the problem.

Comment 30

[Mark Striemer [: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]:

Comment 31

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 32

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/313b8e62b6ef
https://hg.mozilla.org/releases/mozilla-beta/rev/723b6cad1824

Comment 33

[Damira Akh]

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