Closed Bug 1287371 Opened 3 years ago Closed 3 years ago

Tabs hover style was changed in firebug theme

Categories

(DevTools :: Inspector, defect, P1)

defect

Tracking

(firefox50 verified)

VERIFIED FIXED
Firefox 50
Iteration:
50.4 - Aug 1
Tracking Status
firefox50 --- verified

People

(Reporter: magicp.jp, Assigned: steveck)

References

Details

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

Attachments

(3 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20160717030211

Steps to reproduce:

1. Start Nightly
2. Open DevTools > Inspector with Firebug theme
3. Check hover style for inspector sidebar tabs 


Actual results:

Tabs hover style was changed in firebug theme

Regression range:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=a8ab215f15d3a875f964663a6011b5a6cba67919&tochange=453c308dcab1e3236fde0c4ee2e6d0d3d2d60dae


Expected results:

Is it intentional? If it is not, apply previous hover style.
Blocks: 1259819
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Developer Tools: Inspector
OS: Unspecified → All
Hardware: Unspecified → All
Whiteboard: [devtools-html] [triage]
Flags: qe-verify+
Priority: -- → P3
QA Contact: alexandra.lucinet
Whiteboard: [devtools-html] [triage] → [reserve-html]
Whiteboard: [reserve-html] → [reserve-html][good taipei bug]
Attached patch bug-1287371.patch (obsolete) — Splinter Review
Hi Patrick, this patch resumed the old firebug theme style and fix an unrelated issue(tab dotted outline). I'm not sure whether we should apply the old style, maybe we need visual feedback for this?
Attachment #8772782 - Flags: feedback?(pbrosset)
add hover state via devtool to the li elementlooks fine, 
but in real case the hover state is applied to `a` link element instead of the li element
I found another lost state. :hover:active Thanks.
Comment on attachment 8772782 [details] [diff] [review]
bug-1287371.patch

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

::: devtools/client/shared/components/tabs/tabs.css
@@ +29,5 @@
>    white-space: nowrap;
>  }
>  
> +.tabs .tabs-menu-item a:focus {
> +  outline: none;

I might have missed something, but why are you removing the focus outline on the tab?
For accessibility reasons, we want to have dotted outlines around our widgets, so its easier to see where the focus is.

@@ +145,5 @@
>  
>  .theme-firebug .tabs .tabs-menu-item:hover a {
>    border: 1px solid #C8C8C8;
>    border-bottom: 1px solid transparent;
> +  background-color: rgba(0, 0, 0, 0.12);

I don't think this is the right color, and we're missing a border.
The tabs in the sidebar should look exactly the same as the tabs in the toolbox (the ones where the inspector, console, debugger, etc. are).
So, in the toolbox, if I hover over a tab, I see a border around it, and a light grey background.
With this change, I only a darker background, and no border.

Here's a comparison screenshot:
https://dl.dropboxusercontent.com/u/714210/firebug-tab-hover-style.png
Attachment #8772782 - Flags: feedback?(pbrosset)
Hi Steve, Thanks for your work!
Can we use variable declarations in tabs.css?

For example:
background-color: var(--theme-selection-background);
color: var(--theme-selection-color);
Comment on attachment 8773682 [details]
Bug 1287371 - Tabs hover style was changed in firebug theme.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66444/diff/1-2/
(In reply to magicp from comment #6)
> Hi Steve, Thanks for your work!
> Can we use variable declarations in tabs.css?
> 
> For example:
> background-color: var(--theme-selection-background);
> color: var(--theme-selection-color);
Thanks for the suggestion, you're right that it should apply variable here.

Hi Tim, since Patrick is on PTO, it would be great if you could review the patch because you have many contributions in devtool's layout. Just a quick recap for the patch: I simply removed the original transparent border style and applied the firebug selection style for hover + active status.
Comment on attachment 8773682 [details]
Bug 1287371 - Tabs hover style was changed in firebug theme.

https://reviewboard.mozilla.org/r/66444/#review63154

The patch itself looks fine, but it seems like that the patch here belongs in bug 1288401.
Not sure when the issue in attachment 8771872 [details] was fixed, but it seems fixed locally for me.
Attachment #8773682 - Flags: review?(ntim.bugs) → review+
Assignee: nobody → schung
Status: NEW → ASSIGNED
(In reply to Tim Nguyen :ntim (use needinfo?) from comment #9)
> Comment on attachment 8773682 [details]
> Bug 1287371 - Tabs hover style was changed in firebug theme.
> 
> https://reviewboard.mozilla.org/r/66444/#review63154
> 
> The patch itself looks fine, but it seems like that the patch here belongs
> in bug 1288401.

Yeah, I'll also fix the active style for other two themes.

> Not sure when the issue in attachment 8771872 [details] was fixed, but it
> seems fixed locally for me.

Hmmm, this issue seems only be reproduced only after bug 1259819, which just landed about week ago. It still existed in my latest build. Maybe you could rebuild and give it a try.
Keywords: checkin-needed
Attachment #8772782 - Attachment is obsolete: true
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/5beb04588882
Tabs hover style was changed in firebug theme. r=ntim
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5beb04588882
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Iteration: --- → 50.4 - Aug 1
Priority: P3 → P1
I have reproduced this bug with Nightly 50.0a1 (2016-07-17) on Windows 8.1 , 64 bit!

This Bug's fix is verified on Latest Nightly 50.0a1.

Build ID : 20160727030230
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0

[bugday-20160727]
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.