Bug 241228

Adding Alexm in CC, This issue seems related with focusRingColorLight in ThemeAdwaita being quite dark currently: SRGBA<uint8_t> { 46, 52, 54, 150 } and due to this CSS snippet in the controls (thanks Rego for finding out), the buttons are rendered very dark, and if the video is also kind of dark like in this screencast, the user sees almost nothing rendered in the button placeholder. button:focus > picture { background-color: -webkit-focus-ring-color !important; mix-blend-mode: normal !important; } So my question is, should we change this CSS for WPE/GTK or is this a bug in ThemeAdwaita?

So, the focus ring is supposed to be the text color, though looks like I forgot to update it to be a neutral grey. It being text color means that it should be white for media controls. In both GTK4 and Default we switched to thicker blue frames for focus, but it didn't work great with focusing multiline links, so I didn't implement that when updating webkit widget style. Here focus ring color is indeed wrong, we should be using accent color instead. That said, I'm not sure we expose it in user agent CSS? I know Apple ports do that though.

Alternatively we can do the same outline as elsewhere, it doesn't necessarily have to be icon color.

IIUC in CSS -webkit-focus-color-ring ends up in this platform code: https://webkit-search.igalia.com/webkit/source/Source/WebCore/rendering/RenderThemeAdwaita.cpp#189 which AFAICS is aware of of system dark mode, but I suspect by "accent color" you mean something else?

The same CSS is correctly rendered on macOS BTW, so I think it's clearly an issue with our theme code. If possible I'd rather not diverge from the CSS already defined in button.css, which is shared by all modern media controls flavours :)

The problem is that macOS has accent-colored focus rings and we don't. Accent color is this one: https://webkit-search.igalia.com/webkit/source/Source/WebCore/rendering/RenderThemeAdwaita.cpp#103 In our case it's usually blue. That's exactly what I mentioned above wrt trying to implement it before, not sure if I still have those screenshots.

Created attachment 460021 [details] Patch

Ok, so the patch changes focus rings to be more like in macOS. I can't check if it fixes the issue as I don't have modern media controls locally, and the old ones weren't affected - though focus is actually visible on those now. So testing with modern controls would be appreciated. I also expect this to break a ton of tests just like bug 235884, but we'll see what EWS says.

Egh ok, my checkout is too old to apply it then. :(

(In reply to Alexander Mikhaylenko from comment #9) > Egh ok, my checkout is too old to apply it then. :( Only the ChangeLog doesn't apply, we don't use those anymore. I'll test your patch here, thanks for providing it!

After locally fixing this error: /app/webkit/Source/WebCore/rendering/RenderThemeAdwaita.cpp:284:47: error: non-constant-expression cannot be narrowed from type 'char' to 'unsigned char' in initializer list [-Wc++11-narrowing] return SRGBA<uint8_t> { 52, 132, 228, (char) (255 * focusRingOpacity) }; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /app/webkit/Source/WebCore/rendering/RenderThemeAdwaita.cpp:284:47: note: insert an explicit cast to silence this issue return SRGBA<uint8_t> { 52, 132, 228, (char) (255 * focusRingOpacity) }; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ static_cast<unsigned char>( ) 1 error generated. I can see focused button is now blue, attaching screenshot :)

Created attachment 460025 [details] screenshot

I suppose this patch is OK, but I'm now leaning towards disabling the focus ring background in the controls, because it's a bit awkward to have buttons turn blue when the user clicks on them. There's already visual feedback (hopefully), eg the playback stopping when play is pressed and so on.

(In reply to Philippe Normand from comment #13) > I suppose this patch is OK, but I'm now leaning towards disabling the focus > ring background in the controls, because it's a bit awkward to have buttons > turn blue when the user clicks on them. There's already visual feedback > (hopefully), eg the playback stopping when play is pressed and so on. https://bugs.webkit.org/show_bug.cgi?id=241468

As commented on github, disabling focus would break the case where you are, well, focusing the widget. Please don't do that.

Pull request: https://github.com/WebKit/WebKit/pull/1446

Second attempt: https://github.com/WebKit/WebKit/pull/2450

Committed 252596@main (c63818df574b): <https://commits.webkit.org/252596@main> Reviewed commits have been landed. Closing PR #2450 and removing active labels.