1051861 - Gear icon is invisible in High Contrast themes

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect, P3)

Description

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Attachment: newtab_gear_icon_high_contrast.png

Reproducible with:
Latest Firefox 34 Nightly - BuildID: 20140811030203

Environment: Windows 7 x64, Windows 8 x64

Steps to reproduce:
1. Right click on desktop and select Personalize.
2. Select one of the High Contrast Themes (#1, #2, Black, White).
3. Open Firefox and open a New Tab.

Expected results: 
A gear (cog wheel) icon displays on top right of the page.

Actual results:
The gear (cog wheel) icon is invisible in High Contrast Themes.

Comment 1

[Mrinal Dhar]

Hi, I'd like to work on this bug. Could you please help me get started?

Comment 2

[fototakis]

hello I would like to help on this one. It seems this bug replicates easily and affects more parts of the web ui. Loading chrome://browser/skin/newtab/controls.svg in a new url seems to show all of the icons fine. It looks like the icon is covered by something else. Propably some other css property

Comment 3

[:Gijs (he/him)]

This was marked good first bug, but I don't know that we have assets to fix this already, and it looks like there are no instructions on how to do so either. Can one of you help?

Comment 4

[Drew Willcoxon :adw]

Sure, I can help.  I'm not sure right now what's required to fix this bug, but I'll look into it and post another comment.

Mrinal, have you set up a development environment?  Please see https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide for info.

fototakis, thanks for your interest but Mrinal got here first, so we should let Mrinal work on it first.

Comment 5

[Drew Willcoxon :adw]

This is the SVG file that contains the gear icons: http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/newtab/controls.svg?raw=1

You can see there are two gear icons, a gray one for non-hover:

  <g id="icon-gear-default">
    <use xlink:href="#glyphShape-gear"   class="glyphShape-style" />
  </g>

And a blue one for hover:

  <g id="icon-gear-default" transform="translate(32)">
    <use xlink:href="#glyphShape-gear"   class="glyphShape-style-hover-gear" />
  </g>

I think the problem is that both the glyphShape-style and glyphShape-style-hover-gear classes use hardcoded colors for `fill`.  In order to get the right high-contrast colors, they should be using system colors, like GrayText or -moz-DialogText for non-hover and HighlightText for hover.  I think.  I'll need to fire up Windows 7 and verify that's the right fix.  Hopefully I can do that by tomorrow.

Comment 6

[Mrinal Dhar]

I ran it on Windows 8.1, and even though the error was reproducible, the fix you suggested didn't seem to work. There was no trouble with the High Contrast theme #1 and #2, just with the black and white.

Comment 7

[Drew Willcoxon :adw]

It doesn't work for me either.  I'm not sure what the problem is.  I'll try to investigate this more soon, so I'll leave the needinfo as a reminder.

Comment 8

[Drew Willcoxon :adw]

Mrinal, if you'd like, you can join the #fx-team channel on IRC and ask for help.  #fx-team is where the front-end Firefox engineers hang out, and someone there may know better than me how to fix this.  (I'm adw there.)  The channel is most active around 5 am to 5 pm PT (UTC-8).  https://wiki.mozilla.org/IRC

I'll continue to investigate it, though.

Comment 9

[Drew Willcoxon :adw]

OK, I have something that mostly works, but it's ugly.  We need to change not only the SVG but also the CSS that uses it.  Mrinal, this is more work than we originally thought.  Let me know if you have questions.

Apparently, at least in this case, an SVG as a background-image in high-contrast mode just isn't drawn or something.  It's invisible.  The only way I found that works is to force the element that uses the SVG to be `display: list-item` and then use list-style-image.  This case is complicated because it's XHTML.  If newtab were XUL it'd easier because we could use list-style-image without forcing `display: list-item`, at least for certain XUL elements.  We could use a XUL element just for the gear, but we want to make newtab unprivileged in the future, so we shouldn't add any more XUL to it.

You also can't use -moz-image-rect to specify a region of SVG, even when the SVG uses system colors and you use list-style-image.  I'm guessing that's because the SVG is rasterized before the high-contrast system color can be applied, but I don't know.

So we need to do this:

(1) Change controls.svg to use system colors for the gear and hover-gear, but only in Windows non-default themes.  Otherwise use the hard-coded colors like we are now.  You can use %ifdef + media queries to detect Windows non-default themes, for example:

/* Default to using the nice, hard-coded color. */
.glyphShape-style {
  fill: #737373;
}
%ifdef XP_WIN
@media not all and (-moz-windows-default-theme) {
  /* On Windows non-default themes, use a system color. */
  .glyphShape-style {
    fill: GrayText;
  }
}
%endif

Let's use GrayText for the gear and Highlight for the hover-gear.

Then we'll need to add an asterisk (*) next to controls.svg in the three jar.mn files to tell the preprocessor to preprocess the file:

http://mxr.mozilla.org/mozilla-central/search?string=controls.svg&find=jar.mn&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

For examples, you can see other lines that start with asterisks in those files.

(2) Change controls.svg so that individual glyphs can be referenced by anchor fragments, e.g., chrome://browser/skin/newtab/controls.svg#icon-gear-default.

* On <svg>, change width, height, and viewBox's width and height to be 32, which is the natural size of the glyphs.

* In <style>, add:

g:not(:target) {
  display: none;
}

* Remove the transform from each <g> and make sure each has a unique ID.

(3) In newTab.inc.css, change #newtab-customize-button as I mentioned above, to use `display` and list-style-image.

(4) Unfortunately, step (3) offsets the glyph to the left (in LTR languages), so also add `list-style-position: inside`.  After that the glyph is still slightly offset except this time to the right, so to prevent it from being cut off, make #newtab-customize-button 32px by 32px.  Then make its -moz-margin-start something like -10px to move it back to the left.

(5) Unfortunately, step (2) means we have to change all the other CSS in newTab.inc.css that references controls.svg to use ID fragments instead of -moz-image-rect.  You may have to play around with backgrounds and sizes to make things look like they do now.

Let's not worry about making all other glyphs appear correct in high contrast mode.  We can do that in other bugs, and we should probably first file a bug about background-image SVGs not being correctly drawn in high-contrast mode.

Comment 10

[Mrinal Dhar]

Attachment: First patch for the bug

Hi, Drew. 

I made the changes as you directed, and I reached point (4).

When I build and run it now, I don't see the icon there. If I try to open chrome://browser/skin/newtab/controls.svg directly, it gives me a parse error. So I checked in the controls.svg source, and there were two lines automatically added at the end that generated a syntax error. I've marked those lines in the patch (lines 135 & 136 in the controls.svg file). 
Even if I remove those lines, they're added after each build.

Also, running a build gives me a warning about the controls.svg file: "No preprocessor directives found."

Comment 11

[Drew Willcoxon :adw]

Comment on attachment 8561857 [details] [diff] [review]
First patch for the bug

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

Hi Mrinal, thanks for the patch.  I'm not sure where the extra lines in controls.svg are coming from.  Maybe the preprocessor, but if so, it must be a bug.  Try making the changes that I talk about below.  Hopefully they'll fix that problem, too.  Let me know how it goes.

::: browser/themes/shared/newtab/controls.svg
@@ +17,5 @@
>        .glyphShape-style {
>          fill: #737373;
>        }
>  
> +      %ifdef XP_WIN

My mistake, you need to use the # character instead of %.  And those lines must start with #.  No spaces at the beginning.  Like this:

#ifdef XP_WIN
    @media ...
#endif

@@ +25,5 @@
> +          fill: GrayText;
> +        }
> +
> +        .glyphShape-style-hover-gear {
> +          fill: HighlightText;

This rule needs to come after the "default" .glyphShape-style-hover-gear rule below.  Otherwise the default rule will take precedence over this one in all cases.  So just as you have the default rule for .glyphShape-style followed by the XP_WIN case, you need to have the .glyphShape-style-hover-gear default followed by the XP_WIN case.

@@ +62,5 @@
>          fill-opacity: .5;
>          filter: url(#filter-shadow-drop);
>        }
> +
> +      g:not(:target) {

Please put this at the very top of the CSS section, above the /* Glyph Styles */ comment.

::: browser/themes/windows/jar.mn
@@ +159,5 @@
>          skin/classic/browser/feeds/videoFeedIcon16.png               (feeds/feedIcon16.png)
>          skin/classic/browser/feeds/subscribe.css                     (feeds/subscribe.css)
>          skin/classic/browser/feeds/subscribe-ui.css                  (feeds/subscribe-ui.css)
>  *       skin/classic/browser/newtab/newTab.css                       (newtab/newTab.css)
> +*       skin/classic/browser/newtab/controls.svg                     (../shared/newtab/controls.svg)

There are two lines for most files in the Windows jar.mn.  This one here, and then there is another one below for "aero":

>  skin/classic/aero/browser/newtab/controls.svg                (../shared/newtab/controls.svg)

You have to update both lines.  The "aero" one is used on some versions of Windows, I'm not sure which ones.  For me, I'm on Windows 7, and I had to update the aero line to get my controls.svg changes to show up.

Sorry for not mentioning that before.

Comment 12

[Mrinal Dhar]

I added the aero line for preprocessing, like you said. 

Earlier, there was no icon being displayed. When I used to hover over where it should have been, a black 'dot' appeared where the icon should have been. This was when the controls.svg had the syntax error problems. (Due to automatically added lines.)

Now, there is still no icon there on the new tab page. But, when I hover, a horizontal scrollbar appears at the bottom. This is only when I hover over that area. 

Also, I fixed the automatically added lines. There's another warning during the build process, and this one is for themes/windows/content-contextmenu.svg (No preprocessor directives found.)

Comment 13

[Drew Willcoxon :adw]

Did you do all the steps in my comment above?  I think you need to do all of them to make anything show up at all.  I'm talking particularly about the ID fragments part.

Comment 14

[Mrinal Dhar]

Attachment: Gear icon shows up in High Contrast

Hi, Drew. 
I was missing the ID fragments part. It works now, tested it with all 4 high contrast Windows themes. 

I was going to change the other elements to list-style-image as well, but where exactly are the pin and delete glyphIcons used in the css?

Comment 15

[Drew Willcoxon :adw]

Nice, I'll take a look at the patch.

It's going to be a pain to change the other elements to use list-style-image too.  I don't think we should do that.  I'm actually not so sure we even should be changing the gear icon to use list-style-image, but I think that's OK to do to fix this one bug.  Instead, we should find out why SVGs in background-image seem to be invisible in high-contrast mode even when they use system colors.  (I'm not including you in that "we" -- unless you're interested in finding out why, and then by all means go for it!)  We shouldn't have to use list-style-image at all.  But finding out why requires someone with knowledge of SVGs and CSS in Gecko and whatever core code is involved.

Comment 16

[Drew Willcoxon :adw]

Comment on attachment 8563320 [details] [diff] [review]
Gear icon shows up in High Contrast

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

Thanks, this looks good, but in addition to my comments below, do the other icons besides the gear still show up as expected?  I haven't tested your patch but I wouldn't think they would, because the CSS that references them currently doesn't do it by fragment ID.  It uses -moz-image-rect with offsets that don't make sense anymore now that the SVG size is completely different and the glyphs are "on top" of each other.  In other words, you didn't complete step 5 above.

For example, I think you need to change this line here: http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/newtab/newTab.inc.css#153

To:

background-image: url("chrome://browser/skin/newtab/controls.svg#icon-pinned-default");

Please make sure all the icons still work (in the normal color mode, not high-contrast mode).

::: browser/themes/shared/newtab/controls.svg
@@ +125,5 @@
>      <use xlink:href="#glyphShape-circle" class="glyphShape-style-circle" />
>      <use xlink:href="#glyphShape-delete" class="glyphShape-style-hover-active" />
>    </g>
>  
> +  <g id="icon-pin-default">

This is the same ID as the first pin glyph above.  It needs to have a unique ID.  I suggest icon-pinned-default because this is the icon that's used to indicate that a tile has been pinned.  (The glyph above with the same ID is the one in the top corner of tiles that you click to pin them.)

::: browser/themes/shared/newtab/newTab.inc.css
@@ +74,5 @@
>  }
>  
>  #newtab-customize-button:-moz-any(:hover, :active, [active]) {
> +  display: list-item;
> +  list-style-image: url(chrome://browser/skin/newtab/controls.svg#icon-gear-hover);

I think only the list-style-image is necessary here.  All the other rules should cascade from the earlier #newtab-customize-button rule right above.  Could you try removing the others and seeing if that works?

Comment 18

[Dão Gottwald [:dao]]

Attachment: patch

Same approach as in bug 1022588

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6e3c770c9e9
Use Unicode gear symbol as the customize button's label for high contrast mode. r=adw

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e6e3c770c9e9

Comment 21

[Md. Majedul islam]

I have reproduced this bug with Firefox nightly 34.0a1 (2014-08-11) on Windows 10, 64 Bit.

The Bug's fix is now verified on latest aurora 51.0a2 (2016-09-21) 

Build ID 	20160921004005

User Agent 	 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
[bugday-20160921]

Comment 22

[:Gijs (he/him)]

Thanks.