Closed Bug 1022600 Opened 6 years ago Closed 5 years ago

Icon for utilities button in about:addons isn't legible in High Contrast mode

Categories

(Toolkit :: Add-ons Manager, defect)

x86_64
Windows 8.1
defect
Not set
Points:
2

Tracking

()

VERIFIED FIXED
mozilla38
Iteration:
38.2 - 9 Feb
Tracking Status
firefox38 --- verified
firefox39 --- verified

People

(Reporter: Unfocused, Assigned: Paenglab)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

Attached image screenshot
The icon used for utilities button in about:addons isn't legible in High Contrast mode, as it lacks adequate contrast. See attached screenshot.
Flags: firefox-backlog+
Attached image devtools-cog.png (obsolete) —
Devtools cog used in the dark theme.
Points: --- → 2
Whiteboard: p=2
Attached image options.svg
For anyone who wants to work on this bug.
Note that the asset is already high-contrast ready.
Attachment #8535038 - Attachment is obsolete: true
Attached patch Bug1022600.patch (obsolete) — Splinter Review
This patch exchanges the utilities.png with the utilities.svg on OS X and Windows.

On OS X I have chosen fill="#4d4d4d" like the Yosemite toolbar icons have. I leaved the fill="graytext" for Windows from ntim's attachment. This gives the icon on HC-themes a different color than the text (for example on one it's green instead of yellow). If we would use -moz-DialogText, the color would be the same but on default theme it would be black, and this is to dark.

If you want, I could also add two colors for default and non-default themes.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8554168 - Flags: review?(jaws)
Comment on attachment 8554168 [details] [diff] [review]
Bug1022600.patch

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

I think the utilities.svg file can be put in shared. You can use utilities and utilities-native as IDs for the new common file.
Attachment #8554168 - Flags: review?(jaws) → review?(MattN+bmo)
Comment on attachment 8554168 [details] [diff] [review]
Bug1022600.patch

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

(In reply to Tim Nguyen [:ntim] from comment #5)
> I think the utilities.svg file can be put in shared.

I would support putting the file in shared but…

> You can use utilities and utilities-native as IDs for the new common file.

I don't think "utilities-native" is descriptive enough. We could also just have one shape in utilities.svg and pre-process a CSS rule to change the fill property so we don't have to duplicate the shape. If we had a CSS media query to match the OS X versions you care about then we could use that instead of pre-processing.

I actually think the patch is okay without using shared though since we're not duplicating the shape a third time for Linux.

(In reply to Richard Marti (:Paenglab) from comment #4)
> On OS X I have chosen fill="#4d4d4d" like the Yosemite toolbar icons have.

Is this colour used in mozilla-central already? A search for "4d4d4d" doesn't show it. I mostly want to make sure colours are consistent and it's not clear to me where this is coming from. Are the other colours written in hsl/rgb() syntax? If so, use the same syntax in case people want to replace that colour.
Attachment #8554168 - Flags: review?(MattN+bmo) → feedback+
Attached patch Bug1022600.patch (obsolete) — Splinter Review
I'm using now the color #424f5a from Project Chameleon for OS X and Windows default themes (to be ready for bug 989469). For the Windows non-default themes it's still GrayText. To switch I'm using utilities and utilities-native as IDs like we are already using for the in-contnet checkbox and radio SVGs to be consistent.
Attachment #8554168 - Attachment is obsolete: true
Attachment #8556691 - Flags: review?(MattN+bmo)
Comment on attachment 8556691 [details] [diff] [review]
Bug1022600.patch

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

Ah, I don't mind the "-native" suffix now that I realize the suffix is put on the one using system colors instead of the one that I thought was getting hard-coded with a supposedly native OS X color.

Awesome, thanks!
Attachment #8556691 - Flags: review?(MattN+bmo) → review+
Keywords: checkin-needed
Comment on attachment 8556691 [details] [diff] [review]
Bug1022600.patch

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

No prettiness for linux ?

::: toolkit/themes/osx/mozapps/extensions/extensions.css
@@ +296,5 @@
>    padding-left: 3px;
>  }
>  
>  #header-utils-btn {
> +  list-style-image: url("chrome://mozapps/skin/extensions/utilities.svg");

Shouldn't this be utilities.svg#utilities ?
Attached patch Bug1022600.patchSplinter Review
(In reply to Tim Nguyen [:ntim] from comment #9)
> Comment on attachment 8556691 [details] [diff] [review]
> Bug1022600.patch
> 
> Review of attachment 8556691 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> No prettiness for linux ?

The other buttons on Linux have GTK icons, so I decided to stay consistent. With bug 989469 I can use this icon and remove the icons from the other buttons under Linux.

> ::: toolkit/themes/osx/mozapps/extensions/extensions.css
> @@ +296,5 @@
> >    padding-left: 3px;
> >  }
> >  
> >  #header-utils-btn {
> > +  list-style-image: url("chrome://mozapps/skin/extensions/utilities.svg");
> 
> Shouldn't this be utilities.svg#utilities ?

Correct.
Attachment #8556691 - Attachment is obsolete: true
Attachment #8557463 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b0308591015d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla38
Flags: qe-verify+
Iteration: --- → 38.2 - 9 Feb
QA Contact: andrei.vaida
Verified fixed on Nightly 39.0a1 (2015-03-09) and Aurora 38.0a2 (2015-03-09), using Windows 7 (x64) and Windows 8.1 (x64).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.