Bug 220243 – [selectors] :focus should match inside the focus event

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Manuel Rego Casasnovas]

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.

[Manuel Rego Casasnovas]

Created attachment 416944 [details]
Patch

[EWS Watchlist]

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

[Manuel Rego Casasnovas]

(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

[Manuel Rego Casasnovas]

Created attachment 416945 [details]
Patch

[Sergio Villar Senin]

The patch seems correct but it's causing failures in several tests so please do address them.

[Manuel Rego Casasnovas]

Created attachment 416965 [details]
Patch

[Manuel Rego Casasnovas]

The WPT test has been already merged upstream, and the EWSs are green now.

[Manuel Rego Casasnovas]

Created attachment 416977 [details]
Patch

[Frédéric Wang (:fredw)]

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 <[e-mail]>.

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

[Manuel Rego Casasnovas]

(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 <[e-mail]>.
> 
> 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

[Manuel Rego Casasnovas]

Created attachment 416990 [details]
Patch

[EWS]

Committed r271146: <https://trac.webkit.org/changeset/271146>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416990 [details].

[Radar WebKit Bug Importer]

<rdar://problem/72810530>