Replace yosemiteFocusRingShadow macro with --focus-ring-box-shadow variable in OSX theme

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Themes
P3
normal
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: sfoster, Assigned: sfoster)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

9 months ago
Bug 1287763 introduced a CSS variable --focus-ring-box-shadow with the flat/yosemite-style treatment defined once at the top of the osx theme. This bug is to fix the remaining instances of @yosemiteFocusRingShadow@ in the findbar and toolkit to use the same variable.
(Assignee)

Updated

9 months ago
Assignee: nobody → sfoster
Summary: Use replace yosemiteFocusRingShadow macro with --focus-ring-box-shadow variable in OSX theme → Replace yosemiteFocusRingShadow macro with --focus-ring-box-shadow variable in OSX theme
(Assignee)

Updated

9 months ago
Depends on: 1287763
Comment hidden (mozreview-request)

Updated

9 months ago
Component: Theme → Themes
Priority: -- → P3
Product: Firefox → Toolkit

Comment 2

9 months ago
mozreview-review
Comment on attachment 8846852 [details]
Bug 1346280 - Use --focus-ring-box-shadow in place of @focusRingShadow@/@yosemiteRingFocusShadow@ in osx theme.

https://reviewboard.mozilla.org/r/119848/#review122184

Code-wise, this looks OK, but buttons in alerts still don't look right to me for some reason... I'll put up a screenshot in a second.
Attachment #8846852 - Flags: review?(gijskruitbosch+bugs)

Comment 3

9 months ago
Created attachment 8847268 [details]
Screenshot of "Not Now" button in default browser prompt
(Assignee)

Comment 4

9 months ago
(In reply to :Gijs from comment #2)
> Comment on attachment 8846852 [details]
> Bug 1346280 - Use --focus-ring-box-shadow in place of
> @focusRingShadow@/@yosemiteRingFocusShadow@ in osx theme.
> 
> https://reviewboard.mozilla.org/r/119848/#review122184
> 
> Code-wise, this looks OK, but buttons in alerts still don't look right to me
> for some reason... I'll put up a screenshot in a second.

I noticed the same. I think this is one of those cases that was missed first time around? Or I flubbed something. Either way get it fixed in an update to this patch.
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8846852 - Attachment is obsolete: true
(Assignee)

Comment 6

9 months ago
mozreview-review
Comment on attachment 8847772 [details]
Bug 1346280 - Use --focus-ring-box-shadow in place of @focusRingShadow@/@yosemiteRingFocusShadow@ in osx theme.

https://reviewboard.mozilla.org/r/120694/#review122648

I'm not sure why this isn't updating the other review... But this patch makes the same changes to consolidate use of the --focus-ring-box-shadow and also picks up a couple areas we missed first time around - the notification anchor icons, and the prompt/alert buttons.

Comment 7

9 months ago
mozreview-review
Comment on attachment 8847772 [details]
Bug 1346280 - Use --focus-ring-box-shadow in place of @focusRingShadow@/@yosemiteRingFocusShadow@ in osx theme.

https://reviewboard.mozilla.org/r/120694/#review122946

These changes look good to me from code inspection, and I can confirm the prompt buttons are fixed.

Unfortunately, it does seem like there might still be more things we could update - see https://dxr.mozilla.org/mozilla-central/search?q=moz-mac-focusring&redirect=true . These aren't using the preprocessor replacements, but it does look like some might still be using outdated styling, if that makes sense. Another followup to look at those, I guess?
Attachment #8847772 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 8

9 months ago
mozreview-review-reply
Comment on attachment 8847772 [details]
Bug 1346280 - Use --focus-ring-box-shadow in place of @focusRingShadow@/@yosemiteRingFocusShadow@ in osx theme.

https://reviewboard.mozilla.org/r/120694/#review122946

Yeah I know these are bite-sized changesets, but I think I'll file one or more bugs for the remaining issues - I had done the same search and was going to follow-up.

Comment 9

9 months ago
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/967240c44402
Use --focus-ring-box-shadow in place of @focusRingShadow@/@yosemiteRingFocusShadow@ in osx theme. r=Gijs

Comment 10

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/967240c44402
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Updated

9 months ago
Blocks: 1348350
You need to log in before you can comment on or make changes to this bug.