The specs seem clear about the expected behavior for this: - From https://drafts.csswg.org/selectors-4/#the-focus-pseudo: "The :focus pseudo-class applies while an element has the focus" - From https://w3c.github.io/uievents/#event-type-focus: "The focus MUST be given to the element before the dispatch of this event type" Also Chromium and Firefox already does this.
Created attachment 416944 [details] Patch
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
(In reply to EWS Watchlist from comment #2) > This patch modifies the imported WPT tests. Please ensure that any changes > on the tests (not coming from a WPT import) are exported to WPT. Please see > https://trac.webkit.org/wiki/WPTExportProcess The test is not yet in WPT, but it's going to be there before merging this change in WebKit. The test was originally a Chromium internal test and it's being moved upstream in the following Chromium patch: https://chromium-review.googlesource.com/c/chromium/src/+/2610084
Created attachment 416945 [details] Patch
The patch seems correct but it's causing failures in several tests so please do address them.
Created attachment 416965 [details] Patch
The WPT test has been already merged upstream, and the EWSs are green now.
Created attachment 416977 [details] Patch
Comment on attachment 416977 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416977&action=review LGTM, but I'd rather tweak the WPT tests first and reimport all in preliminary patch. > Source/WebCore/ChangeLog:9 > + The specs seem clear about the expected behavior for this: nit: s/is/seem/ ? > Source/WebCore/ChangeLog:15 > + A similar fix has been done on Blink in r201118 by <kochi@chromium.org>. maybe add a reference to https://src.chromium.org/viewvc/blink?revision=201118&view=revision ? > LayoutTests/ChangeLog:11 > + and these tests will be re-imported and unskipped in a follow-up patch. Maybe it would be better to import all sync changes in a preliminary patch, and then just changes the expectation in this patch. > LayoutTests/imported/w3c/ChangeLog:11 > + - focus-visible-005.html now fails, but it was passing because it was checking anything due to this bug. was not > LayoutTests/imported/w3c/web-platform-tests/css/selectors/focus-in-focus-event-001-expected.txt:3 > +PASS Checks that ':focus' pseudo-class matches inside 'focus' evente handler event handler > LayoutTests/imported/w3c/web-platform-tests/css/selectors/focus-in-focus-event-001.html:17 > + }, "Checks that ':focus' pseudo-class matches inside 'focus' evente handler"); event handler
(In reply to Frédéric Wang (:fredw) from comment #9) > Comment on attachment 416977 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=416977&action=review > > LGTM, but I'd rather tweak the WPT tests first and reimport all in > preliminary patch. Thanks for the review, I believe I prefer the current order, as the modified tests won't do anything useful without this patch. And this patch is about fixing a clear issue with its own WPT test, so I prefer not to mix it too much with the :focus-visible tests, skipping them sounds fine so far as we don't have any implementation. And also I already have the patch in the related but to update them. > > > Source/WebCore/ChangeLog:9 > > + The specs seem clear about the expected behavior for this: > > nit: s/is/seem/ ? Fixed. > > > Source/WebCore/ChangeLog:15 > > + A similar fix has been done on Blink in r201118 by <kochi@chromium.org>. > > maybe add a reference to > https://src.chromium.org/viewvc/blink?revision=201118&view=revision ? Done. > > > LayoutTests/ChangeLog:11 > > + and these tests will be re-imported and unskipped in a follow-up patch. > > Maybe it would be better to import all sync changes in a preliminary patch, > and then just changes the expectation in this patch. As mentioned earlier, I'll do it the other way around. > > > LayoutTests/imported/w3c/ChangeLog:11 > > + - focus-visible-005.html now fails, but it was passing because it was checking anything due to this bug. > > was not Nice catch, fixed too. > > > LayoutTests/imported/w3c/web-platform-tests/css/selectors/focus-in-focus-event-001-expected.txt:3 > > +PASS Checks that ':focus' pseudo-class matches inside 'focus' evente handler > > event handler Ack. > > > LayoutTests/imported/w3c/web-platform-tests/css/selectors/focus-in-focus-event-001.html:17 > > + }, "Checks that ':focus' pseudo-class matches inside 'focus' evente handler"); > > event handler Fixed upstream: https://github.com/web-platform-tests/wpt/pull/27042
Created attachment 416990 [details] Patch
Committed r271146: <https://trac.webkit.org/changeset/271146> All reviewed patches have been landed. Closing bug and clearing flags on attachment 416990 [details].
<rdar://problem/72810530>