Bug 220946 - [css-flexbox] REGRESSION(r266695): content inside a `<button>` inside a flex container has a height of `0` without a declared `min-height`
Summary: [css-flexbox] REGRESSION(r266695): content inside a `<button>` inside a flex ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords: InRadar
Depends on: 210089
Blocks:
  Show dependency treegraph
 
Reported: 2021-01-25 13:59 PST by Devin Rousso
Modified: 2021-01-29 02:01 PST (History)
13 users (show)

See Also:


Attachments
[HTML] reduction (285 bytes, text/html)
2021-01-25 13:59 PST, Devin Rousso
no flags Details
Rendering in different engines (20.99 KB, image/png)
2021-01-26 02:12 PST, Sergio Villar Senin
no flags Details
Patch (5.56 KB, patch)
2021-01-28 01:46 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (7.40 KB, patch)
2021-01-28 03:46 PST, Sergio Villar Senin
zalan: review+
zalan: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2021-01-25 13:59:25 PST
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.
Comment 1 Devin Rousso 2021-01-25 14:00:32 PST
<rdar://problem/73011184>
Comment 2 Sergio Villar Senin 2021-01-26 01:55:38 PST
Checking
Comment 3 Sergio Villar Senin 2021-01-26 02:12:18 PST
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.
Comment 4 Sergio Villar Senin 2021-01-26 02:37:39 PST
(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.
Comment 5 Devin Rousso 2021-01-26 10:04:30 PST
(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>`).
Comment 6 Sergio Villar Senin 2021-01-28 01:46:46 PST
Created attachment 418631 [details]
Patch
Comment 7 Sergio Villar Senin 2021-01-28 01:47:59 PST
Zalan mind taking a look? You did the optimization so you'd likely be the best reviewer for this.
Comment 8 Sergio Villar Senin 2021-01-28 03:46:37 PST
Created attachment 418634 [details]
Patch
Comment 9 zalan 2021-01-28 19:35:13 PST
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.
Comment 10 Sergio Villar Senin 2021-01-29 01:42:25 PST
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.
Comment 11 Sergio Villar Senin 2021-01-29 02:01:36 PST
Committed r272054: <https://trac.webkit.org/changeset/272054>