Bug 224601 – [selectors] Update :focus-visible tests from WPT

• Component: New Bugs
• Status: RESOLVED FIXED

[Manuel Rego Casasnovas]

[selectors] Update :focus-visible tests from WPT

[Manuel Rego Casasnovas]

Created attachment 426097 [details]
Patch

[Manuel Rego Casasnovas]

Created attachment 426101 [details]
Patch

[Manuel Rego Casasnovas]

Created attachment 426103 [details]
Patch

[Darin Adler]

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?

[Manuel Rego Casasnovas]

Created attachment 426122 [details]
Patch

[Manuel Rego Casasnovas]

(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.

[Darin Adler]

(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.

[Manuel Rego Casasnovas]

(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.

[Manuel Rego Casasnovas]

Created attachment 426182 [details]
Patch

[Manuel Rego Casasnovas]

Created attachment 426184 [details]
Patch

[Manuel Rego Casasnovas]

Created attachment 426193 [details]
Patch for landing

[Manuel Rego Casasnovas]

(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.

[EWS]

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].

[Darin Adler]

Looks to me like the thing we landed does still have the changes due to the bug in the expected files!

[Darin Adler]

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.

[Darin Adler]

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.

[Manuel Rego Casasnovas]

Reopening to attach new patch.

[Manuel Rego Casasnovas]

Created attachment 426394 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/76827947>

[Manuel Rego Casasnovas]

Created attachment 426395 [details]
Patch

[EWS]

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].