1642343 - outline-style auto on mac draws white row of pixels.

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect, P1)

Description

[Dirkjan Ochtman (:djc)]

Attachment: Nightly 2020-06-01.png

As I understand it, Gecko is in the process of showing focus rings in more places. However, I noticed at least one example where the focus ring that recently started getting shown on crates.io (around the search input box) looks quite ugly, which makes even drawing the focus ring look like a regression compared to not doing so.

In comparison, the Safari focus ring is more subdued and follows the border-radius.

Comment 1

[Dirkjan Ochtman (:djc)]

Attachment: Safari.png

Comment 2

[Emilio Cobos Álvarez (:emilio)]

So following border-radius is basically bug 315209. Interestingly Safari only does this for outline: auto, not for outline: 1px solid for example.

However there's another issue here. That white inner border is really ugly, and that seems like an issue in the Mac implementation of outline: auto.

Instead of duping this and filing a new bug for the snapping issue, let's just morph this into a bug for that Cocoa issue.

Another potential thing to do to mitigate this is to mace the focus model on mac match other desktop platforms. That should also be an easy change if needed, but that probably needs its own pref.

Comment 3

[Markus Stange [:mstange]]

These diff hunks probably fix the issue: https://gist.github.com/mstange/9e7b750b175d2703d14dc39500318dac#file-2-make_ns_theme_focus__for_outline_style_auto__look_good_on_macos_-diff-L77-L284

(The "BezierPathWithRoundedRect" function seems unnecessary because [NSBezierPath bezierPathWithRoundedRect:xRadius:yRadius] exists and is 10.5+ so it's supported everywhere we ship.)

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1642343 - Fix Mac themed focus outline to not show white edges. r=mstange

This can be reproduced trivially on a MacBook on the test page with
layout.css.devPixelsPerPx = 1.

I wanted to fix this independently of following the outline radius.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1642343 - Attempt at making Mac outline follow border radius.

This was my attempt to rebase Markus' patch. Not clear to me we can get
rid of the extra function because the one mentioned in the last comment
only allows specifying two lengths, not all the eight that CSS allows
you to.

I have no intention to get this one finished at least just yet, because
I ran out of time today and my cocoa-foo is not great.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Can you confirm that the build here fixes the issue for you? I had to tweak DPI settings to repro so I'd rather get your confirmation just in case.

It should appear in about an hour or so from now (probably earlier). There should be a target.dmg in the "Job details" page of the "B" job, but let me know if you don't find it (whenever it's up) and I can try to help.

Thank you!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d92961ab6c4
Fix Mac themed focus outline to not show white edges. r=mstange

Comment 8

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/7d92961ab6c4

Comment 9

[Dirkjan Ochtman (:djc)]

Yeah, the white row of pixels is gone with this build. (Otherwise still looks pretty bad, though.)

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9153639 [details]
Bug 1642343 - Fix Mac themed focus outline to not show white edges. r=mstange

Beta/Release Uplift Approval Request

• User impact if declined: White edges in some outlines on Mac. We've started to show auto-style outlines more often, and it'd be sad to do that without fixing this.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: comment 0
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple change to how we paint outlines on Mac
• String changes made/needed: none

Comment 11

[Emilio Cobos Álvarez (:emilio)]

(In reply to Dirkjan Ochtman (:djc) from comment #9)

Yeah, the white row of pixels is gone with this build. (Otherwise still looks pretty bad, though.)

Ok, fair enough. I'll file another one to try to land the other patch to follow the border radius. Though my familiarity with the cocoa APIs to do that is probably not that much compared to Markus' :)

Comment 12

[Julien Cristau [:jcristau]]

Comment on attachment 9153639 [details]
Bug 1642343 - Fix Mac themed focus outline to not show white edges. r=mstange

approved for 78.0b3

Comment 13

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c4c1941f5a8d

Comment 14

[Bogdan Maris, Desktop QA]

Reproduced the initial issue using an old Nightly from 2020-06-01, verified that the issue is fixed using the latest Nightly 79.0a1 and Firefox 78.0b4 on macOS 10.15.6 beta.