Bug 241228 - [GTK][WPE] Revamp focus rings
Summary: [GTK][WPE] Revamp focus rings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-06-02 09:52 PDT by Philippe Normand
Modified: 2022-07-19 06:35 PDT (History)
2 users (show)

See Also:


Attachments
screencast (397.31 KB, video/webm)
2022-06-02 09:52 PDT, Philippe Normand
no flags Details
Patch (12.82 KB, patch)
2022-06-03 14:34 PDT, Alexander Mikhaylenko
alexm: review?
Details | Formatted Diff | Diff
screenshot (65.22 KB, image/png)
2022-06-04 01:40 PDT, Philippe Normand
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2022-06-02 09:52:29 PDT
Created attachment 459966 [details]
screencast

see screencast
Comment 1 Philippe Normand 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?
Comment 2 Alexander Mikhaylenko 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.
Comment 3 Alexander Mikhaylenko 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.
Comment 4 Philippe Normand 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?
Comment 5 Philippe Normand 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 :)
Comment 6 Alexander Mikhaylenko 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.
Comment 7 Alexander Mikhaylenko 2022-06-03 14:34:49 PDT
Created attachment 460021 [details]
Patch
Comment 8 Alexander Mikhaylenko 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.
Comment 9 Alexander Mikhaylenko 2022-06-03 14:38:10 PDT
Egh ok, my checkout is too old to apply it then. :(
Comment 10 Philippe Normand 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!
Comment 11 Philippe Normand 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 :)
Comment 12 Philippe Normand 2022-06-04 01:40:10 PDT
Created attachment 460025 [details]
screenshot
Comment 13 Philippe Normand 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.
Comment 14 Philippe Normand 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
Comment 15 Alexander Mikhaylenko 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.
Comment 16 Philippe Normand 2022-06-10 07:25:56 PDT
Pull request: https://github.com/WebKit/WebKit/pull/1446
Comment 17 Alexander Mikhaylenko 2022-07-15 09:14:39 PDT
Second attempt: https://github.com/WebKit/WebKit/pull/2450
Comment 18 EWS 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.