Bug 178332 – Text nodes with display:contents parent should render as if they were wrapped in an unstyled <span>

• Component: CSS
• Status: RESOLVED FIXED

[Antti Koivisto]

According to https://github.com/w3c/csswg-drafts/issues/1118

<div style="display:contents;color:green">text</div>

must result in green text even though div doesn't generate a box.

[Antti Koivisto]

Created attachment 323877 [details]
patch

[Antti Koivisto]

Created attachment 323879 [details]
patch

[Antti Koivisto]

Created attachment 323920 [details]
patch

[Alexey Proskuryakov]

This patch is currently killing every EWS bot that tries to process it, looking into it...

[Chris Dumez]

(In reply to Alexey Proskuryakov from comment #4)
> This patch is currently killing every EWS bot that tries to process it,
> looking into it...

Antti has some serious power :D

[Alexey Proskuryakov]

There are multiple failures when running tests for this patch manually on the bot, not sure how real they are

[8914/45246] fast/css-generated-content/details-summary-before-after.html failed unexpectedly (text diff)
[13699/45246] fast/html/details-add-child-1.html failed unexpectedly (text diff)
[13788/45246] fast/html/details-add-child-2.html failed unexpectedly (text diff)
[13875/45246] fast/html/details-add-details-child-1.html failed unexpectedly (text diff)
[13963/45246] fast/html/details-add-details-child-2.html failed unexpectedly (text diff)
[14052/45246] fast/html/details-add-summary-1-and-click.html failed unexpectedly (text diff)
[14142/45246] fast/html/details-add-summary-1.html failed unexpectedly (text diff)
[14228/45246] fast/html/details-add-summary-2-and-click.html failed unexpectedly (text diff)
[14378/45246] fast/html/details-add-summary-2.html failed unexpectedly (text diff)
[14456/45246] fast/html/details-add-summary-3-and-click.html failed unexpectedly (text diff)
[14523/45246] fast/html/details-add-summary-3.html failed unexpectedly (text diff)
[14591/45246] fast/html/details-add-summary-4-and-click.html failed unexpectedly (text diff)
[14665/45246] fast/html/details-add-summary-4.html failed unexpectedly (text diff)
[14677/45246] fast/multicol/span/clone-anonymous-block-non-inline-child-crash.html failed unexpectedly (text diff)
[14713/45246] fast/html/details-add-summary-5-and-click.html failed unexpectedly (text diff)
[14738/45246] fast/multicol/span/clone-summary.html failed unexpectedly (text diff)
[14768/45246] fast/html/details-add-summary-5.html failed unexpectedly (text diff)
[14800/45246] fast/html/details-add-summary-6-and-click.html failed unexpectedly (text diff)
[14856/45246] fast/html/details-add-summary-6.html failed unexpectedly (text diff)
[14945/45246] fast/html/details-add-summary-7-and-click.html failed unexpectedly (text diff)
[15014/45246] fast/html/details-add-summary-7.html failed unexpectedly (text diff)
[15081/45246] fast/html/details-add-summary-8-and-click.html failed unexpectedly (text diff)
[15147/45246] fast/html/details-add-summary-8.html failed unexpectedly (text diff)
[15228/45246] fast/html/details-add-summary-9-and-click.html failed unexpectedly (text diff)
[15316/45246] fast/html/details-add-summary-9.html failed unexpectedly (text diff)
[15370/45246] fast/html/details-add-summary-10-and-click.html failed unexpectedly (text diff)
[15466/45246] fast/html/details-add-summary-10.html failed unexpectedly (text diff)
[15664/45246] fast/html/details-add-summary-child-1.html failed unexpectedly (text diff)
[15786/45246] fast/html/details-add-summary-child-2.html failed unexpectedly (text diff)
[16099/45246] fast/html/details-marker-style.html failed unexpectedly (text diff)
...
and then progress stops.

[Alexey Proskuryakov]

Looks like the problem is that a patch that causes more than 30 failures kills EWS bots now. There may be some other circumstances required to reproduce, Jonathan will take a look.

In the meanwhile, please don't post patches that break many tests ;-)

[Alexey Proskuryakov]

Comment on attachment 323920 [details]
patch

Obsoleting the patch, so that it doesn't keep destroying EWS.

[Antti Koivisto]

> [8914/45246] fast/css-generated-content/details-summary-before-after.html
> failed unexpectedly (text diff)
> [13699/45246] fast/html/details-add-child-1.html failed unexpectedly (text
> diff)
> [13788/45246] fast/html/details-add-child-2.html failed unexpectedly (text
> diff)
> [13875/45246] fast/html/details-add-details-child-1.html failed unexpectedly
> (text diff)

Wish there was a way to see this while the tests are still running.

[Antti Koivisto]

Created attachment 324001 [details]
patch

[Ryosuke Niwa]

Comment on attachment 324001 [details]
patch

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

> Source/WebCore/rendering/RenderElement.cpp:228
> +        auto* textRendererWithDisplayContentsParent = RenderText::findForDisplayContentsInlineWrapper(rendererForFirstLineStyle);

Maybe we call this findByDisplayContentsInlineWrapperCandidate instead?

> Source/WebCore/rendering/RenderText.cpp:137
> +    static NeverDestroyed<HashMap<const RenderText*, WeakPtr<RenderInline>>> map;

It would be nice if we can make it so that nobody can possibly de-reference RenderText* so that it's UAF-safe
but that'd require quite a bit of hash traits hackery... :(

> Source/WebCore/rendering/RenderText.cpp:1761
> +        inlineWrapperForDisplayContentsMap().remove(this);

Can we assert that the entry did exist in the map?

> Source/WebCore/rendering/RenderText.cpp:1765
> +    inlineWrapperForDisplayContentsMap().add(this, makeWeakPtr(wrapper));

Can we assert that this is isNewEntry?

> LayoutTests/ChangeLog:10
> [phone]  Antti Koivisto  <[e-mail]>

Double change log entries.

[Antti Koivisto]

Created attachment 324006 [details]
patch

[Antti Koivisto]

*** Bug 177060 has been marked as a duplicate of this bug. ***

[WebKit Commit Bot]

Comment on attachment 324006 [details]
patch

Clearing flags on attachment: 324006

Committed r223514: <https://trac.webkit.org/changeset/223514>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35026267>

[Darin Adler]

Comment on attachment 324006 [details]
patch

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

> Source/WebCore/style/StyleUpdate.h:73
> +    std::optional<std::unique_ptr<RenderStyle>> inheritedDisplayContentsStyle;

Seems a little strange to use std::optional<std::unique_ptr<>>; gives us both nullopt and nullptr, but do we really need both?

[Antti Koivisto]

> Seems a little strange to use std::optional<std::unique_ptr<>>; gives us
> both nullopt and nullptr, but do we really need both?

Yeah, nullopt signals that we don't have a value and nullptr signals we have a value and it is null.

I did debate with myself about this vs using a separate bit. This looked better in code.