1562701 - Rework the reframing logic for `<details>` elements

Status: RESOLVED FIXED

(Core :: Layout, task, P3)

Description

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Currently, when inserting a range of nodes into <details>, we treat <details> frames as if they have multiple insertion points. We then issue several single inserting commands, and insert the range of nodes one by one. However, what we do in inserting one node is merely reframing the <details> element. Also, the assertion that checks <details> should accept only single insertion cause bug 1547391 for multicol <details> with column-span:all children.

I think we can move the reframing logic for <details> into nsCSSFrameConstructor::WipeContainingBlock(), and do reframing if the range of nodes to be inserted contains <summary>; or do something smarter that checks the <summary> is going to be the main summary before do reframing.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Bug 1308080 would also make all this special logic unnecessary.

Comment 2

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #1)

Bug 1308080 would also make all this special logic unnecessary.

Right, bug 1308080 can make all this special logic go away. However, before we decide to take the patch, it still better to improve the special reframing logic to fix bug 1547391, and to reframe <details> only if a main <summary> is inserted.

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1562701 Part 1 - Check a details frame has a valid main summary in more places.

In next part, we are going to change the mechanism for reframing a
details frame, so this patch adds debug checks to more places like
AppendFrames, InsertFrame, and RemoveFrame to ensure the frame tree is
still sane.

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1562701 - Move the reframing logic of <details> element into WipeContainingBlock.

No tests added in this patch because the reftests
layout/reftests/details-summary/dynamic-add-*.html already cover adding
<summary> into a <details> in various situations.

Depends on D36528

Comment 5

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Re comment 0:

do reframing if the range of nodes to be inserted contains <summary>; or do something smarter that checks the <summary> is going to be the main summary before do reframing.

I realize it doesn't work if we only do reframing if the nodes to be inserted containing <summary>, because if we insert nodes without any <summary> at the place before <summary>, the summary frame won't be the first child ...

Anyway, I'll just move the logic to reframe <details> into WipeContainingBlock without introducing behavior changes.

Comment 6

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

David, review ping for this bug and bug 1547391. Thanks.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/10faf756124c
Move the reframing logic of <details> element into WipeContainingBlock. r=dbaron

Comment 8

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/10faf756124c

Comment 9

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/17788 for changes under testing/web-platform/tests

Comment 10

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged