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

RESOLVED FIXED in Firefox 53

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: scottwu, Assigned: scottwu)

Tracking

(Blocks 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

3 years ago
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.
Component: XUL Widgets → Themes
Priority: -- → P3
Are you going to work on this? Might also be a good first bug if you provide instructions on how to fix this.
Flags: needinfo?(scwwu)
Comment hidden (mozreview-request)
Assignee

Comment 3

3 years ago
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!
Assignee: nobody → scwwu
Flags: needinfo?(scwwu) → needinfo?(dao+bmo)
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?
Flags: needinfo?(dao+bmo)
Assignee

Comment 5

3 years ago
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?
Flags: needinfo?(dao+bmo)
(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
Flags: needinfo?(dao+bmo)
Comment hidden (mozreview-request)
Comment on attachment 8816357 [details]
Bug 1317600 - Spinner arrow buttons no longer depend on find bar resources;

Nice!
Attachment #8816357 - Flags: review?(dao+bmo) → review+
Assignee

Comment 9

3 years ago
Thank you Dão!
Keywords: checkin-needed

Comment 10

3 years ago
Pushed by dgottwald@mozilla.com:
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
Keywords: checkin-needed

Comment 11

3 years ago
Pushed by archaeopteryx@coole-files.de:
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
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

3 years ago
Backout by archaeopteryx@coole-files.de:
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/37fb958b2f9b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.