Bug 220946 – [css-flexbox] REGRESSION(r266695): content inside a `<button>` inside a flex container has a height of `0` without a declared `min-height`

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Devin Rousso]

Created attachment 418333 [details]
[HTML] reduction

Inspecting the attached `reduction.html`, the Computed details sidebar panel shows a height of `0` for the child of the `<button>`.  After adding `min-height: 0;` to the parent of the `<button>` (which is the child of the `display: flex;`), however, the height then shows `50%` (`100px`) and the `background-color: red;` becomes visible.

[Devin Rousso]

<rdar://problem/73011184>

[Sergio Villar Senin]

Checking

[Sergio Villar Senin]

Created attachment 418383 [details]
Rendering in different engines

JFTR I'm attaching the rendering in 3 different engines, WebKit (pre-r266695), Chromium and Firefox. As you can see it's different in all of them, so it's unclear what's the right one.

Anyway I'll get back the old behaviour for buttons.

[Sergio Villar Senin]

(In reply to Sergio Villar Senin from comment #3)
> Created attachment 418383 [details]
> Rendering in different engines
> 
> JFTR I'm attaching the rendering in 3 different engines, WebKit
> (pre-r266695), Chromium and Firefox. As you can see it's different in all of
> them, so it's unclear what's the right one.

Ah the example contains a button, which has different UA styles in every single engine. Nevermind.

[Devin Rousso]

(In reply to Sergio Villar Senin from comment #4)
> (In reply to Sergio Villar Senin from comment #3)
> > Created attachment 418383 [details]
> > Rendering in different engines
> > 
> > JFTR I'm attaching the rendering in 3 different engines, WebKit (pre-r266695), Chromium and Firefox. As you can see it's different in all of them, so it's unclear what's the right one.
> 
> Ah the example contains a button, which has different UA styles in every  single engine. Nevermind.

Sorry I should've clarified.  The issue is that the `background-color: red;` isn't shown after r266695 (unless you add a `min-height: 0;` to the parent of `<button>`).

[Sergio Villar Senin]

Created attachment 418631 [details]
Patch

[Sergio Villar Senin]

Zalan mind taking a look? You did the optimization so you'd likely be the best reviewer for this.

[Sergio Villar Senin]

Created attachment 418634 [details]
Patch

[alan]

Comment on attachment 418634 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        FlexibleBoxImpl's like RenderButton wrap their DOM children in anonymous blocks. Those anonymous blocks are

Is it really always the case? -and what exactly do you mean by DOM children. This is the render tree, we don't have DOM objects here.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1639
> +        auto* descendants = percentHeightDescendants();

I'd prefer writing it out like this
auto& descendants = *percentHeightDescendants();
so that if "descendants" is later used in this function no one starts null checking it.

[Sergio Villar Senin]

Comment on attachment 418634 [details]
Patch

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

>> Source/WebCore/ChangeLog:14
>> +        FlexibleBoxImpl's like RenderButton wrap their DOM children in anonymous blocks. Those anonymous blocks are
> 
> Is it really always the case? -and what exactly do you mean by DOM children. This is the render tree, we don't have DOM objects here.

It's badly explained indeed. I wanted to say that <button> is a FlexibleBoxImpl which wraps the <button> children in anonymous blocks. I should not have mixed the render objects with the dom ones in the same explanation.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1639
>> +        auto* descendants = percentHeightDescendants();
> 
> I'd prefer writing it out like this
> auto& descendants = *percentHeightDescendants();
> so that if "descendants" is later used in this function no one starts null checking it.

OK.

[Sergio Villar Senin]

Committed r272054: <https://trac.webkit.org/changeset/272054>