Bug 170935 – [selectors4] :focus-within tests using shadow DOM don't pass on WK1

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Manuel Rego Casasnovas]

This issue has been detected while importing the W3C Test Suite in bug #170898.

There's a timing issue on Mac WK1 and the result doesn't match the expected in the following tests:
* imported/w3c/web-platform-tests/css/selectors4/focus-within-shadow-001.html
* imported/w3c/web-platform-tests/css/selectors4/focus-within-shadow-002.html
* imported/w3c/web-platform-tests/css/selectors4/focus-within-shadow-003.html
* imported/w3c/web-platform-tests/css/selectors4/focus-within-shadow-004.html
* imported/w3c/web-platform-tests/css/selectors4/focus-within-shadow-005.html

And probably the same will happen for focus-within-shadow-006.html,
but that's skipped for now due to a different reason (see bug #170899).

These tests use the "reftest-wait" class (http://web-platform-tests.org/writing-tests/reftests.html)
but maybe that's ignored in WK1 or something.

Using things like "testRunner.waitUntilDone()" and "testRunner.notifyDone()"
make it work as expected.

[Manuel Rego Casasnovas]

Actually I've realized that if we remove the "setTimeout()" call
on these tests they pass in both WK1 and WK2,
so probably this could be directly fixed upstream on WPT repo.

[Manuel Rego Casasnovas]

(In reply to Manuel Rego Casasnovas from comment #1)
> Actually I've realized that if we remove the "setTimeout()" call
> on these tests they pass in both WK1 and WK2,
> so probably this could be directly fixed upstream on WPT repo.

I've a PR to fix this on WPT (once it's merged, I'll re-import the tests to get this fixed):
https://github.com/w3c/web-platform-tests/pull/5594

[Manuel Rego Casasnovas]

Created attachment 309033 [details]
Patch

[Manuel Rego Casasnovas]

(In reply to Manuel Rego Casasnovas from comment #2)
> I've a PR to fix this on WPT (once it's merged, I'll re-import the tests to
> get this fixed):
> https://github.com/w3c/web-platform-tests/pull/5594

This has been merged now, so I'm re-importing the tests to get this issue fixed.
Youenn, please could you take a look?

Note that 2 new tests are now failing due to bug #47182:
* focus-display-none-001.html
* focus-within-display-none-001.html

I've added the expected results with the "FAIL" messages.
What's the way to go with this kind of issues?
I can think in different options:
1) Skip the tests in TextExpectations. Probably not good as
   we won't realize if the tests start to pass due to some other patch.
2) Add the expected results with the "FAIL" messages (what I'm doing here).
3) Add the expected results with "PASS" messages and mark the tests as Failure
   on TestExpectations.
4) Any other option?

Please tell me what's the preferred one. Thanks.

[Manuel Rego Casasnovas]

Ping Youenn, could you take a look? Thanks.

[youenn fablet]

(In reply to Manuel Rego Casasnovas from comment #4)
> (In reply to Manuel Rego Casasnovas from comment #2)
> > I've a PR to fix this on WPT (once it's merged, I'll re-import the tests to
> > get this fixed):
> > https://github.com/w3c/web-platform-tests/pull/5594
> 
> This has been merged now, so I'm re-importing the tests to get this issue
> fixed.

When you are reimporting tests, you could use -t option to get WPT ToT.
Something like: Tools/Scripts/import-w3c-tests web-platform-tests/css/selector4 -t
The script will create a hidden clone of WPT repository.

> Youenn, please could you take a look?

Sure

> Note that 2 new tests are now failing due to bug #47182:
> * focus-display-none-001.html
> * focus-within-display-none-001.html
> 
> I've added the expected results with the "FAIL" messages.
> What's the way to go with this kind of issues?
> I can think in different options:
> 1) Skip the tests in TextExpectations. Probably not good as
>    we won't realize if the tests start to pass due to some other patch.
> 2) Add the expected results with the "FAIL" messages (what I'm doing here).
> 3) Add the expected results with "PASS" messages and mark the tests as
> Failure
>    on TestExpectations.
> 4) Any other option?
> 
> Please tell me what's the preferred one. Thanks.

Right, 2 is the preferred option.
It is true that WPT are about conformance and regression testing at the same time.
I would favor regression testing.

[youenn fablet]

Comment on attachment 309033 [details]
Patch

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

> LayoutTests/imported/w3c/web-platform-tests/css/selectors4/focus-within-display-none-001.html:9
> +<script src="../../../../../resources/testharnessreport.js"></script>

This is one small difference here of importing from the hidden clone or from your specific clone.
With the hidden clone, testharness links would remain the same.
We might want to switch to that type of links but we are not doing so yet.

Also, going from the clone created by the script ensures that you only import changes from WPT master.

[Manuel Rego Casasnovas]

Created attachment 309714 [details]
Patch for landing

[Manuel Rego Casasnovas]

Comment on attachment 309033 [details]
Patch

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

Thanks for the information!

>> LayoutTests/imported/w3c/web-platform-tests/css/selectors4/focus-within-display-none-001.html:9
>> +<script src="../../../../../resources/testharnessreport.js"></script>
> 
> This is one small difference here of importing from the hidden clone or from your specific clone.
> With the hidden clone, testharness links would remain the same.
> We might want to switch to that type of links but we are not doing so yet.
> 
> Also, going from the clone created by the script ensures that you only import changes from WPT master.

Ok, I've imported using the "-t" command.

[Build Bot]

Comment on attachment 309714 [details]
Patch for landing

Attachment 309714 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3718845

New failing tests:
media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-buttons-styles.html

[Build Bot]

Created attachment 309723 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Manuel Rego Casasnovas]

Comment on attachment 309714 [details]
Patch for landing

EWS failures are unrelated to this patch. Let's land this.

[WebKit Commit Bot]

Comment on attachment 309714 [details]
Patch for landing

Clearing flags on attachment: 309714

Committed r216736: <http://trac.webkit.org/changeset/216736>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.