Closed Bug 1254261 Opened 4 years ago Closed 4 years ago

Toolbar button icons no longer displayed

Categories

(DevTools :: Responsive Design Mode, defect, P1)

defect

Tracking

(firefox47 affected, firefox48 fixed)

VERIFIED FIXED
Firefox 48
Iteration:
48.1 - Mar 21
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

Details

(Whiteboard: [multiviewport] [mvp-rdm])

Attachments

(1 file)

It looks like the CSS mask-image support we were using has been disabled for now.

Let's try another approach to get the icons displayed again.
Flags: qe-verify?
:bgrins, flagging you for review in case you see a better way to do this.  :gl and I weren't sure what the best way to pull in the SVG icons would be, while still maintaining some way to style the color.

Options discussed include:

* The attached <use> tags for each theme and mode approach
* Background positioning to slice different bits out of the SVG
* Embedding the SVG content into the main document

Each has trade offs, not really seeing one as the best way myself.
Flags: qe-verify? → qe-verify+
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #2)
> :bgrins, flagging you for review in case you see a better way to do this. 
> :gl and I weren't sure what the best way to pull in the SVG icons would be,
> while still maintaining some way to style the color.
> 
> Options discussed include:
> 
> * The attached <use> tags for each theme and mode approach
> * Background positioning to slice different bits out of the SVG
> * Embedding the SVG content into the main document
> 
> Each has trade offs, not really seeing one as the best way myself.

We've mostly avoided this problem so far by using identical selection colors across themes and doing a CSS inversion filter for the light theme.  Would this be an option for buttons in the RDM panel?  Seems like it'd be OK to match the command icon styling for light/dark theme.
Flags: needinfo?(jryans)
Iteration: 47.3 - Mar 7 → 48.1 - Mar 21
Whiteboard: [multiviewport] → [multiviewport] [mvp-rdm]
QA Contact: mihai.boldan
Comment on attachment 8727567 [details]
MozReview Request: Bug 1254261 - Restore icons by using command button styles. r=gl,bgrins

https://reviewboard.mozilla.org/r/38511/#review35463

::: devtools/client/responsive.html/index.css:64
(Diff revision 1)
> -  background-color: var(--theme-body-color);
> +  background-color: var(--theme-toolbar-background);

I think we should use "background-color: transparent" here instead so we only have to change the background-color in one place for the button container.
Attachment #8727567 - Flags: review?(gl) → review+
Please avoid adding icon sprites (or using <use>) when you can, these are not very good for perf.

I think we should be consistent with what we do inside the toolbox: we use a white icon, then invert it to get a light theme icon, and we apply a blue color filter on it when it's checked. Maybe the blue color doesn't match exactly the wanted color, but at least it's shared with the toolbox, and we can update everything in one place.
Comment on attachment 8727567 [details]
MozReview Request: Bug 1254261 - Restore icons by using command button styles. r=gl,bgrins

Agree with Comment 5.  I believe 'whitesmoke' is the color we've been using
Attachment #8727567 - Flags: review?(bgrinstead) → review-
Correction: Not whitesmoke, but #EAF5FC at least for the command icons.  As taken from: https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/images/command-paintflashing.png
Comment on attachment 8727567 [details]
MozReview Request: Bug 1254261 - Restore icons by using command button styles. r=gl,bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38511/diff/1-2/
Attachment #8727567 - Attachment description: MozReview Request: Bug 1254261 - Restore icons by embedding fill colors in SVG. r=gl,bgrins → MozReview Request: Bug 1254261 - Restore icons by using command button styles. r=gl,bgrins
Attachment #8727567 - Flags: review- → review?(bgrinstead)
Comment on attachment 8727567 [details]
MozReview Request: Bug 1254261 - Restore icons by using command button styles. r=gl,bgrins

Okay, this version uses the command button filters, etc.
Flags: needinfo?(jryans)
Attachment #8727567 - Flags: review+ → review?(gl)
Comment on attachment 8727567 [details]
MozReview Request: Bug 1254261 - Restore icons by using command button styles. r=gl,bgrins

https://reviewboard.mozilla.org/r/38511/#review35661

Works for me
Attachment #8727567 - Flags: review?(bgrinstead) → review+
Comment on attachment 8727567 [details]
MozReview Request: Bug 1254261 - Restore icons by using command button styles. r=gl,bgrins

Since this approach tweaks the colors slightly, let's check with Helen as well.
Attachment #8727567 - Flags: ui-review?(hholmes)
https://reviewboard.mozilla.org/r/38511/#review35663

Lots of cleanup can be done.

::: devtools/client/responsive.html/index.css:52
(Diff revision 2)
>    padding: 0;

You can remove the border: none; display: block; padding: 0; properties. They are already set in toolbars.css

::: devtools/client/responsive.html/index.css:55
(Diff revision 2)
>    opacity: 0.8;

The opacity: 0.8; rule can be removed. opacity: 0.8 is already set on ::before.

::: devtools/client/responsive.html/index.css:62
(Diff revision 2)
>    opacity: 1;

This can be removed too (the exact same style is set for ::before).

::: devtools/client/responsive.html/index.css:67
(Diff revision 2)
> +  filter: url("chrome://devtools/skin/images/filters.svg#checked-icon-state");

The opacity: 1 property can be removed. The same style is set on ::before in toolbars.css

Also, the filter should be set on .toolbar-button:active::before.
Thanks :ntim!  Will clean up after remaining reviews.
Comment on attachment 8727567 [details]
MozReview Request: Bug 1254261 - Restore icons by using command button styles. r=gl,bgrins

https://reviewboard.mozilla.org/r/38511/#review36257
Attachment #8727567 - Flags: review?(gl) → review+
Blocks: 1241714
Comment on attachment 8727567 [details]
MozReview Request: Bug 1254261 - Restore icons by using command button styles. r=gl,bgrins

One small thing!

I notice the 'x' button is looking really large in comparison to our rotate button; can we change the size of the 'x' to be smaller or the 'rotate' to be larger so they match in size? (I'd prefer a smaller x)
Attachment #8727567 - Flags: ui-review?(hholmes) → feedback+
Comment on attachment 8727567 [details]
MozReview Request: Bug 1254261 - Restore icons by using command button styles. r=gl,bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38511/diff/2-3/
Attachment #8727567 - Flags: feedback+
Comment on attachment 8727567 [details]
MozReview Request: Bug 1254261 - Restore icons by using command button styles. r=gl,bgrins

Made :ntim's cleanup changes.

I also made the close "x" smaller, down to 12 x 12.  Scaling it down in this way without changing the SVG does mean the lines are now slightly thinner.  Will this seem strange next to future icons in the global toolbar?  I suppose it depends which size is used for the future icons...
Attachment #8727567 - Flags: ui-review?(hholmes)
Attachment #8727567 - Flags: ui-review?(hholmes) → ui-review+
https://hg.mozilla.org/mozilla-central/rev/043af397fef3
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Both 'X' and RDM toolbar buttons are correctly displayed on Firefox 48.0a1 (2016-04-05).
The tests were performed on Windows 10 x86, Mac OS X 10.9.5 and on Ubuntu 12.04 x86.
I am marking this issue Verified-Fixed.
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.