Fix and reintroduce initial implementation of CSS Grid Masonry. #6940
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
EWS run on previous version of this PR (hash 0d49e82)
|
webkit-ews-buildbot
added
the
merging-blocked
brentfulgham
approved these changes
Nov 29, 2022
Source/WebCore/rendering/Grid.cpp
Outdated
| @@ -186,6 +186,13 @@ GridSpan Grid::gridItemSpan(const RenderBox& gridItem, GridTrackSizingDirection | |||
| return direction == ForColumns ? area.columns : area.rows; | |||
| } | |||
|
|
|||
| void Grid::setupGridForMasonryLayout() | |||
| { | |||
| // FIXME: See if we can resize grid instead of clearing it here: https://bugs.webkit.org/show_bug.cgi?id=248472 | |||
There was a problem hiding this comment.
Nit:
// FIXME(248472): See if we can resize ... etc.
| if (m_gridAxisDirection == ForColumns) | ||
| child->setOverridingContainingBlockContentLogicalWidth(m_renderGrid.m_trackSizingAlgorithm.estimatedGridAreaBreadthForChild(*child, ForColumns)); | ||
| else | ||
| child->setOverridingContainingBlockContentLogicalHeight(m_renderGrid.m_trackSizingAlgorithm.estimatedGridAreaBreadthForChild(*child, ForRows)); |
There was a problem hiding this comment.
Still curious why this isn't just:
child->setOverridingContainingBlockContentLogicalWidth(m_renderGrid.m_trackSizingAlgorithm.estimatedGridAreaBreadthForChild(*child, m_gridAxisDirection));
There was a problem hiding this comment.
That works if the grid axis is in the column direction because we want to update the width. If it's in the row direction we need to update the height instead. (Or are you talking more about replacing the ForColumns/ForRows with m_gridAxisDirection)
| child->setOverridingContainingBlockContentLogicalHeight(m_renderGrid.m_trackSizingAlgorithm.estimatedGridAreaBreadthForChild(*child, ForRows)); | ||
| } | ||
|
|
||
| void GridMasonryLayout::insertIntoGridAndLayoutItem(RenderBox* child, const GridArea& area) |
There was a problem hiding this comment.
If we never nullptr-check child, could it be passed as a RenderBox&?
| marginBoxSize = child->logicalWidth() + child->marginLogicalWidth(); | ||
| } | ||
| return marginBoxSize; | ||
| } |
There was a problem hiding this comment.
Nit: Missing a newline here.
| LayoutUnit GridMasonryLayout::offsetForChild(const RenderBox& child) const | ||
| { | ||
| if (m_itemOffsets.contains(&child)) | ||
| return m_itemOffsets.get(&child); |
There was a problem hiding this comment.
Nit: This pattern causes two lookups in the hash table (one for 'contains', another for 'get'). If you want to use the result of the lookup, it's better to use 'find':
const auto& offsetIter = m_itemOffsets.find(&child);
if (offsetIter == m_itemOffsets.end())
return 0_lu;
return *offsetIter;
There might be a findOr<> thing that would work, too.
| @@ -1070,7 +1156,6 @@ void RenderGrid::layoutGridItems() | |||
| prepareChildForPositionedLayout(*child); | |||
| continue; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
You shouldn't remove this whitespace.
sammygill
removed
the
merging-blocked
sammygill
force-pushed
the
eng/css-grid-Fix-initial-masonry-implementation
branch
from
0d49e82
to
c1c2507
Compare
|
EWS run on previous version of this PR (hash c1c2507) |
sammygill
force-pushed
the
eng/css-grid-Fix-initial-masonry-implementation
branch
from
c1c2507
to
7f6896c
Compare
|
EWS run on current version of this PR (hash 7f6896c) |
https://bugs.webkit.org/show_bug.cgi?id=248475 rdar://102772770 Reviewed by Brent Fulgham. This patch reintroduces the initial implementation of CSS Grid Masonry after the original one was incorrectly merged in. This addresses the build issue that was occurring along with some feedback addressed in the original PR. Below is the original description from the original patch. CSS Grid Module Level 3 defines a new layout mode for grid containers called Masonry Layout. Masonry layout is triggered by setting either grid-template-rows or grid-template-columns to “masonry.” The axis that is set to “masonry,” will be called the masonry axis and the other axis will be known as the grid axis. The algorithm will go through each item that needs a position and choose the grid axis track that has the most available room. The specification does not provide any strict requirements for the masonry axis track (e.g. how many there should be or when they should be created), so the initial implementation has only one masonry axis track. Masonry layout is performed after an initial layout of the grid. The initial layout will be done as if the masonry axis had “none,” set as its value. The initial setup for masonry layout is done in the end of RenderGrid::placeItemsOnGrid after the initial layout has completed. This gives us the opportunity to collect items into a HashMap or an appropriate Vector that will be used in a particular part of the layout algorithm. The HashMap will contain grid items that were placed on the first implicit line in the masonry axis and their GridAreas so that they can be easily placed in the original positions during the first step of the algorithm. The number of grid axis tracks that are available for this initial placement should be equal to the number of them before auto placement of grid items occurs in the original item placement algorithm. The two Vectors are used to store the items which have a definite grid axis track placement and items with an indefinite placement, since those will need to be handled separately. Once the grid items have been collected, control is given over to the GridMasonryLayout class that holds all of the state and information for masonry layout to actually position the items into the appropriate areas. GridMasonryLayout will hold information such as the running positions of each of the tracks to reference during the positioning algorithm. These running positions will be held inside of a Vector that will be referenced using the corresponding starting line index of the track. Each item will also have a corresponding offset that is stored in a HashMap to be used to position at the correct location in its track. * LayoutTests/TestExpectations: * Source/WebCore/Sources.txt: * Source/WebCore/WebCore.xcodeproj/project.pbxproj: * Source/WebCore/rendering/Grid.cpp: (WebCore::Grid::setupGridForMasonryLayout): * Source/WebCore/rendering/Grid.h: * Source/WebCore/rendering/GridMasonryLayout.cpp: Added. (WebCore::GridMasonryLayout::performMasonryPlacement): (WebCore::GridMasonryLayout::resizeAndResetRunningPositions): (WebCore::GridMasonryLayout::addItemsToFirstTrack): (WebCore::GridMasonryLayout::placeItemsWithDefiniteGridAxisPosition): (WebCore::GridMasonryLayout::placeItemsWithIndefiniteGridAxisPosition): (WebCore::GridMasonryLayout::setItemGridAxisContainingBlockToGridArea): (WebCore::GridMasonryLayout::insertIntoGridAndLayoutItem): (WebCore::GridMasonryLayout::masonryAxisMarginBoxForItem): (WebCore::GridMasonryLayout::updateRunningPositions): (WebCore::GridMasonryLayout::updateItemOffset): (WebCore::GridMasonryLayout::nextMasonryPositionForItem): (WebCore::GridMasonryLayout::offsetForChild const): * Source/WebCore/rendering/GridMasonryLayout.h: Added. (WebCore::GridMasonryLayout::GridMasonryLayout): (WebCore::GridMasonryLayout::gridContentSize const): * Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp: (WebCore::GridTrackSizingAlgorithm::computeTrackBasedSize const): * Source/WebCore/rendering/RenderGrid.cpp: (WebCore::RenderGrid::RenderGrid): (WebCore::isMasonryRows): (WebCore::isMasonryColumns): (WebCore::RenderGrid::placeItemsOnGrid): (WebCore::RenderGrid::allocateSpaceForMasonryVectors): (WebCore::RenderGrid::masonryContentSize const): (WebCore::itemGridAreaStartsAtFirstLine): (WebCore::RenderGrid::hasDefiniteGridAxisPosition const): (WebCore::itemGridAreaIsWithinImplicitGrid): (WebCore::RenderGrid::collectMasonryItems const): (WebCore::RenderGrid::layoutGridItems): (WebCore::RenderGrid::gridAreaBreadthForChildIncludingAlignmentOffsets const): (WebCore::RenderGrid::columnAxisOffsetForChild const): (WebCore::RenderGrid::rowAxisOffsetForChild const): * Source/WebCore/rendering/RenderGrid.h: * Source/WebCore/rendering/style/GridArea.h: (WebCore::GridSpan::masonryAxisTranslatedDefiniteGridSpan): (WebCore::GridSpan::GridSpan): * Source/WebCore/rendering/style/GridPositionsResolver.h: (WebCore::GridPositionsResolver::gridAxisDirection): * Source/WebCore/rendering/style/StyleGridData.cpp: (WebCore::StyleGridData::setColumns): Canonical link: https://commits.webkit.org/257185@main
webkit-early-warning-system
force-pushed
the
eng/css-grid-Fix-initial-masonry-implementation
branch
from
7f6896c
to
aeadced
Compare
|
Committed 257185@main (aeadced): https://commits.webkit.org/257185@main Reviewed commits have been landed. Closing PR #6940 and removing active labels. |
webkit-commit-queue
removed
the
merge-queue
sammygill
deleted the
eng/css-grid-Fix-initial-masonry-implementation
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
aeadced
7f6896c
🧪 api-gtk🧪 mac-wk1🧪 mac-AS-debug-wk2