Bug 225590 – [css-flexbox] Do not clamp flex base size with {min|max}-{height|width}

• Status: REOPENED
• Component: New Bugs

[Sergio Villar Senin]

[css-flexbox] Do not clamp flex base size with {min|max}-{height|width}

[Sergio Villar Senin]

Created attachment 428165 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/78100329>

[Sergio Villar Senin]

Created attachment 431337 [details]
Patch

[zalan]

Comment on attachment 431337 [details]
Patch

:( same as https://bugs.webkit.org/show_bug.cgi?id=226634#c5

[Simon Fraser (smfr)]

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?

[Sergio Villar Senin]

(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

[Sergio Villar Senin]

Committed r279271 (239150@main): <https://commits.webkit.org/239150@main>

[Sergio Villar Senin]

(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

[Sergio Villar Senin]

*** Bug 226634 has been marked as a duplicate of this bug. ***

[WebKit Commit Bot]

Re-opened since this is blocked by bug 227764

[Sergio Villar Senin]

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).

[Sergio Villar Senin]

Created attachment 441578 [details]
Patch

[Manuel Rego Casasnovas]

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.

[Sergio Villar Senin]

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

[Sergio Villar Senin]

Committed r284397 (243173@main): <https://commits.webkit.org/243173@main>

[Yusuke Suzuki]

Reverted in https://trac.webkit.org/changeset/285045/webkit
Because the relying change causes 100% CPU usage in Twitter