Summary: | Implement css property contain-intrinsic-size | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | cathiechen <cathiechen> | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Component: | CSS | Assignee: | cathiechen <cathiechen> | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | bramus, cdumez, changseok, darin, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jer.noble, jfernandez, joepeck, kondapallykalyan, macpherson, menard, mifenton, mmaxfield, pdr, philipj, rbuis, rego, sergio, sgill26, simon.fraser, svillar, webkit-bug-importer, youssefdevelops, y_soliman | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Version: | WebKit Local Build | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
Bug Blocks: | 236707 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
cathiechen
2022-04-06 07:39:05 PDT
Created attachment 456818 [details]
WIP-patch
Created attachment 457691 [details]
WIP-patch-for-ews
Created attachment 457692 [details]
WIP-patch-for-review
Created attachment 457693 [details]
WIP-patch-for-ews
Created attachment 457694 [details]
WIP-patch-for-review
Created attachment 457699 [details]
WIP-patch-for-ews
Created attachment 457700 [details]
WIP-patch-for-review
Created attachment 457701 [details]
WIP-patch-for-ews
Created attachment 457702 [details]
WIP-patch-for-review
Comment on attachment 457702 [details] WIP-patch-for-review View in context: https://bugs.webkit.org/attachment.cgi?id=457702&action=review > Source/WebCore/rendering/style/RenderStyle.cpp:783 > + if (first.containIntrinsicWidth != second.containIntrinsicWidth Maybe we should move this change to the parsing patch? Created attachment 457794 [details]
Patch-for-review
Created attachment 457795 [details]
patch-for-ews-with-parsing-code
Comment on attachment 457794 [details] Patch-for-review Hi, I think this patch is ready for review. Thanks! * The patch-for-review only contains the implementation part, not the parsing and runtime flags code. * The patch-for-ews contains the parsing and runtime flags code, see bug 238181. Created attachment 457797 [details]
Patch-for-review
Created attachment 457799 [details]
patch-for-ews-with-parsing-code
Created attachment 457800 [details]
Patch-for-review
Comment on attachment 457800 [details] Patch-for-review View in context: https://bugs.webkit.org/attachment.cgi?id=457800&action=review > Source/WebCore/rendering/RenderBox.cpp:5520 > + return false; I think it would be nice to minimise shouldApplySizeContainment calls and also maybe turning this into ASSERT. > Source/WebCore/rendering/RenderReplaced.cpp:526 > + // Don't let size containment effect aspect ratio. Maybe we should split this out? Affect. > Source/WebCore/rendering/style/RenderStyle.cpp:779 > + // This change should be slipt out, maybe only size containment needs relayout Split. > Source/WebCore/rendering/style/RenderStyle.cpp:784 > + || first.containIntrinsicHeight != second.containIntrinsicHeight) Can probably go on one line. Comment on attachment 457800 [details] Patch-for-review View in context: https://bugs.webkit.org/attachment.cgi?id=457800&action=review Hi Rob, thanks for the reivew! >> Source/WebCore/rendering/RenderBox.cpp:5520 >> + return false; > > I think it would be nice to minimise shouldApplySizeContainment calls and also maybe turning this into ASSERT. Hmm, there are some scenarios we need to check shouldApplySizeContainment first. How about we add an new interface sizeContainmentHasExplicitIntrinsicInnerLogicalWidth()? >> Source/WebCore/rendering/RenderReplaced.cpp:526 >> + // Don't let size containment effect aspect ratio. Maybe we should split this out? > > Affect. Done >> Source/WebCore/rendering/style/RenderStyle.cpp:779 >> + // This change should be slipt out, maybe only size containment needs relayout > > Split. Done >> Source/WebCore/rendering/style/RenderStyle.cpp:784 >> + || first.containIntrinsicHeight != second.containIntrinsicHeight) > > Can probably go on one line. Done Created attachment 458117 [details]
Patch
Created attachment 458208 [details]
Patch
Created attachment 458252 [details]
Patch
Comment on attachment 458252 [details]
Patch
Hi, I think this patch is ready for review:) Please take a look, thanks!
Comment on attachment 458252 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458252&action=review > Source/WebCore/rendering/RenderBox.cpp:3095 > + LayoutUnit explicitIntrinsicHeight = sizeContainmentHasExplicitIntrinsicInnerLogicalHeight() ? explicitIntrinsicInnerLogicalHeight() : LayoutUnit(0); LayoutUnit(0) -> LayoutUnit(). > Source/WebCore/rendering/RenderBox.cpp:3234 > + estimatedHeight += explicitIntrinsicInnerLogicalHeight() + scrollbarLogicalHeight(); So is the explicit intrinsic height always excluding any scrollbar heights? > Source/WebCore/rendering/RenderListBox.cpp:209 > + maxLogicalWidth = 2 * optionsSpacingHorizontal; In case sizeContainmentHasExplicitIntrinsicInnerLogicalWidth holds true, we do this statement for no good reason, since we know it will be overwritten. > Source/WebCore/rendering/RenderListBox.cpp:214 > + minLogicalWidth = maxLogicalWidth; Is minLogicalWidth undefined in case style().width().isPercentOrCalculated() is true? > Source/WebCore/rendering/RenderSlider.cpp:74 > + minLogicalWidth = maxLogicalWidth = explicitIntrinsicInnerLogicalWidth(); I see some places min = max = .... and some max = min = ... . Better to pick one style. Comment on attachment 458252 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458252&action=review Hi Rob, thanks for the review! >> Source/WebCore/rendering/RenderBox.cpp:3095 >> + LayoutUnit explicitIntrinsicHeight = sizeContainmentHasExplicitIntrinsicInnerLogicalHeight() ? explicitIntrinsicInnerLogicalHeight() : LayoutUnit(0); > > LayoutUnit(0) -> LayoutUnit(). Done >> Source/WebCore/rendering/RenderBox.cpp:3234 >> + estimatedHeight += explicitIntrinsicInnerLogicalHeight() + scrollbarLogicalHeight(); > > So is the explicit intrinsic height always excluding any scrollbar heights? Nope, scrollbar is not included in the explicit intrinsic size. >> Source/WebCore/rendering/RenderListBox.cpp:209 >> + maxLogicalWidth = 2 * optionsSpacingHorizontal; > > In case sizeContainmentHasExplicitIntrinsicInnerLogicalWidth holds true, we do this statement for no good reason, since we know it will be overwritten. Yes, this is outside because it can be reused by `!shouldApplySizeContainment(*this)` and `shouldApplySizeContainment(*this) && !sizeContainmentHasExplicitIntrinsicInnerLogicalWidth()` >> Source/WebCore/rendering/RenderListBox.cpp:214 >> + minLogicalWidth = maxLogicalWidth; > > Is minLogicalWidth undefined in case style().width().isPercentOrCalculated() is true? I guess so. This is copied from the following code, we don't want to change the behavior. Hmm, looks like we can reuse the following code, I didn't because contain-intrinsic-size-009.html fails. It looks like the failure is related to `overflow:visible` doesn't make select's scroll bar disappeared. The value of contain-intrinsic-width shouldn't include scrollbar. I'll try to explain this in TestExpectations. >> Source/WebCore/rendering/RenderSlider.cpp:74 >> + minLogicalWidth = maxLogicalWidth = explicitIntrinsicInnerLogicalWidth(); > > I see some places min = max = .... and some max = min = ... . Better to pick one style. Done Comment on attachment 458252 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458252&action=review >>> Source/WebCore/rendering/RenderListBox.cpp:209 >>> + maxLogicalWidth = 2 * optionsSpacingHorizontal; >> >> In case sizeContainmentHasExplicitIntrinsicInnerLogicalWidth holds true, we do this statement for no good reason, since we know it will be overwritten. > > Yes, this is outside because it can be reused by `!shouldApplySizeContainment(*this)` and `shouldApplySizeContainment(*this) && !sizeContainmentHasExplicitIntrinsicInnerLogicalWidth()` This seems hard to read, I'll rewrite it. Created attachment 458257 [details]
Patch
Comment on attachment 458257 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458257&action=review > Source/WebCore/rendering/RenderBox.h:673 > + LayoutUnit explicitIntrinsicInnerWidth() const; This could return optional, then maybe some hasFoo methods are not needed. > Source/WebCore/rendering/RenderListBox.cpp:213 > + maxLogicalWidth = 2* optionsSpacingHorizontal; 2* -> 2 * Comment on attachment 458257 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458257&action=review > Source/WebCore/rendering/RenderReplaced.h:46 > + LayoutSize size = LayoutSize(); The "= LayoutSize()" here doesn’t do anything and can be omitted. LayoutUnit values are initialized to 0, unlike scalars which can be left uninitialized. > Source/WebCore/rendering/RenderVideo.cpp:117 > + LayoutSize size = LayoutSize(); The "= LayoutSize()" here doesn’t do anything and can be omitted. Comment on attachment 458257 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458257&action=review >> Source/WebCore/rendering/RenderBox.h:673 >> + LayoutUnit explicitIntrinsicInnerWidth() const; > > This could return optional, then maybe some hasFoo methods are not needed. Looks like optional could make the interface much more clear. Done! >> Source/WebCore/rendering/RenderListBox.cpp:213 >> + maxLogicalWidth = 2* optionsSpacingHorizontal; > > 2* -> 2 * Done, thanks! >> Source/WebCore/rendering/RenderReplaced.h:46 >> + LayoutSize size = LayoutSize(); > > The "= LayoutSize()" here doesn’t do anything and can be omitted. LayoutUnit values are initialized to 0, unlike scalars which can be left uninitialized. Done, thanks! >> Source/WebCore/rendering/RenderVideo.cpp:117 >> + LayoutSize size = LayoutSize(); > > The "= LayoutSize()" here doesn’t do anything and can be omitted. Done, thanks! Created attachment 458438 [details]
Patch
Created attachment 458439 [details]
Patch
Created attachment 458440 [details]
Patch
Comment on attachment 458440 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458440&action=review > Source/WebCore/ChangeLog:8 > + This patch add support for contain-intrinsic-size value "none | <length>". The value "auto && <length>" add -> adds. > Source/WebCore/rendering/RenderBox.cpp:5535 > + return std::optional<LayoutUnit>(width.value().value()); This probably does not need the explicit cast. > Source/WebCore/rendering/RenderBox.cpp:5551 > + return std::optional<LayoutUnit>(height.value().value()); Ditto. > Source/WebCore/rendering/RenderGrid.cpp:544 > + return std::optional<LayoutUnit>(); Ditto. Comment on attachment 458440 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458440&action=review >> Source/WebCore/ChangeLog:8 >> + This patch add support for contain-intrinsic-size value "none | <length>". The value "auto && <length>" > > add -> adds. Done >> Source/WebCore/rendering/RenderBox.cpp:5535 >> + return std::optional<LayoutUnit>(width.value().value()); > > This probably does not need the explicit cast. Tried locally, it seems needed, for width.value().value() is a float, not LayoutUnit. >> Source/WebCore/rendering/RenderGrid.cpp:544 >> + return std::optional<LayoutUnit>(); > > Ditto. Done. Created attachment 458448 [details]
Patch
Comment on attachment 458448 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458448&action=review Patch is now good enough for me, maybe nice to use the has_value() though. > Source/WebCore/rendering/RenderGrid.cpp:509 > + LayoutUnit totalGuttersSize = explicitIntrinsicInnerLogicalSize(ForColumns) ? 0_lu : guttersSize(m_grid, ForColumns, 0, numTracks(ForColumns), std::nullopt); Maybe has_value() is clearer here. > Source/WebCore/rendering/RenderGrid.cpp:531 > + LayoutUnit totalGuttersSize = direction == ForColumns && explicitIntrinsicInnerLogicalSize(direction) ? 0_lu : guttersSize(grid, direction, 0, numberOfTracks, std::nullopt); Ditto. Comment on attachment 458448 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458448&action=review >> Source/WebCore/rendering/RenderGrid.cpp:509 >> + LayoutUnit totalGuttersSize = explicitIntrinsicInnerLogicalSize(ForColumns) ? 0_lu : guttersSize(m_grid, ForColumns, 0, numTracks(ForColumns), std::nullopt); > > Maybe has_value() is clearer here. Done, thanks:) >> Source/WebCore/rendering/RenderGrid.cpp:531 >> + LayoutUnit totalGuttersSize = direction == ForColumns && explicitIntrinsicInnerLogicalSize(direction) ? 0_lu : guttersSize(grid, direction, 0, numberOfTracks, std::nullopt); > > Ditto. Done Created attachment 458452 [details]
Patch
I am happy with the patch now, thanks Cathie! Created attachment 459164 [details]
Patch
Rebase the code. Created attachment 459749 [details]
Patch
Pull request: https://github.com/WebKit/WebKit/pull/1373 *** Bug 240888 has been marked as a duplicate of this bug. *** Rebase the code: https://github.com/WebKit/WebKit/pull/1799 Committed 255971@main (7ed83ab2929f): <https://commits.webkit.org/255971@main> Reviewed commits have been landed. Closing PR #1799 and removing active labels. |