[webkit-reviews] review granted: [Bug 220243] [selectors] :focus should match inside the focus event : [Attachment 416977] Patch

[bugzilla-daemon at webkit.org]

Frédéric Wang (:fredw) <fred.wang at free.fr> has granted Manuel Rego Casasnovas
<rego at igalia.com>'s request for review:
Bug 220243: [selectors] :focus should match inside the focus event
https://bugs.webkit.org/show_bug.cgi?id=220243

Attachment 416977: Patch

https://bugs.webkit.org/attachment.cgi?id=416977&action=review




--- Comment #9 from Frédéric Wang (:fredw) <fred.wang at free.fr> ---
Comment on attachment 416977
  --> https://bugs.webkit.org/attachment.cgi?id=416977
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 at 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