Closed Bug 1288401 Opened 3 years ago Closed 3 years ago

Inspector sidebar tabs are missing the :active styling

Categories

(DevTools :: Inspector, defect, P1)

50 Branch
defect

Tracking

(firefox50+ verified, firefox51 verified)

VERIFIED FIXED
Firefox 51
Iteration:
51.1 - Aug 15
Tracking Status
firefox50 + verified
firefox51 --- verified

People

(Reporter: pbro, Assigned: steveck)

References

Details

(Keywords: regression, Whiteboard: [reserve-html] [good taipei bug])

Attachments

(2 files)

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.
Whiteboard: [devtools-html] [triage]
Whiteboard: [devtools-html] [triage] → [devtools-html] [triage][good taipei bug]
Flags: qe-verify+
Priority: -- → P3
QA Contact: alexandra.lucinet
Whiteboard: [devtools-html] [triage][good taipei bug] → [reserve-html] [good taipei bug]
Continue the work from bug 1287371
Assignee: nobody → schung
Status: NEW → ASSIGNED
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.
Attachment #8774677 - Flags: feedback?(ntim.bugs)
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.
Attachment #8774677 - Flags: feedback?(ntim.bugs) → feedback+
(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.
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 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.
Attachment #8776869 - Flags: review?(ntim.bugs)
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/
Attachment #8776869 - Flags: review?(ntim.bugs)
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!
Attachment #8776869 - Flags: review?(ntim.bugs) → review+
Thanks!
Keywords: checkin-needed
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/809f50d9fc9f
Inspector sidebar tabs are missing the :active styling. r=ntim
Keywords: checkin-needed
[Tracking Requested - why for this release]: Regression
Version: 49 Branch → 50 Branch
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
Attachment #8776869 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/809f50d9fc9f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Iteration: --- → 51.1 - Aug 15
Priority: P3 → P1
Comment on attachment 8776869 [details]
Bug 1288401 - Inspector sidebar tabs are missing the :active styling.

regression, CSS only, Aurora50+
Attachment #8776869 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Contact: adalucin → cristian.comorasu
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 .
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.