1288401 - Inspector sidebar tabs are missing the :active styling

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Patrick Brosset <:pbro>]

In bug 1259819, the sidebar tabs have been migrated to HTML.
I think we lost a bit of CSS styling in the process.

Take the toolbox tabs for example (the inspector, console, ...).
On hover they get a different color.
And when you click on them (and hold the mouse button down), they get yet another color.

Now, look at the inspector sidebar tabs.
They do get the right hover color.
But the active color is gone.

Comment 1

[Steve Chung [:steveck]]

Continue the work from bug 1287371

Comment 2

[Steve Chung [:steveck]]

Attachment: bug-1288401.patch

Hi Tim, I reused the existing toolbar-tab-hover-active variable and modified the tab color for dark theme, since the color should be different between hover and original state in the previous version.

Comment 3

[Tim Nguyen :ntim]

Comment on attachment 8774677 [details] [diff] [review]
bug-1288401.patch

Review of attachment 8774677 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine, but the selected tab seems to have an unwanted active state in dark and light themes.

Comment 4

[Steve Chung [:steveck]]

Attachment: Bug 1288401 - Inspector sidebar tabs are missing the :active styling.

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

Comment 5

[Steve Chung [:steveck]]

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #3)
> Comment on attachment 8774677 [details] [diff] [review]
> bug-1288401.patch
> 
> Review of attachment 8774677 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine, but the selected tab seems to have an unwanted active state in
> dark and light themes.

Nice catch! I changed the setting of bgColor from inner anchor to tabs menu item and forced the bgColor set to the selected tab still the same even with active/hover pseudo classes. Now the tab behavior should be the same between inspector toolbar and main toolbar.

Comment 6

[Tim Nguyen :ntim]

https://reviewboard.mozilla.org/r/68530/#review65650

::: devtools/client/shared/components/tabs/tabs.css:92
(Diff revision 1)
>  .theme-dark .tabs .tabs-menu-item.is-active,
>  .theme-dark .tabs .tabs-menu-item.is-active:hover,
> +.theme-dark .tabs .tabs-menu-item.is-active:hover:active,
>  .theme-light .tabs .tabs-menu-item.is-active,
> -.theme-light .tabs .tabs-menu-item.is-active:hover {
> +.theme-light .tabs .tabs-menu-item.is-active:hover,
> +.theme-light .tabs .tabs-menu-item.is-active:hover:active {

Instead of duplicating .is-active for :hover and :hover:active, can you add `:not(.is-active)ˋ to the :hover and :hover:active rules defined above?

Comment 7

[Tim Nguyen :ntim]

Comment on attachment 8776869 [details]
Bug 1288401 - Inspector sidebar tabs are missing the :active styling.

https://reviewboard.mozilla.org/r/68530/#review65798

Looks good to me otherwise.

Comment 8

[Steve Chung [:steveck]]

Comment on attachment 8776869 [details]
Bug 1288401 - Inspector sidebar tabs are missing the :active styling.

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

Comment 9

[Tim Nguyen :ntim]

Comment on attachment 8776869 [details]
Bug 1288401 - Inspector sidebar tabs are missing the :active styling.

https://reviewboard.mozilla.org/r/68530/#review65814

Looks good to me!

Comment 10

[Steve Chung [:steveck]]

Thanks!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/809f50d9fc9f
Inspector sidebar tabs are missing the :active styling. r=ntim

Comment 12

[Tim Nguyen :ntim]

[Tracking Requested - why for this release]: Regression

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 8776869 [details]
Bug 1288401 - Inspector sidebar tabs are missing the :active styling.

Approval Request Comment
[Feature/regressing bug #]: bug 1259819
[User impact if declined]: See comment #0
[Describe test coverage new/current, TreeHerder]: landed on fx-team
[Risks and why]: low, css only
[String/UUID change made/needed]: no

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/809f50d9fc9f

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8776869 [details]
Bug 1288401 - Inspector sidebar tabs are missing the :active styling.

regression, CSS only, Aurora50+

Comment 16

[Tim Nguyen :ntim]

https://hg.mozilla.org/releases/mozilla-aurora/rev/89b4dd71d6a2

Comment 17

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I did not find any issues while verifying this fix with the latest Nightly 50.0a1 and Aurora 51.0a1, using the following platforms: Windows 10 x64, Ubuntu 14.04 and Max OS X 10.0.5 .