Bug 190065

Summary: Enable <summary> to be a flex container
Product: WebKit Reporter: Paul Irish <paulirish>
Component: Layout and RenderingAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, bugs.webkit.org, cdumez, changseok, clopez, dante3333, darin, esprehn+autocc, ews-watchlist, ggaren, glenn, gyuyoung.kim, jordanbrennan, karolin.gjothlen, koivisto, kondapallykalyan, mcatanzaro, m.kurz+webkitbugs, pdr, rbuis, rniwa, simon.fraser, svillar, viktor.holmberg, webkit-bug-importer, wenson_hsieh, youennf, ysuzuki, zalan
Priority: P2 Keywords: FromImplementor, InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 226859    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Rendering sample
none
Patch koivisto: review+

Description Paul Irish 2018-09-27 17:58:17 PDT
Much like the fieldset flex bug http://wkb.ug/19264, <summary> elements cannot be flex containers.

demo: https://codepen.io/paulirish/pen/OBJXWq
Comment 1 Jordan 2021-02-04 12:05:22 PST
Any update? Still an issue today...
Comment 2 Viktor Holmberg 2021-02-11 04:37:18 PST
+1
Comment 3 karolin.gjothlen 2021-03-16 09:54:08 PDT
I agree, fixing this would be really appreciated.
Comment 4 Nicolas H. 2021-04-27 05:54:04 PDT
Same here, this bug only happens on Safari :-\
Comment 5 Sergio Villar Senin 2021-04-28 04:06:21 PDT
Created attachment 427252 [details]
Patch
Comment 6 Sergio Villar Senin 2021-04-28 04:58:45 PDT
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
Comment 7 Sergio Villar Senin 2021-04-28 07:10:07 PDT
Created attachment 427262 [details]
Patch
Comment 8 Sergio Villar Senin 2021-04-29 03:07:56 PDT
Created attachment 427335 [details]
Patch
Comment 9 Antti Koivisto 2021-04-29 06:50:48 PDT
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?
Comment 10 Sergio Villar Senin 2021-04-29 07:52:38 PDT
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.
Comment 11 Sergio Villar Senin 2021-04-29 07:55:01 PDT
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.
Comment 12 Antti Koivisto 2021-05-27 08:17:55 PDT
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.
Comment 13 Sergio Villar Senin 2021-05-28 04:56:21 PDT
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
Comment 14 Sergio Villar Senin 2021-05-31 03:03:25 PDT
Created attachment 430192 [details]
Patch
Comment 15 Sergio Villar Senin 2021-05-31 09:18:15 PDT
Committed r278280 (238317@main): <https://commits.webkit.org/238317@main>
Comment 16 Radar WebKit Bug Importer 2021-05-31 09:19:16 PDT
<rdar://problem/78689690>