WebKit Bugzilla
New
Browse
Search+
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
241228
[GTK][WPE] Revamp focus rings
https://bugs.webkit.org/show_bug.cgi?id=241228
Summary
[GTK][WPE] Revamp focus rings
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
Details
Patch
(12.82 KB, patch)
2022-06-03 14:34 PDT
,
Alice Mikhaylenko
alicem
: review?
Details
Formatted Diff
Diff
screenshot
(65.22 KB, image/png)
2022-06-04 01:40 PDT
,
Philippe Normand
no flags
Details
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 460021
[details]
Patch
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
Pull request:
https://github.com/WebKit/WebKit/pull/1446
Alice Mikhaylenko
Comment 17
2022-07-15 09:14:39 PDT
Second attempt:
https://github.com/WebKit/WebKit/pull/2450
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.
Top of Page
Format For Printing
XML
Clone This Bug