Bug 170935 - [selectors4] :focus-within tests using shadow DOM don't pass on WK1
Summary: [selectors4] :focus-within tests using shadow DOM don't pass on WK1
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Manuel Rego Casasnovas
URL:
Keywords:
Depends on: 170898
Blocks:
  Show dependency treegraph
 
Reported: 2017-04-18 00:48 PDT by Manuel Rego Casasnovas
Modified: 2017-05-11 23:58 PDT (History)
6 users (show)

See Also:


Attachments
Patch (26.74 KB, patch)
2017-05-04 05:10 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch for landing (26.19 KB, patch)
2017-05-11 09:21 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-elcapitan (1.57 MB, application/zip)
2017-05-11 10:49 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas 2017-04-18 00:48:27 PDT
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.
Comment 1 Manuel Rego Casasnovas 2017-04-18 00:53:04 PDT
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.
Comment 2 Manuel Rego Casasnovas 2017-04-24 22:07:52 PDT
(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
Comment 3 Manuel Rego Casasnovas 2017-05-04 05:10:31 PDT
Created attachment 309033 [details]
Patch
Comment 4 Manuel Rego Casasnovas 2017-05-04 07:07:37 PDT
(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.
Comment 5 Manuel Rego Casasnovas 2017-05-09 12:09:18 PDT
Ping Youenn, could you take a look? Thanks.
Comment 6 youenn fablet 2017-05-11 08:23:10 PDT
(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.
Comment 7 youenn fablet 2017-05-11 08:25:47 PDT
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.
Comment 8 Manuel Rego Casasnovas 2017-05-11 09:21:45 PDT
Created attachment 309714 [details]
Patch for landing
Comment 9 Manuel Rego Casasnovas 2017-05-11 09:23:19 PDT
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.
Comment 10 Build Bot 2017-05-11 10:49:28 PDT
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
Comment 11 Build Bot 2017-05-11 10:49:29 PDT
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
Comment 12 Manuel Rego Casasnovas 2017-05-11 23:30:19 PDT
Comment on attachment 309714 [details]
Patch for landing

EWS failures are unrelated to this patch. Let's land this.
Comment 13 WebKit Commit Bot 2017-05-11 23:58:24 PDT
Comment on attachment 309714 [details]
Patch for landing

Clearing flags on attachment: 309714

Committed r216736: <http://trac.webkit.org/changeset/216736>
Comment 14 WebKit Commit Bot 2017-05-11 23:58:26 PDT
All reviewed patches have been landed.  Closing bug.