Tabs hover style was changed in firebug theme

VERIFIED FIXED in Firefox 50

Status

P1
normal
VERIFIED FIXED
2 years ago
2 months ago

People

(Reporter: magicp.jp, Assigned: steveck)

Tracking

Trunk
Firefox 50
Dependency tree / graph

Firefox Tracking Flags

(firefox50 verified)

Details

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

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Created attachment 8771872 [details]
tabs-hover-style-was-changed-in-firebug-theme.png

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.
(Reporter)

Updated

2 years ago
Blocks: 1259819
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox50: --- → affected
Component: Untriaged → Developer Tools: Inspector
OS: Unspecified → All
Hardware: Unspecified → All

Updated

2 years ago
Blocks: 1263741
Whiteboard: [devtools-html] [triage]

Updated

2 years ago
Flags: qe-verify+
Priority: -- → P3
QA Contact: alexandra.lucinet
Whiteboard: [devtools-html] [triage] → [reserve-html]
Whiteboard: [reserve-html] → [reserve-html][good taipei bug]
(Assignee)

Comment 1

2 years ago
Created attachment 8772782 [details] [diff] [review]
bug-1287371.patch

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)

Comment 2

2 years ago
Created attachment 8772795 [details]
add hover state via devtool

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
(Reporter)

Comment 3

2 years ago
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)
(Assignee)

Comment 5

2 years ago
Created attachment 8773682 [details]
Bug 1287371 - Tabs hover style was changed in firebug theme.

Review commit: https://reviewboard.mozilla.org/r/66444/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66444/
Attachment #8773682 - Flags: review?(ntim.bugs)
(Reporter)

Comment 6

2 years ago
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);
(Assignee)

Comment 7

2 years ago
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/
(Assignee)

Comment 8

2 years ago
(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 9

2 years ago
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+

Updated

2 years ago
Assignee: nobody → schung
Status: NEW → ASSIGNED
(Assignee)

Comment 10

2 years ago
(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
(Assignee)

Updated

2 years ago
Attachment #8772782 - Attachment is obsolete: true

Comment 11

2 years ago
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

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5beb04588882
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50

Updated

2 years ago
Iteration: --- → 50.4 - Aug 1
Priority: P3 → P1

Comment 13

2 years ago
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]

Updated

2 years ago
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified

Updated

2 years ago
Flags: qe-verify+

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.