Bug 215369

Summary: [css-flexbox] Only update the intrinsic height if we don't have override height
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: New BugsAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: changseok, clopez, darin, esprehn+autocc, ews-watchlist, glenn, graouts, jfernandez, kondapallykalyan, pdr, svillar, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch jfernandez: review+

Sergio Villar Senin
Reported 2020-08-11 04:43:17 PDT
[css-flexbox] Don't update the intrinsic height if we don't have override height
Attachments
Patch (4.41 KB, patch)
2020-08-11 04:58 PDT, Sergio Villar Senin 		jfernandez: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Sergio Villar Senin
Comment 1 2020-08-11 04:58:10 PDT
Created attachment 406375 [details] Patch
Javier Fernandez
Comment 2 2020-08-11 05:12:55 PDT
Comment on attachment 406375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406375&action=review > Source/WebCore/rendering/RenderBox.cpp:2781 > + if (isFloatingOrOutOfFlowPositioned() || !parent() || !parent()->isFlexibleBox() || hasOverrideContentLogicalHeight()) I think Grid items with stretch alignment will have the same problem with this cache. Wouldn't make more sense to just check for the HasOverrideContentLogicalHeight and remove the clauses about flexible box ?
Sergio Villar Senin
Comment 3 2020-08-11 07:40:37 PDT
*** Bug 210088 has been marked as a duplicate of this bug. ***
Sergio Villar Senin
Comment 4 2020-08-11 07:56:31 PDT
(In reply to Javier Fernandez from comment #2) > Comment on attachment 406375 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=406375&action=review > > > Source/WebCore/rendering/RenderBox.cpp:2781 > > + if (isFloatingOrOutOfFlowPositioned() || !parent() || !parent()->isFlexibleBox() || hasOverrideContentLogicalHeight()) > > I think Grid items with stretch alignment will have the same problem with > this cache. Wouldn't make more sense to just check for the > HasOverrideContentLogicalHeight and remove the clauses about flexible box ? Right, we might eventually need it for grid too. However we cannot remove the checks now beause that triggers an infinite recursion between layoutBlock() <-> relayoutToAvoidWidows() that should be fixed as part of another patch. I could include a FIXME if you want.
Javier Fernandez
Comment 5 2020-08-11 08:03:17 PDT
Comment on attachment 406375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406375&action=review >>> Source/WebCore/rendering/RenderBox.cpp:2781 >>> + if (isFloatingOrOutOfFlowPositioned() || !parent() || !parent()->isFlexibleBox() || hasOverrideContentLogicalHeight()) >> >> I think Grid items with stretch alignment will have the same problem with this cache. Wouldn't make more sense to just check for the HasOverrideContentLogicalHeight and remove the clauses about flexible box ? > > Right, we might eventually need it for grid too. However we cannot remove the checks now beause that triggers an infinite recursion between > > layoutBlock() <-> relayoutToAvoidWidows() > > that should be fixed as part of another patch. I could include a FIXME if you want. Yes, please, add that FIXME.
Javier Fernandez
Comment 6 2020-08-11 08:03:50 PDT
Comment on attachment 406375 [details] Patch r=me
Sergio Villar Senin
Comment 7 2020-08-11 08:22:35 PDT
Committed r265497: <https://trac.webkit.org/changeset/265497>
Radar WebKit Bug Importer
Comment 8 2020-08-11 08:23:19 PDT
<rdar://problem/66840803>
Note You need to log in before you can comment on or make changes to this bug.