Bug 241228

Summary: [GTK][WPE] Revamp focus rings
Product: WebKit Reporter: Philippe Normand <pnormand>
Component: WebKitGTKAssignee: Philippe Normand <philn>
Status: RESOLVED FIXED    
Severity: Normal CC: alicem, bugs-noreply
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
screencast
none
Patch
alicem: review?
screenshot none

Philippe Normand
Reported 2022-06-02 09:52:29 PDT
Created attachment 459966 [details] screencast see screencast
Attachments
screencast (397.31 KB, video/webm)
2022-06-02 09:52 PDT, Philippe Normand
no flags
Patch (12.82 KB, patch)
2022-06-03 14:34 PDT, Alice Mikhaylenko
alicem: review?
screenshot (65.22 KB, image/png)
2022-06-04 01:40 PDT, Philippe Normand
no flags
Philippe Normand
Comment 1 2022-06-03 08:22:00 PDT
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?
Alice Mikhaylenko
Comment 2 2022-06-03 11:10:19 PDT
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.
Alice Mikhaylenko
Comment 3 2022-06-03 11:10:48 PDT
Alternatively we can do the same outline as elsewhere, it doesn't necessarily have to be icon color.
Philippe Normand
Comment 4 2022-06-03 11:18:05 PDT
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?
Philippe Normand
Comment 5 2022-06-03 11:21:43 PDT
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 :)
Alice Mikhaylenko
Comment 6 2022-06-03 11:23:16 PDT
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.
Alice Mikhaylenko
Comment 7 2022-06-03 14:34:49 PDT
Alice Mikhaylenko
Comment 8 2022-06-03 14:36:51 PDT
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.
Alice Mikhaylenko
Comment 9 2022-06-03 14:38:10 PDT
Egh ok, my checkout is too old to apply it then. :(
Philippe Normand
Comment 10 2022-06-04 01:22:56 PDT
(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!
Philippe Normand
Comment 11 2022-06-04 01:39:49 PDT
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 :)
Philippe Normand
Comment 12 2022-06-04 01:40:10 PDT
Created attachment 460025 [details] screenshot
Philippe Normand
Comment 13 2022-06-08 10:00:45 PDT
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.
Philippe Normand
Comment 14 2022-06-09 08:08:18 PDT
(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
Alice Mikhaylenko
Comment 15 2022-06-10 03:47:15 PDT
As commented on github, disabling focus would break the case where you are, well, focusing the widget. Please don't do that.
Philippe Normand
Comment 16 2022-06-10 07:25:56 PDT
Alice Mikhaylenko
Comment 17 2022-07-15 09:14:39 PDT
EWS
Comment 18 2022-07-19 06:35:02 PDT
Committed 252596@main (c63818df574b): <https://commits.webkit.org/252596@main> Reviewed commits have been landed. Closing PR #2450 and removing active labels.
Note You need to log in before you can comment on or make changes to this bug.