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

Status: RESOLVED FIXED

(Toolkit :: Themes, defect, P3)

Description

[Sam Foster [:sfoster] (he/him)]

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.

Comment 1

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1346280 - Use --focus-ring-box-shadow in place of @focusRingShadow@/@yosemiteRingFocusShadow@ in osx theme.

* replace use of @focusRingShadow@ with the CSS variable in toolkit osx theme and browser theme files
* remove redundant @media blocks as --focus-ring-box-shadow is already mac-theme specific

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

Comment 2

[:Gijs (he/him)]

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.

Comment 3

[:Gijs (he/him)]

Attachment: Screenshot of "Not Now" button in default browser prompt

Comment 4

[Sam Foster [:sfoster] (he/him)]

(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 5

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1346280 - Use --focus-ring-box-shadow in place of @focusRingShadow@/@yosemiteRingFocusShadow@ in osx theme.

* replace use of @focusRingShadow@ with the CSS variable in toolkit osx theme and browser theme files
* remove redundant @media blocks as --focus-ring-box-shadow is already mac-theme specific
* fix focus style for tabprompt buttons (alert buttons)
* fix focus style for notification anchor icons

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

Comment 6

[Sam Foster [:sfoster] (he/him)]

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

[:Gijs (he/him)]

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?

Comment 8

[Sam Foster [:sfoster] (he/him)]

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

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/967240c44402