Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-grid][masonry] Containing block for items in a masonry columns grid should be the grid's content box logical width #13720

Conversation

sammygill
Copy link
Contributor

@sammygill sammygill commented May 10, 2023

ff409be

[css-grid][masonry] Containing block for items in a masonry columns grid should be the grid's content box logical width
https://bugs.webkit.org/show_bug.cgi?id=256611
rdar://109170495

Reviewed by Matt Woodrow.

The masonry spec states, "The containing block for a grid item is
formed by its grid area in the grid axis and the grid container’s
content-box in the masonry axis."

This means that for a grid that has masonry columns specified, the
masonry axis will be in the logical width direction of the grid. The
items should have their containing block set to the content box logical
box of the grid.

We can modify GridTrackSizingAlgorithm::gridAreaBreadthForChild to
return this value when the grid is a masonry columns grid since normally
the grid area would be used as the containing block in non-masonry
grids. This allows the rest of the code to use the grid area sizes of
the containing block in both masonry and non-masonry scenarios.
Previously, this function would have attempted to compute the
value by iterating over the tracks in the specified direction, but there
are no tracks in the masonry direction so we use the logic specified by
the masonry spec instead.

However, in order for this change to work properly we had to make a
change in RenderGrid::layoutMasonry by removing code that was
incorrectly overriding the logical width of the grid. The previous code
was attempting to set the logical width of the grid to the masonry
content size when the grid had masonry columns specified and an auto
logical width. There were 2 main issues with this piece of code:
1.) m_masonryLayout.gridContentSize() will always return 0 since we
actually haven't performed masonry layout at this point
2.) The grid shouldn't be overriding its logical width like this anyways
and it should instead be set by sized by the rules of the formatting
context it is participating in (e.g. block or inline layout).

By removing this code we can get the actual width of the grid later on
when we call m_renderGrid->contentLogicalWidth() rather than the
incorrect 0 value that it was being set to.

The following example highlights the changes that were made.
<style>
grid {
    display: grid;
    grid-template-columns:masonry;
    grid-template-rows: auto;
}
</style>
<grid>
    <svg width="100" height="100" viewBox="0 0 1 1" style="width: 100%; max-width: 100px; background: green;"></svg>
</grid>

By removing the extra code in RenderGrid::layoutMasonry, the grid will
get sizes as a block level box in the block formatting context it is
participating in, giving it a logical width that takes up its available
space. The svg's containing block logical width is set to its value so
it is able to resolve its percentage width to the correct value whereas
before the containing block logical width would have been 0px.

https://drafts.csswg.org/css-grid-3/#containing-block

* LayoutTests/TestExpectations:
* LayoutTests/imported/w3c/web-platform-tests/css/css-grid/masonry/tentative/masonry-columns-item-containing-block-is-grid-content-width-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-grid/masonry/tentative/masonry-columns-item-containing-block-is-grid-content-width.html: Added.
* LayoutTests/platform/glib/TestExpectations:
* Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:
(WebCore::GridTrackSizingAlgorithm::gridAreaBreadthForChild const):
* Source/WebCore/rendering/GridTrackSizingAlgorithm.h:
* Source/WebCore/rendering/RenderGrid.cpp:
(WebCore::RenderGrid::layoutMasonry):

Canonical link: https://commits.webkit.org/264011@main

0a7a8f8

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@sammygill sammygill self-assigned this May 10, 2023
@sammygill sammygill added the Layout and Rendering For bugs with layout and rendering of Web pages. label May 10, 2023
@sammygill sammygill force-pushed the eng/css-gridmasonry-Images-with-percentage-width-in-masonry-columns-grid-not-rendering branch from 2e6fb8f to 01f0bec Compare May 10, 2023 22:21
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 11, 2023
Copy link
Member

@stwrt stwrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still reviewing this. Just noticed this small thing so far.

@@ -637,6 +637,8 @@ std::optional<LayoutUnit> GridTrackSizingAlgorithm::estimatedGridAreaBreadthForC

std::optional<LayoutUnit> GridTrackSizingAlgorithm::gridAreaBreadthForChild(const RenderBox& child, GridTrackSizingDirection direction) const
{
if (m_renderGrid->areMasonryColumns())
return m_renderGrid->contentLogicalWidth();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add new line here.

@sammygill sammygill removed the merging-blocked Applied to prevent a change from being merged label May 11, 2023
@sammygill sammygill force-pushed the eng/css-gridmasonry-Images-with-percentage-width-in-masonry-columns-grid-not-rendering branch from 01f0bec to 0a7a8f8 Compare May 11, 2023 23:17
@sammygill sammygill added the merge-queue Applied to send a pull request to merge-queue label May 12, 2023
…rid should be the grid's content box logical width

https://bugs.webkit.org/show_bug.cgi?id=256611
rdar://109170495

Reviewed by Matt Woodrow.

The masonry spec states, "The containing block for a grid item is
formed by its grid area in the grid axis and the grid container’s
content-box in the masonry axis."

This means that for a grid that has masonry columns specified, the
masonry axis will be in the logical width direction of the grid. The
items should have their containing block set to the content box logical
box of the grid.

We can modify GridTrackSizingAlgorithm::gridAreaBreadthForChild to
return this value when the grid is a masonry columns grid since normally
the grid area would be used as the containing block in non-masonry
grids. This allows the rest of the code to use the grid area sizes of
the containing block in both masonry and non-masonry scenarios.
Previously, this function would have attempted to compute the
value by iterating over the tracks in the specified direction, but there
are no tracks in the masonry direction so we use the logic specified by
the masonry spec instead.

However, in order for this change to work properly we had to make a
change in RenderGrid::layoutMasonry by removing code that was
incorrectly overriding the logical width of the grid. The previous code
was attempting to set the logical width of the grid to the masonry
content size when the grid had masonry columns specified and an auto
logical width. There were 2 main issues with this piece of code:
1.) m_masonryLayout.gridContentSize() will always return 0 since we
actually haven't performed masonry layout at this point
2.) The grid shouldn't be overriding its logical width like this anyways
and it should instead be set by sized by the rules of the formatting
context it is participating in (e.g. block or inline layout).

By removing this code we can get the actual width of the grid later on
when we call m_renderGrid->contentLogicalWidth() rather than the
incorrect 0 value that it was being set to.

The following example highlights the changes that were made.
<style>
grid {
    display: grid;
    grid-template-columns:masonry;
    grid-template-rows: auto;
}
</style>
<grid>
    <svg width="100" height="100" viewBox="0 0 1 1" style="width: 100%; max-width: 100px; background: green;"></svg>
</grid>

By removing the extra code in RenderGrid::layoutMasonry, the grid will
get sizes as a block level box in the block formatting context it is
participating in, giving it a logical width that takes up its available
space. The svg's containing block logical width is set to its value so
it is able to resolve its percentage width to the correct value whereas
before the containing block logical width would have been 0px.

https://drafts.csswg.org/css-grid-3/#containing-block

* LayoutTests/TestExpectations:
* LayoutTests/imported/w3c/web-platform-tests/css/css-grid/masonry/tentative/masonry-columns-item-containing-block-is-grid-content-width-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-grid/masonry/tentative/masonry-columns-item-containing-block-is-grid-content-width.html: Added.
* LayoutTests/platform/glib/TestExpectations:
* Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:
(WebCore::GridTrackSizingAlgorithm::gridAreaBreadthForChild const):
* Source/WebCore/rendering/GridTrackSizingAlgorithm.h:
* Source/WebCore/rendering/RenderGrid.cpp:
(WebCore::RenderGrid::layoutMasonry):

Canonical link: https://commits.webkit.org/264011@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/css-gridmasonry-Images-with-percentage-width-in-masonry-columns-grid-not-rendering branch from 0a7a8f8 to ff409be Compare May 12, 2023 14:41
@webkit-commit-queue
Copy link
Collaborator

Committed 264011@main (ff409be): https://commits.webkit.org/264011@main

Reviewed commits have been landed. Closing PR #13720 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit ff409be into WebKit:main May 12, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 12, 2023
@sammygill sammygill deleted the eng/css-gridmasonry-Images-with-percentage-width-in-masonry-columns-grid-not-rendering branch April 19, 2024 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Layout and Rendering For bugs with layout and rendering of Web pages.
Projects
None yet
6 participants