Closed Bug 1308080 Opened 8 years ago Closed 2 years ago

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

Categories

(Core :: Layout, defect, P3)

defect

Tracking

()

RESOLVED FIXED
106 Branch
Tracking Status
firefox106 --- fixed

People

(Reporter: TYLin, Assigned: emilio)

References

(Blocks 3 open bugs)

Details

Attachments

(1 file)

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.
Priority: -- → P3
See Also: → 1472897
Blocks: 1558049

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.

Assignee: nobody → emilio
Status: NEW → ASSIGNED
Flags: needinfo?(emilio)
Blocks: 1788179
See Also: → 1789166
Attachment #9071676 - Attachment description: Bug 1308080 - Make HTMLDetailsElement use a shadow tree rather than custom layout code. r=TYLin → WIP: Bug 1308080 - Make <details> use a shadow tree as per spec. r=TYLin,smaug
Attachment #9071676 - Attachment description: WIP: Bug 1308080 - Make <details> use a shadow tree as per spec. r=TYLin,smaug → Bug 1308080 - Make <details> use a shadow tree as per spec. r=TYLin,smaug
Attachment #9071676 - Attachment description: Bug 1308080 - Make <details> use a shadow tree as per spec. r=TYLin,smaug → WIP: Bug 1308080 - Make <details> use a shadow tree as per spec. r=TYLin,smaug
Attachment #9071676 - Attachment description: WIP: Bug 1308080 - Make <details> use a shadow tree as per spec. r=TYLin,smaug → Bug 1308080 - Make <details> use a shadow tree as per spec. r=TYLin,smaug
Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/27c0485f1740
Make <details> use a shadow tree as per spec. r=TYLin,smaug

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

Flags: needinfo?(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).

Flags: needinfo?(emilio) → needinfo?(aethanyc)

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.

Flags: needinfo?(aethanyc)
Blocks: 1791144
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6ae1077f7310
Make <details> use a shadow tree as per spec. r=TYLin,smaug
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 106 Branch

== 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

Blocks: 1790083
Regressions: 1791873
No longer regressions: 1791873
Regressions: 1791873
No longer regressions: 1791873
Regressions: 1794720
Regressions: 1797957
Regressions: 1804925
Regressions: 1880988
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: