1308080 - Re-implement move the first summary to the front on details' shadow tree

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

The spec says this for details element.

The element's shadow tree is expected to take the element's first child summary element, if any, and place it in a first block box container, and then take the element's remaining descendants, if any, and place them in a second block box container.

https://html.spec.whatwg.org/multipage/rendering.html#the-details-and-summary-elements

Currently, we do this on frame construction list in nsCSSFrameConstructor::AddFrameConstructionItemsInternal(). We should re-implement this behavior on details' shadow tree.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1308080 - Make <details> use a shadow tree as per spec. r=TYLin,smaug

Pretty much just wanted to know what would it take to do this. This is sort-of
inefficient: if we'd take this I'd move the stylesheet to a <link> so that it's
at least shared across <details> elements.

This is only failing the pseudo-element tests, which is sorta-expected per
bug 1472897.

This fixes bug 1558049, and probably makes straight-forward to make details
elements support more stuff like flex or grid, if we wanted to.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/27c0485f1740
Make <details> use a shadow tree as per spec. r=TYLin,smaug

Comment 3

[Sandor Molnar[:smolnar]]

Backed out changeset 27c0485f1740 (bug 1308080) for causing assertion failure in dom/ShadowRoot.h & wr failures in /css/css-multicol/multicol-span-all-dynamic

Backout link: https://hg.mozilla.org/integration/autoland/rev/c39d663fc19689cb546d1567b9c80648f5f09822

Push with failures

Failure log assertion

Failure log wr4

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Ah, the unlink issue was very easy to fix. However the wr4 failure not so much...

Failure is here: https://treeherder.mozilla.org/jobs?repo=try&revision=dfeb4b55e7bd75cc2dcd7ff7607df6df94c34c15

Repros consistently on both Win and macOS on automation. The patch adds an element to a <details> in a multicol context.

At first I thought that it might've been a whitespace-collapsing issue, but that can't be it, see the fixup commit in the try run. I think the issue is that before my patch we'd reconstruct the whole <details> on insertion here. Now we don't, which is better.

I'd debug more thoroughly, but I can't repro locally on either Windows or Linux. Ting-Yu, would you think it'd be ok for me to annotate the test on macOS and Windows pointing to a follow-up bug? I can try to repro on macOS, but probably not till next week, and it doesn't seem a big issue (I'm ~sure before my patch this can repro with a regular block, if we just get the right frame tree).

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Ting-Yu, would you think it'd be ok for me to annotate the test on macOS and Windows pointing to a follow-up bug?

It should be ok to deal with this in a follow-up. Please file a bug track this.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ae1077f7310
Make <details> use a shadow tree as per spec. r=TYLin,smaug

Comment 7

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/6ae1077f7310

Comment 11

[Andrej (:aglavic)]

== Change summary for alert #35422 (as of Tue, 20 Sep 2022 05:53:49 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
5%	pdfpaint	macosx1015-64-shippable-qr	e10s fission stylo webrender	714.01 -> 676.10
5%	pdfpaint	macosx1015-64-shippable-qr	e10s fission stylo webrender-sw	706.21 -> 673.92

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=35422