Bug 171984 – Add a RuntimeEnabledFeature for display: contents

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Emilio Cobos Álvarez]

I plan to work on this over the next few weeks.

I've gone through the tests, and the state of display: contents is not great when enabling it to all elements, so I think we should keep it turned off by default (except for <slot>).

I'd want to add a flag for this that is on during testing, but off everywhere else.

[Emilio Cobos Álvarez]

Created attachment 309749 [details]
Patch

[Build Bot]

Comment on attachment 309749 [details]
Patch

Attachment 309749 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3720160

New failing tests:
imported/w3c/web-platform-tests/innerText/getter.html

[Build Bot]

Created attachment 309778 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

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

[Build Bot]

Comment on attachment 309749 [details]
Patch

Attachment 309749 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3720040

New failing tests:
imported/w3c/web-platform-tests/innerText/getter.html

[Build Bot]

Created attachment 309779 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Ryosuke Niwa]

Antti implemented this feature so he should be reviewing your patches related to display: contents.

[Emilio Cobos Álvarez]

Created attachment 309782 [details]
Patch

[Emilio Cobos Álvarez]

(In reply to Ryosuke Niwa from comment #6)
> Antti implemented this feature so he should be reviewing your patches
> related to display: contents.

Sounds good, thanks!

[Antti Koivisto]

One of the first steps should be to import the tests from

https://github.com/w3c/web-platform-tests/tree/master/css/css-display-3

[Emilio Cobos Álvarez]

Created attachment 309796 [details]
Patch

[Emilio Cobos Álvarez]

Created attachment 309797 [details]
Patch

[Emilio Cobos Álvarez]

Err, sorry for the noise, I can't land the patch myself yet. Last version has the "Reviewed by..." line, if somebody could land it for me that'd be awesome.

(In reply to Antti Koivisto from comment #9)
> One of the first steps should be to import the tests from
> 
> https://github.com/w3c/web-platform-tests/tree/master/css/css-display-3

Yeah, I have patches locally for this, which I'll submit for review tomorrow, probably.

Thanks for the review Antti!

[Ryosuke Niwa]

Comment on attachment 309797 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Add a RuntimeEnabledFeature for display: contents, defaulted to false.
> +
> +        The "defaulted to false" is not only because there are spec issues, but because
> +        I ran the WPT suite, and there was a fair amount of crashes and messed render
> +        trees.
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=171984
> +
> +        Reviewed by Antti Koivisto.

Change log here is really messed up.

The bug title should be followed by a bug URL, followed by a blank line, then a long description.

[Build Bot]

Comment on attachment 309797 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/innerText/getter.html

[Build Bot]

Created attachment 309824 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

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

[Build Bot]

Comment on attachment 309797 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/innerText/getter.html

[Build Bot]

Created attachment 309825 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

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

[Sam Weinig]

Is there any reason this need to be a RuntimeEnabledFeature rather than a Setting?

[Emilio Cobos Álvarez]

Created attachment 310084 [details]
Patch

[Emilio Cobos Álvarez]

(In reply to Ryosuke Niwa from comment #13)
> Comment on attachment 309797 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309797&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        Add a RuntimeEnabledFeature for display: contents, defaulted to false.
> > +
> > +        The "defaulted to false" is not only because there are spec issues, but because
> > +        I ran the WPT suite, and there was a fair amount of crashes and messed render
> > +        trees.
> > +
> > +        https://bugs.webkit.org/show_bug.cgi?id=171984
> > +
> > +        Reviewed by Antti Koivisto.
> 
> Change log here is really messed up.
> 
> The bug title should be followed by a bug URL, followed by a blank line,
> then a long description.

Thanks for the heads-up, prepare-ChangeLog posted the whole git commit description before the url field, and no script complained about it, so I just assumed it was fine. I updated and amended it.

(In reply to Sam Weinig from comment #18)
> Is there any reason this need to be a RuntimeEnabledFeature rather than a
> Setting?

From a quick look RuntimeEnabledFeatures it what's usually used for web features, but happy to change it if there's a good reason to.

[Build Bot]

Comment on attachment 310084 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/innerText/getter.html

[Build Bot]

Created attachment 310085 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

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

[Build Bot]

Comment on attachment 310084 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/innerText/getter.html

[Build Bot]

Created attachment 310086 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

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

[Emilio Cobos Álvarez]

So I have no clue why the test is failing in OSX. I guess the test runner is not picking up the flag?

Presumably I'm missing some WebView.mm bits or something? Not sure how should I proceed given I don't own a Mac. I guess I'll try some more monkey-see-monkey-do based on other preferences, probably I need to add it to the EXPERIMENTAL_FEATURES?

I'll flag for review again once I get it working.

[Emilio Cobos Álvarez]

Oh, and regarding Settings vs. RuntimeEnabledFeatures, it looks to me per [1] that RuntimeEnabledFeatures should be the right place for it, but maybe I'm missing something.

[1]: https://lists.webkit.org/pipermail/webkit-dev/2017-January/028678.html

[Emilio Cobos Álvarez]

Created attachment 310088 [details]
WIP

[Emilio Cobos Álvarez]

Created attachment 310089 [details]
WIP

[Emilio Cobos Álvarez]

Created attachment 310090 [details]
WIP

[Emilio Cobos Álvarez]

Created attachment 310092 [details]
Patch

[Emilio Cobos Álvarez]

Ok, that seemed to help, could I get the patch re-reviewed please?

Thanks a lot in advance!

[WebKit Commit Bot]

Comment on attachment 310092 [details]
Patch

Clearing flags on attachment: 310092

Committed r216974: <http://trac.webkit.org/changeset/216974>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

Reverted r216974 for reason:

Revision caused consistent timeouts on all platforms.

Committed r216981: <http://trac.webkit.org/changeset/216981>

[Matt Lewis]

The Builds and history:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2FinnerText%2Fgetter.html

https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK2%20(Tests)/builds/1602
https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/builds/1177
https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20(Tests)/builds/1544
https://build.webkit.org/builders/Apple%20iOS%2010%20Simulator%20Debug%20WK2%20(Tests)/builds/1416
https://build.webkit.org/builders/Apple%20iOS%2010%20Simulator%20Release%20WK2%20(Tests)/builds/1523

The test are timing out on both wk1 and wk2, Release and Debug, of all macOS, iOS Simulator, and GTK

[Emilio Cobos Álvarez]

Thanks for reopening Matt.

I can repro after rebasing my checkout. I've bisected and it's caused by https://trac.webkit.org/changeset/216972/webkit.

I still don't know enough about that code to know whether that's correct or the bug lies somewhere else, but I guess I'll need to come up with a fix...

[Emilio Cobos Álvarez]

Err, I meant https://trac.webkit.org/changeset/216966/webkit

[Emilio Cobos Álvarez]

I'm still debugging it, and it's fascinating.

The change mentioned in the previous comment looks correct to me, and indeed the test passes in isolation. It seems all related to the following <svg> container in the getter.html. If I remove the SVG container (and the svg tests), it just works. If I remove only the svg tests, but I leave the svg container, the second display: contents test times out.

[Emilio Cobos Álvarez]

Created attachment 310869 [details]
Patch

[Emilio Cobos Álvarez]

(In reply to Emilio Cobos Álvarez from comment #39)
> Created attachment 310869 [details]
> Patch

I shouldn't have really uploaded this yet, sorry about that. It's just the same patch rebased, but apparently webkit-patch upload -g HEAD~ (where HEAD~ was the patch for bug 172443) did the wrong thing here.

Anyway this should be good to go if/when bug 172443 lands.

[Build Bot]

Comment on attachment 310869 [details]
Patch

Attachment 310869 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3794436

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 310874 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

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

[Build Bot]

Comment on attachment 310869 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 310878 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

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

[Build Bot]

Comment on attachment 310869 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 310880 [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

[Build Bot]

Comment on attachment 310869 [details]
Patch

Attachment 310869 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3794488

New failing tests:
fast/forms/input-text-paste-maxlength.html
fast/forms/input-text-maxlength.html
accessibility/ios-simulator/speak-selection-content.html
fast/forms/textarea-maxlength.html
editing/text-iterator/count-matches-in-form.html
editing/text-iterator/find-string-on-flat-tree.html
fast/forms/number/number-validity-badinput.html
fast/forms/input-value-sanitization.html

[Build Bot]

Created attachment 310881 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Emilio Cobos Álvarez]

Created attachment 310911 [details]
Patch

[Build Bot]

Comment on attachment 310911 [details]
Patch

Attachment 310911 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3796056

New failing tests:
imported/w3c/web-platform-tests/cssom/getComputedStyle-pseudo.html

[Build Bot]

Created attachment 310921 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

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

[Build Bot]

Comment on attachment 310911 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/cssom/getComputedStyle-pseudo.html

[Build Bot]

Created attachment 310925 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

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

[Build Bot]

Comment on attachment 310911 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/cssom/getComputedStyle-pseudo.html

[Build Bot]

Created attachment 310928 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

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

[Build Bot]

Comment on attachment 310911 [details]
Patch

Attachment 310911 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3796169

New failing tests:
imported/w3c/web-platform-tests/cssom/getComputedStyle-pseudo.html

[Build Bot]

Created attachment 310933 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Emilio Cobos Álvarez]

(In reply to Build Bot from comment #57)
> Created attachment 310933 [details]
> Archive of layout-test-results from ews126 for ios-simulator-wk2
> 
> The attached test failures were seen while running run-webkit-tests on the
> ios-sim-ews.
> Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

Nothing like making your own patch turn red because of a test written by you :-)

(https://github.com/w3c/web-platform-tests/pull/5830)

[Emilio Cobos Álvarez]

Created attachment 310953 [details]
Patch

[Emilio Cobos Álvarez]

Comment on attachment 310953 [details]
Patch

Hi Antti, could I get this re-reviewed/relanded?

This is the same patch as before, just rebased, and with some expectations updated for a newly imported test too.

[WebKit Commit Bot]

Comment on attachment 310953 [details]
Patch

Clearing flags on attachment: 310953

Committed r217273: <http://trac.webkit.org/changeset/217273>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

(In reply to WebKit Commit Bot from comment #61)
> Comment on attachment 310953 [details]
> Patch
> 
> Clearing flags on attachment: 310953
> 
> Committed r217273: <http://trac.webkit.org/changeset/217273>

It seems that imported/w3c/web-platform-tests/innerText/getter.html is hitting a flaky assertion failure on macOS and iOS after this change:

https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r217275%20(1295)/results.html

[Emilio Cobos Álvarez]

(In reply to Ryan Haddad from comment #63)
> (In reply to WebKit Commit Bot from comment #61)
> > Comment on attachment 310953 [details]
> > Patch
> > 
> > Clearing flags on attachment: 310953
> > 
> > Committed r217273: <http://trac.webkit.org/changeset/217273>
> 
> It seems that imported/w3c/web-platform-tests/innerText/getter.html is
> hitting a flaky assertion failure on macOS and iOS after this change:
> 
> https://build.webkit.org/results/
> Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r217275%20(1295)/results.html

Ugh, so that's a |Text| instance being removed, and it seems to be failing the !renderer() assertion, which looks plausible.

I'll try to look into it during the next few days, but I'd prefer not to revert this patch if it's not too much of a priority, because the patch just enables testing support for a feature that is conditionally enabled right now, and all these issues indicate that there are potentially a lot of other related bugs lying around I think.

[Alexey Proskuryakov]

Additionally, this test started failing on Windows:

https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r217488%20(816)/imported/w3c/web-platform-tests/innerText/getter-pretty-diff.html