Closed Bug 1288963 Opened 3 years ago Closed 3 years ago

Fix inversion of various icons in the shader editor, notification box and memory tool

Categories

(DevTools :: General, defect, P2)

defect

Tracking

(firefox49 unaffected, firefox50 verified)

VERIFIED FIXED
Firefox 50
Tracking Status
firefox49 --- unaffected
firefox50 --- verified

People

(Reporter: magicp.jp, Assigned: ntim)

References

Details

Attachments

(2 files, 3 obsolete files)

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

Steps to reproduce:

1. Start Nightly
2. Go to about:home
3. Open DevTools > Canvas
4. Record animation frame > Warning message appears (this is right behavior)



Actual results:

messageCloseButton does not have appropriate contrast color for background-color.

Regression range:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=664bd6aa11e1c9e53f5e6e7bca4990265b563802&tochange=7ba94e0c5daa48d7667519c930eca31d98648512


Expected results:

Don't invert color of messageCloseButton for warning.
Blocks: 1268591
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Developer Tools: Canvas Debugger
OS: Unspecified → All
Hardware: Unspecified → All
Summary: Don't invert color of messageCloseButton for warning → Fix invertion of various icons in the shader editor, notification box, memory tool and filter widget
Looks like there are a bunch of hardcoded filter: invert(1) in devtools which are causing this: https://dxr.mozilla.org/mozilla-central/search?q=filter%3A+invert(1&redirect=false
Assignee: nobody → ntim.bugs
Component: Developer Tools: Canvas Debugger → Developer Tools
Priority: -- → P2
Summary: Fix invertion of various icons in the shader editor, notification box, memory tool and filter widget → Fix inversion of various icons in the shader editor, notification box, memory tool and filter widget
We need to get this in by the end of week for it to ride the train.  Tim, is that something you'll be able to do?
Flags: needinfo?(ntim.bugs)
(In reply to Brian Grinstead [:bgrins] from comment #2)
> We need to get this in by the end of week for it to ride the train.  Tim, is
> that something you'll be able to do?

Yeah.
Julian took care of the filter widget in bug 1288375.
Flags: needinfo?(ntim.bugs)
Summary: Fix inversion of various icons in the shader editor, notification box, memory tool and filter widget → Fix inversion of various icons in the shader editor, notification box and memory tool
Attached patch Patch (obsolete) — Splinter Review
Attachment #8775082 - Flags: review?(bgrinstead)
Comment on attachment 8775082 [details] [diff] [review]
Patch

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

Just one note about hover / active backgrounds, the patch generally looks right so r=me once this is addressed

::: devtools/client/shared/components/notification-box.css
@@ +89,4 @@
>  }
>  
>  .notificationbox .messageCloseButton:hover {
> +  background-color: #aaaa;

I don't think these colors are used anywhere else in the theme.  I'd prefer to borrow colors used elsewhere, like the toolbox tabs hover / active background for light theme:

  --toolbar-tab-hover: rgba(170, 170, 170, .2);
  --toolbar-tab-hover-active: rgba(170, 170, 170, .4);

(note we can't use the actual variables here, just the colors themselves)
Attachment #8775082 - Flags: review?(bgrinstead)
Attached patch Patch (obsolete) — Splinter Review
Attachment #8775082 - Attachment is obsolete: true
Attachment #8776014 - Flags: review?(bgrinstead)
Attached patch Patch (obsolete) — Splinter Review
Attachment #8776014 - Attachment is obsolete: true
Attachment #8776014 - Flags: review?(bgrinstead)
Attachment #8776017 - Flags: review?(bgrinstead)
Attached patch PatchSplinter Review
Forgot to qref
Attachment #8776017 - Attachment is obsolete: true
Attachment #8776017 - Flags: review?(bgrinstead)
Attachment #8776018 - Flags: review?(bgrinstead)
Comment on attachment 8776018 [details] [diff] [review]
Patch

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

::: devtools/client/themes/shadereditor.css
@@ +65,5 @@
>    background-position: 0 0;
>  }
>  
> +.side-menu-widget-item:not(.selected) .checkbox-check {
> +  filter: var(--icon-filter);

I wasn't able to test this really - I wasn't seeing any image at all in the shared editor for the checkbox.  Is this just something weird I'm seeing locally?
Attachment #8776018 - Flags: review?(bgrinstead) → review+
(In reply to Brian Grinstead [:bgrins] from comment #10)
> Comment on attachment 8776018 [details] [diff] [review]
> Patch
> 
> Review of attachment 8776018 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/themes/shadereditor.css
> @@ +65,5 @@
> >    background-position: 0 0;
> >  }
> >  
> > +.side-menu-widget-item:not(.selected) .checkbox-check {
> > +  filter: var(--icon-filter);
> 
> I wasn't able to test this really - I wasn't seeing any image at all in the
> shared editor for the checkbox.  Is this just something weird I'm seeing
> locally?

You to need to run the shader editor on a webgl enabled page with shaders defined on it.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/ab643d9880b4
Fix inversion of various devtools icons. r=bgrins
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ab643d9880b4
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Depends on: 1296331
I have reproduced this Bug on Nightly 50.0a1 (2016-07-24) on Windows 10, 64 Bit!

The bug's fix is now verified on latest Firefox Developer Edition 50.0a2 

Aurora   50.0a2 :
Build ID	   : 20160825004011
User Agent 	   : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0

 [testday-20160826]
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.