Summary: | [selectors] :focus should match inside the focus event | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Manuel Rego Casasnovas <rego> | ||||||||||||
Component: | CSS | Assignee: | Manuel Rego Casasnovas <rego> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | cdumez, clopez, esprehn+autocc, ews-watchlist, fred.wang, kangil.han, svillar, webkit-bug-importer, youennf | ||||||||||||
Priority: | P2 | Keywords: | BrowserCompat, InRadar, WebExposed | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
See Also: | https://bugs.chromium.org/p/chromium/issues/detail?id=523126 | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 220312 | ||||||||||||||
Attachments: |
|
Description
Manuel Rego Casasnovas
2021-01-04 08:32:33 PST
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]. |