Bug 224601 - [selectors] Update :focus-visible tests from WPT
Summary: [selectors] Update :focus-visible tests from WPT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Manuel Rego Casasnovas
URL:
Keywords: InRadar
Depends on:
Blocks: 224598
  Show dependency treegraph
 
Reported: 2021-04-15 05:28 PDT by Manuel Rego Casasnovas
Modified: 2021-04-18 23:57 PDT (History)
8 users (show)

See Also:


Attachments
Patch (102.11 KB, patch)
2021-04-15 05:28 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (104.51 KB, patch)
2021-04-15 06:10 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (104.86 KB, patch)
2021-04-15 07:23 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (107.94 KB, patch)
2021-04-15 11:57 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (108.16 KB, patch)
2021-04-15 22:08 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (108.22 KB, patch)
2021-04-15 22:34 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch for landing (106.42 KB, patch)
2021-04-16 01:07 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (4.80 KB, patch)
2021-04-18 22:09 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (12.11 KB, patch)
2021-04-18 22:16 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas 2021-04-15 05:28:25 PDT
[selectors] Update :focus-visible tests from WPT
Comment 1 Manuel Rego Casasnovas 2021-04-15 05:28:56 PDT
Created attachment 426097 [details]
Patch
Comment 2 Manuel Rego Casasnovas 2021-04-15 06:10:00 PDT
Created attachment 426101 [details]
Patch
Comment 3 Manuel Rego Casasnovas 2021-04-15 07:23:51 PDT
Created attachment 426103 [details]
Patch
Comment 4 Darin Adler 2021-04-15 10:27:45 PDT
Comment on attachment 426103 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426103&action=review

> LayoutTests/imported/w3c/web-platform-tests/css/selectors/focus-visible-002.html:33
> -  This test checks that <code>:focus-visible</code> always matches on <code>&lt;input&gt;</code> elements which take text input, regardless of focus mechanism.
> +  This test checks that <code>:focus-visible</code> always matches on <code><input></code> elements which take text input, regardless of focus mechanism.

Do we know who made this type of change in WPT and why?
Comment 5 Manuel Rego Casasnovas 2021-04-15 11:57:00 PDT
Created attachment 426122 [details]
Patch
Comment 6 Manuel Rego Casasnovas 2021-04-15 11:57:37 PDT
(In reply to Darin Adler from comment #4)
> Comment on attachment 426103 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426103&action=review
> 
> > LayoutTests/imported/w3c/web-platform-tests/css/selectors/focus-visible-002.html:33
> > -  This test checks that <code>:focus-visible</code> always matches on <code>&lt;input&gt;</code> elements which take text input, regardless of focus mechanism.
> > +  This test checks that <code>:focus-visible</code> always matches on <code><input></code> elements which take text input, regardless of focus mechanism.
> 
> Do we know who made this type of change in WPT and why?

The test in WPT hasn't changed, so maybe it's the import script doing the change now.
Comment 7 Darin Adler 2021-04-15 12:15:38 PDT
(In reply to Manuel Rego Casasnovas from comment #6)
> The test in WPT hasn't changed, so maybe it's the import script doing the
> change now.

Can you research this? Like changes have there been to the import script since we imported this correctly? I’m really concerned about damaging the tests like this while importing them. For now it might be harmless?

I would like to help fix this.
Comment 8 Manuel Rego Casasnovas 2021-04-15 12:40:28 PDT
(In reply to Darin Adler from comment #7)
> (In reply to Manuel Rego Casasnovas from comment #6)
> > The test in WPT hasn't changed, so maybe it's the import script doing the
> > change now.
> 
> Can you research this? Like changes have there been to the import script
> since we imported this correctly? I’m really concerned about damaging the
> tests like this while importing them. For now it might be harmless?
> 
> I would like to help fix this.

Yeah, I'll investigate it, and report my findings on a separated bug.

Maybe the first time I copied the tests manually instead of using the import (cannot remember, though I usually go with the import script), or maybe there were some recent change on the import script.
Comment 9 Manuel Rego Casasnovas 2021-04-15 22:08:19 PDT
Created attachment 426182 [details]
Patch
Comment 10 Manuel Rego Casasnovas 2021-04-15 22:34:04 PDT
Created attachment 426184 [details]
Patch
Comment 11 Manuel Rego Casasnovas 2021-04-16 01:07:04 PDT
Created attachment 426193 [details]
Patch for landing
Comment 12 Manuel Rego Casasnovas 2021-04-16 01:09:38 PDT
(In reply to Manuel Rego Casasnovas from comment #8)
> (In reply to Darin Adler from comment #7)
> > (In reply to Manuel Rego Casasnovas from comment #6)
> > > The test in WPT hasn't changed, so maybe it's the import script doing the
> > > change now.
> > 
> > Can you research this? Like changes have there been to the import script
> > since we imported this correctly? I’m really concerned about damaging the
> > tests like this while importing them. For now it might be harmless?
> > 
> > I would like to help fix this.
> 
> Yeah, I'll investigate it, and report my findings on a separated bug.
> 
> Maybe the first time I copied the tests manually instead of using the import
> (cannot remember, though I usually go with the import script), or maybe
> there were some recent change on the import script.

This was an issue in Python 3.5 or bigger, reported it with a fix:
https://bugs.webkit.org/show_bug.cgi?id=224658

I've used python2 on the last patch, so we avoid those modifications.

Nice catch Darin.
Comment 13 EWS 2021-04-16 01:53:42 PDT
Committed r276127 (236622@main): <https://commits.webkit.org/236622@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426193 [details].
Comment 14 Darin Adler 2021-04-16 10:45:16 PDT
Looks to me like the thing we landed does still have the changes due to the bug in the expected files!
Comment 15 Darin Adler 2021-04-16 10:46:02 PDT
I’m concerned that some of the files now have incorrect expectations in them, but are also skipped. Not a great state to be in.
Comment 16 Darin Adler 2021-04-16 10:46:43 PDT
One example:

    web-platform-tests/css/selectors/focus-visible-002-expected.txt

I don’t think this should have been changed at all as part of this import.
Comment 17 Manuel Rego Casasnovas 2021-04-18 22:09:46 PDT
Reopening to attach new patch.
Comment 18 Manuel Rego Casasnovas 2021-04-18 22:09:49 PDT
Created attachment 426394 [details]
Patch
Comment 19 Radar WebKit Bug Importer 2021-04-18 22:10:08 PDT
<rdar://problem/76827947>
Comment 20 Manuel Rego Casasnovas 2021-04-18 22:16:11 PDT
Created attachment 426395 [details]
Patch
Comment 21 EWS 2021-04-18 23:57:22 PDT
Committed r276237 (236719@main): <https://commits.webkit.org/236719@main>

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