Bug 225590

Summary: [css-flexbox] Do not clamp flex base size with {min|max}-{height|width}
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: New BugsAssignee: Sergio Villar Senin <svillar>
Status: REOPENED ---    
Severity: Normal CC: cdumez, changseok, commit-queue, esprehn+autocc, ews-watchlist, glenn, graouts, kondapallykalyan, pdr, rego, simon.fraser, svillar, webkit-bug-importer, youennf, ysuzuki, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 227764, 232481    
Bug Blocks: 227597, 227878    
Attachments:
Description Flags
Patch
none
Patch
none
Patch rego: review+, ews-feeder: commit-queue-

Description Sergio Villar Senin 2021-05-10 04:34:44 PDT
[css-flexbox] Do not clamp flex base size with {min|max}-{height|width}
Comment 1 Sergio Villar Senin 2021-05-10 04:48:56 PDT
Created attachment 428165 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2021-05-17 04:35:17 PDT
<rdar://problem/78100329>
Comment 3 Sergio Villar Senin 2021-06-14 09:27:53 PDT
Created attachment 431337 [details]
Patch
Comment 4 zalan 2021-06-24 20:29:53 PDT
Comment on attachment 431337 [details]
Patch

:( same as https://bugs.webkit.org/show_bug.cgi?id=226634#c5
Comment 5 Simon Fraser (smfr) 2021-06-24 20:37:35 PDT
Comment on attachment 431337 [details]
Patch

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

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1309
> +// A scoped class that resets either min-width & max-witdh or min-height & max-height (depending on the

witdh

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1314
> +class ScopedUnboundedBox {

Can we share some of this with DisableMinMaxConstrainsScope?
Comment 6 Sergio Villar Senin 2021-06-25 01:06:12 PDT
(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 431337 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=431337&action=review
> 
> > Source/WebCore/rendering/RenderFlexibleBox.cpp:1309
> > +// A scoped class that resets either min-width & max-witdh or min-height & max-height (depending on the
> 
> witdh
> 
> > Source/WebCore/rendering/RenderFlexibleBox.cpp:1314
> > +class ScopedUnboundedBox {
> 
> Can we share some of this with DisableMinMaxConstrainsScope?

No without making them unnecessarily complex because there are some usages of ScopedUnboundedBox that do not imply DisableMinMaxConstrainScope
Comment 7 Sergio Villar Senin 2021-06-25 01:59:23 PDT
Committed r279271 (239150@main): <https://commits.webkit.org/239150@main>
Comment 8 Sergio Villar Senin 2021-06-25 03:00:02 PDT
(In reply to Sergio Villar Senin from comment #6)
> (In reply to Simon Fraser (smfr) from comment #5)
> > Comment on attachment 431337 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=431337&action=review
> > 
> > > Source/WebCore/rendering/RenderFlexibleBox.cpp:1309
> > > +// A scoped class that resets either min-width & max-witdh or min-height & max-height (depending on the
> > 
> > witdh
> > 
> > > Source/WebCore/rendering/RenderFlexibleBox.cpp:1314
> > > +class ScopedUnboundedBox {
> > 
> > Can we share some of this with DisableMinMaxConstrainsScope?
> 
> No without making them unnecessarily complex because there are some usages
> of ScopedUnboundedBox that do not imply DisableMinMaxConstrainScope

Oh wait I thought you're talking about the overriding scope. Now that you mention I think I have messed it up and reported two bugs for the same issue...

I'll mark bug 226634 as duplicate. Sorry for the mess
Comment 9 Sergio Villar Senin 2021-06-25 03:00:35 PDT
*** Bug 226634 has been marked as a duplicate of this bug. ***
Comment 10 WebKit Commit Bot 2021-07-07 12:35:18 PDT
Re-opened since this is blocked by bug 227764
Comment 11 Sergio Villar Senin 2021-10-01 05:17:54 PDT
I've a new patch for this issue that does not suffer from the Gmail bug. It's based on the previous work done in bug 230755 so I need to get that in before posting a new patch here (which BTW fixes 3 tests with no regressions).
Comment 12 Sergio Villar Senin 2021-10-18 03:18:36 PDT
Created attachment 441578 [details]
Patch
Comment 13 Manuel Rego Casasnovas 2021-10-18 04:17:55 PDT
Comment on attachment 441578 [details]
Patch

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

r=me, let's hope there's no more unexpected breakage.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1010
> +        m_originalMin = m_isHorizontalWritingModeInParallelFlow ? m_child.style().minWidth() : m_child.style().minHeight();
> +        m_originalMax = m_isHorizontalWritingModeInParallelFlow ? m_child.style().maxWidth() : m_child.style().maxHeight();

Nit: We've the very same if just below, maybe we can put this inside that if directly.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1048
>      Length m_originalLength;
> +    Length m_originalMin;
> +    Length m_originalMax;

Nit: I've the feeling that m_originalSize, m_originalMinSize and m_originalMaxSize might be easier to understand.
Comment 14 Sergio Villar Senin 2021-10-18 04:24:46 PDT
Comment on attachment 441578 [details]
Patch

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

Thanks for the review!

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1010
>> +        m_originalMax = m_isHorizontalWritingModeInParallelFlow ? m_child.style().maxWidth() : m_child.style().maxHeight();
> 
> Nit: We've the very same if just below, maybe we can put this inside that if directly.

Sure, I developed both patches independently so came up with two different presentations. I'll move them there.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1048
>> +    Length m_originalMax;
> 
> Nit: I've the feeling that m_originalSize, m_originalMinSize and m_originalMaxSize might be easier to understand.

Makes sense. I'll rename before landing
Comment 15 Sergio Villar Senin 2021-10-18 13:51:10 PDT
Committed r284397 (243173@main): <https://commits.webkit.org/243173@main>
Comment 16 Yusuke Suzuki 2021-10-29 13:18:32 PDT
Reverted in https://trac.webkit.org/changeset/285045/webkit
Because the relying change causes 100% CPU usage in Twitter