1317600 - [DateTimePicker] Spinner arrow buttons shouldn't depend on find bar resources

Status: RESOLVED FIXED

(Toolkit :: Themes, defect, P3)

Description

[Scott Wu [:scottwu]]

As pointed by :dao in bug 1301284:

> ::: toolkit/themes/shared/timepicker.css:63

> > +.spinner-container button.up {
> > +  mask: url("chrome://global/skin/icons/find-arrows.svg#glyph-find-previous") no-repeat 50% 50%;
> > +}
> > +
> > +.spinner-container button.down {
> > +  mask: url("chrome://global/skin/icons/find-arrows.svg#glyph-find-next") no-repeat 50% 50%;
> > +}

The svg is for the find bar and shouldn't be used directly here.

Comment 1

[Dão Gottwald [:dao]]

Are you going to work on this? Might also be a good first bug if you provide instructions on how to fix this.

Comment 2

[Scott Wu [:scottwu]]

Attachment: Bug 1317600 - Spinner arrow buttons no longer depend on find bar resources;

Review commit: https://reviewboard.mozilla.org/r/97120/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97120/

Comment 3

[Scott Wu [:scottwu]]

I've actually got the fix when working on a different bug, but haven't got the chance to put it here. Sorry about the delay!

I created a new svg based on the resources from the visual spec. Wonder if you could take a look? Thanks!

Comment 4

[Dão Gottwald [:dao]]

Why are you using mask here (rather than list-style-image or background-image), and if there's a good reason for using mask, why are you specifying a fill color? Isn't that color irrelevant when using mask?

Comment 5

[Scott Wu [:scottwu]]

Yes you are right that the fill color is irrelevant in this case and I'll take it out.

Initially I tried to use background-image but I couldn't find a way to change the fill (when :hover or active), so I used mask to achieve that. Do you think there's a better approach?

Comment 6

[Dão Gottwald [:dao]]

(In reply to Scott Wu [:scottwu] from comment #5)
> Initially I tried to use background-image but I couldn't find a way to
> change the fill (when :hover or active), so I used mask to achieve that. Do
> you think there's a better approach?

We use chrome://global/skin/filters.svg#fill for that.

Here's an example: https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/downloads/downloads.inc.css#138

Comment 7

[Scott Wu [:scottwu]]

Comment on attachment 8816357 [details]
Bug 1317600 - Spinner arrow buttons no longer depend on find bar resources;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97120/diff/1-2/

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8816357 [details]
Bug 1317600 - Spinner arrow buttons no longer depend on find bar resources;

Nice!

Comment 9

[Scott Wu [:scottwu]]

Thank you Dão!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/53cf106dd90c
Make date / time picker spinner arrow buttons no longer depend on find bar resources; r=dao

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37fb958b2f9b
Make date / time picker spinner arrow buttons no longer depend on find bar resources; r=dao

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Please ignore the backout which will be posted here in a few minutes, this was done by mistake and the patch landed again, see comment 11. Thank you.

Comment 13

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ace52a373931
Backed out changeset 53cf106dd90c on suspicion of causing sanitization tests like browser_sanitize-timespans.js to fail on OS X. r=backout

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/37fb958b2f9b