Closed Bug 1346280 Opened 4 years ago Closed 4 years ago

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

Categories

(Toolkit :: Themes, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: sfoster, Assigned: sfoster)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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: 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
Depends on: 1287763
Component: Theme → Themes
Priority: -- → P3
Product: Firefox → Toolkit
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)
(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.
Attachment #8846852 - Attachment is obsolete: true
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 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+
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.
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
https://hg.mozilla.org/mozilla-central/rev/967240c44402
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Blocks: 1348350
You need to log in before you can comment on or make changes to this bug.