1474728 - All tabs menu styles missing for keyboard navigation

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Mark Striemer [:mstriemer]]

When navigating the all tabs menu with the keyboard there are no focus styles for the list of tabs in the main menu or hidden tabs menu.

Comment 1

[Mark Striemer [:mstriemer]]

Attachment: Bug 1474728 - Fix focus styles in all tabs menu

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

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 8991122 [details]
Bug 1474728 - Fix focus styles in all tabs menu

https://reviewboard.mozilla.org/r/256102/#review263082

::: browser/themes/shared/tabs.inc.css:795
(Diff revision 1)
>  .all-tabs-item[selected][tabIsVisible] {
>    box-shadow: inset -4px 0 ThreeDShadow, inset 4px 0 var(--blue-40);
>  }
>  
> -.all-tabs-button,
> -.all-tabs-secondary-button {
> +.all-tabs-item:hover > .all-tabs-button:hover,
> +.all-tabs-item:hover > .all-tabs-secondary-button:hover {

What's the point of the .all-tabs-item:hover part? :hover on a child already implies :hover on a parent, doesn't it?

Generally, can you please document what this code is meant to do?

Comment 3

[Mark Striemer [:mstriemer]]

Comment on attachment 8991122 [details]
Bug 1474728 - Fix focus styles in all tabs menu

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

Comment 4

[Mark Striemer [:mstriemer]]

Comment on attachment 8991122 [details]
Bug 1474728 - Fix focus styles in all tabs menu

https://reviewboard.mozilla.org/r/256102/#review263082

> What's the point of the .all-tabs-item:hover part? :hover on a child already implies :hover on a parent, doesn't it?
> 
> Generally, can you please document what this code is meant to do?

That was needed for specificity. I removed the double :hover and left it as !important, but moved it next to the item's :hover style.

Comment 5

[Mark Striemer [:mstriemer]]

Attachment: all-tabs-hover-keyboard.mov.gif

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8991122 [details]
Bug 1474728 - Fix focus styles in all tabs menu

https://reviewboard.mozilla.org/r/256102/#review263358

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf6c59d38c9c
Fix focus styles in all tabs menu r=dao

Comment 8

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/bf6c59d38c9c

Comment 9

[CosminB]

Attachment: Bug1474728.gif

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

Please see the attached video.

Comment 10

[Mark Striemer [:mstriemer]]

Comment on attachment 8991122 [details]
Bug 1474728 - Fix focus styles in all tabs menu

Approval Request Comment
[Feature/Bug causing the regression]: bug 1446101
[User impact if declined]: Keyboard navigation in the all tabs menu is not visible
[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]: 
  1. Open enough tabs (or hide a tab) to see the all tabs button
  2. Open the all tabs menu
  3. Navigate the menu using the arrow keys/space/enter
  4. Verify that tabs are visually highlighted when they are active
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Small CSS change to make a single rule more specific
[String changes made/needed]: No

Comment 11

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

Comment on attachment 8991122 [details]
Bug 1474728 - Fix focus styles in all tabs menu

CSS fix for regression in 62, verified in nightly.  Let's uplift for beta 10.

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/83e495b4c933

Comment 13

[CosminB]

Attachment: Bug1474728.gif

This issue is verified as fixed on Firefox 62.0b10 (20180719140244) under Win 7 64-bit and Mac OS X 10.13.3.

Please see the attached video.