Bug 190065 – Enable <summary> to be a flex container

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Paul Irish]

Much like the fieldset flex bug http://wkb.ug/19264, <summary> elements cannot be flex containers.

demo: https://codepen.io/paulirish/pen/OBJXWq

[Jordan]

Any update? Still an issue today...

[Viktor Holmberg]

+1

I agree, fixing this would be really appreciated.

[Nicolas H.]

Same here, this bug only happens on Safari :-\

[Sergio Villar Senin]

Created attachment 427252 [details]
Patch

[Sergio Villar Senin]

I've uploaded also a patch for WPT with the tests from Blink in here https://chromium-review.googlesource.com/c/chromium/src/+/2856537

If they got approved I'd just import them instead of adding these ones to imported/blink

[Sergio Villar Senin]

Created attachment 427262 [details]
Patch

[Sergio Villar Senin]

Created attachment 427335 [details]
Patch

[Antti Koivisto]

Comment on attachment 427335 [details]
Patch

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

> Source/WebCore/rendering/RenderElement.cpp:179
> +        // list-item should have no effect for <summary> as they're already list items. Let it
> +        // fallthrough to the default case where it will create a RenderBlockFlow.
> +        if (creationType == CreateAllRenderers)
> +            return createRenderer<RenderListItem>(element, WTFMove(style));
> +        FALLTHROUGH;

I don't understand this. If you fall through here you are going to create RenderFlexibleBox not RenderBlockFlow.

Also what does "as they're already list items" mean?

[Sergio Villar Senin]

Comment on attachment 427335 [details]
Patch

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

>> Source/WebCore/rendering/RenderElement.cpp:179
>> +        FALLTHROUGH;
> 
> I don't understand this. If you fall through here you are going to create RenderFlexibleBox not RenderBlockFlow.
> 
> Also what does "as they're already list items" mean?

Right that's intended. If we specify "display:list-item" then we want the <summary> to have a RenderFlexibleBox as renderer because we don't want them to be rendered as list items as they already have a marker before the text (we would end up with 2 markers, one for the list item and another for the summary, no other engine does that). The "as they're already list items" is indeed a pretty bad wording for what I mentioned before.

[Sergio Villar Senin]

Created attachment 427345 [details]
Rendering sample

I hope this screenshot could help me explain my point about summary as a list item. Left, current status, we get a double marker one for the list item and another one for the summary. Right the new rendering with the patch, only the marker for the summary is shown.

[Antti Koivisto]

Comment on attachment 427335 [details]
Patch

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

>>> Source/WebCore/rendering/RenderElement.cpp:179
>>> +        FALLTHROUGH;
>> 
>> I don't understand this. If you fall through here you are going to create RenderFlexibleBox not RenderBlockFlow.
>> 
>> Also what does "as they're already list items" mean?
> 
> Right that's intended. If we specify "display:list-item" then we want the <summary> to have a RenderFlexibleBox as renderer because we don't want them to be rendered as list items as they already have a marker before the text (we would end up with 2 markers, one for the list item and another for the summary, no other engine does that). The "as they're already list items" is indeed a pretty bad wording for what I mentioned before.

The part I don't understand is that your code comment says "Let it fallthrough to the default case where it will create a RenderBlockFlow" while it actually creates a RenderFlexibleBox.

[Sergio Villar Senin]

Comment on attachment 427335 [details]
Patch

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

>>>> Source/WebCore/rendering/RenderElement.cpp:179
>>>> +        FALLTHROUGH;
>>> 
>>> I don't understand this. If you fall through here you are going to create RenderFlexibleBox not RenderBlockFlow.
>>> 
>>> Also what does "as they're already list items" mean?
>> 
>> Right that's intended. If we specify "display:list-item" then we want the <summary> to have a RenderFlexibleBox as renderer because we don't want them to be rendered as list items as they already have a marker before the text (we would end up with 2 markers, one for the list item and another for the summary, no other engine does that). The "as they're already list items" is indeed a pretty bad wording for what I mentioned before.
> 
> The part I don't understand is that your code comment says "Let it fallthrough to the default case where it will create a RenderBlockFlow" while it actually creates a RenderFlexibleBox.

Oh yeah, that's wrong. The whole comment is really misleading, sorry about that. Yeah the idea is that in the case of a <summary> element with display:list-item we'll create a flexbox instead of a RenderListItem. I'll revamp it

[Sergio Villar Senin]

Created attachment 430192 [details]
Patch

[Sergio Villar Senin]

Committed r278280 (238317@main): <https://commits.webkit.org/238317@main>

[Radar WebKit Bug Importer]

<rdar://problem/78689690>