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
Implement align-content on block containers #21522
Conversation
f2375d1
to
77f159f
Compare
Source/WebCore/layout/integration/inline/LayoutIntegrationLineLayout.cpp
Outdated
Show resolved
Hide resolved
Source/WebCore/layout/integration/inline/LayoutIntegrationLineLayout.cpp
Outdated
Show resolved
Hide resolved
Source/WebCore/layout/integration/inline/LayoutIntegrationLineLayout.cpp
Show resolved
Hide resolved
for (auto& object : m_boxTree.renderers()) { | ||
Layout::Box& layoutBox = *object->layoutBox(); | ||
if (layoutBox.isOutOfFlowPositioned() && layoutBox.style().hasStaticBlockPosition(isHorizontalWritingMode)) { | ||
RenderBox& renderer = downcast<RenderBox>(m_boxTree.rendererForLayoutBox(layoutBox)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RenderBox& renderer = downcast<RenderBox>(m_boxTree.rendererForLayoutBox(layoutBox)); | |
auto& renderer = downcast<RenderBox>(m_boxTree.rendererForLayoutBox(layoutBox)); |
Is this downcast guaranteed to succeed? Are we guaranteed to get a RenderBox here?
Also this might need smart pointers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super familiar with the layout integration layer myself, but I copied the logic from here: https://searchfox.org/wubkat/source/Source/WebCore/layout/integration/inline/LayoutIntegrationLineLayout.cpp#417
for (auto& renderObject : m_boxTree.renderers()) {
auto& layoutBox = *renderObject->layoutBox();
if (!layoutBox.isFloatingPositioned() && !layoutBox.isOutOfFlowPositioned())
continue;
if (layoutBox.isLineBreakBox())
continue;
auto& renderer = downcast<RenderBox>(m_boxTree.rendererForLayoutBox(layoutBox));
If that's working off of valid assumptions, then my understanding is that this will work, too.
EWS run on previous version of this PR (hash 77f159f)
|
I kinda prefer to not use |
Source/WebCore/layout/integration/inline/LayoutIntegrationLineLayout.cpp
Show resolved
Hide resolved
Source/WebCore/layout/integration/inline/LayoutIntegrationLineLayout.cpp
Outdated
Show resolved
Hide resolved
Source/WebCore/layout/integration/inline/LayoutIntegrationLineLayout.cpp
Outdated
Show resolved
Hide resolved
@@ -197,6 +197,20 @@ AggressiveTileRetentionEnabled: | |||
WebCore: | |||
default: false | |||
|
|||
AlignContentOnBlocksEnabled: | |||
type: bool | |||
status: stable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we really at stable
yet? I would start with testable
or preview
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can always switch it off later if it's a problem, but Greg asked me to land this today if possible, even if it wasn't perfect.
@@ -35,4 +35,57 @@ TextStream& operator<<(TextStream& ts, const StyleContentAlignmentData& o) | |||
return ts << o.position() << " " << o.distribution() << " " << o.overflow(); | |||
} | |||
|
|||
bool StyleContentAlignmentData::isStartward(bool isRTL, bool isFlexReverse) const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like isStartward
, but I'm not sure what this is trying to compute so can't suggest a better name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's representing all the values that would be equivalent to start, so, mapping to start (vs end vs center) per flex-direction / direction / fallback alignment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does something like isEquivalentToContentPositionStart
sound?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well... It's a little odd for e.g. ContentDistribution::SpaceBetween
, for example, or ContentPosition::Baseline
. They're not equivalent to start, but they fall back to it.
905f7ac
to
e5055e9
Compare
ASSERT(renderer->layer()); | ||
CheckedRef layer = *renderer->layer(); | ||
layer->setStaticBlockPosition(layer->staticBlockPosition() + blockShift); | ||
renderer->setChildNeedsLayout(MarkOnlyThis); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what guarantees that we end up running layout on this renderer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing inherent to the line layout object, but my logic goes:
- for everything that we're a containing block for, we're running positioned box layout after the align-content shifting
- for everything that our ancestors are a containing block for, they need to wait until we're done with layout to be able to place static positioned boxes that are our descendants anyway, so they need to be running positioned box layout after we're done here anyway
It's not foolproof logic, and there's possibly something I missed here, but I think that should capture most of it...
LayoutUnit shiftY = (m_horizontalWritingMode) ? blockShift : 0_lu; | ||
|
||
for (auto& floater : m_set) { | ||
floater->m_frameRect.move(shiftX, shiftY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather use public API (which could do both of these)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I didn't go that route was that I didn't want anyone else trying to modify e.g. an individual FloatObject directly, because that could break some invariants in the FloatObjects tracking logic.
Since we're shifting every Float together by the same delta simultaneously, we're not breaking that logic. But if some external caller tried to that, things would likely break. Note that there are setX() and setY() methods on the FloatObject, but I'm specifically bypassing their checks here. I don't want anyone other than this method to be able to do that, though, because this method guarantees they're all moved together.
b80b86b
to
522100b
Compare
EWS run on previous version of this PR (hash 522100b)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few more places we could infer types.
} | ||
|
||
for (auto& object : m_boxTree.renderers()) { | ||
Layout::Box& layoutBox = *object->layoutBox(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Layout::Box& layoutBox = *object->layoutBox(); | |
auto& layoutBox = *object->layoutBox(); |
The variable name is pretty self explanatory -> auto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish it was, but everything here is representing a layout box. In fact, because this is peeling its way through the integration layers, it's all representing the same layout box, they're all just different types representing the same layout box. Tbh, the fact "layoutBox" is referencing the type Layout::Box was totally lost on me when I was reading this code (because everything here uses auto
). It's only self-explanatory when you're an expert in this codebase.
522100b
to
e4d949f
Compare
EWS run on previous version of this PR (hash e4d949f)
|
e4d949f
to
3107db9
Compare
EWS run on previous version of this PR (hash 3107db9)
|
3107db9
to
640c33b
Compare
EWS run on previous version of this PR (hash 640c33b)
|
640c33b
to
a6c133a
Compare
EWS run on current version of this PR (hash a6c133a)
|
https://bugs.webkit.org/show_bug.cgi?id=266085 rdar://114740670 Reviewed by Alan Baradlay. Implements 'align-content' on block containers by shifting the contents of RenderBlockFlow if there is extra space after sizing. See https://www.w3.org/TR/css-align/#distribution-block Alignment is not supported if the box is fragmented. To support this, adds methods for shifting content in the block axis to: - FloatingObjects - legacy RenderLineBoxList - modern LineLayout Also implements the requirement that non-normal 'align-content' values establish a new formatting context. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-001-expected.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-001-ref.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-001.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-002.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-003.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-004.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-005.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-006.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-007.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-008.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-009.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-010.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-011.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-break-content-010-aligned-ref.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-break-content-010-expected.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-break-content-010-unaligned-ref.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-break-content-010.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-break-content-020-aligned-ref.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-break-content-020-expected.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-break-content-020-unaligned-ref.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-break-content-020.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-break-overflow-010-expected.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-break-overflow-010-ref.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-break-overflow-010.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-break-overflow-020-expected.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-break-overflow-020-ref.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-break-overflow-020.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-overflow-000-expected.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-overflow-000-ref.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-overflow-000.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-block-simple-height-change.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-align/blocks/align-content-table-cell.html: Added. * Source/WebCore/layout/integration/inline/LayoutIntegrationLineLayout.cpp: (WebCore::LayoutIntegration::LineLayout::shiftLinesBy): Implement shifting line boxes and their contents. * Source/WebCore/layout/integration/inline/LayoutIntegrationLineLayout.h: Add API to shift line boxes and their contents. * Source/WebCore/rendering/FloatingObjects.cpp: (WebCore::FloatingObjects::shiftFloatsBy): Implement shifting floats while avoiding side-effects. * Source/WebCore/rendering/FloatingObjects.h: Add APIs for shifting all floats by a fixed amount. * Source/WebCore/rendering/RenderBlockFlow.cpp: (WebCore::RenderBlockFlow::layoutBlock): Shift for align-content, trigger abspos reflow if shifted. (WebCore::RenderBlockFlow::shiftForAlignContent): Implement method to shift content per align-content. * Source/WebCore/rendering/RenderBlockFlow.h: Add shiftForAlignContent() * Source/WebCore/rendering/RenderElement.cpp: (WebCore::RenderElement::createsNewFormattingContext const): Establish new formatting context. * Source/WebCore/rendering/RenderLineBoxList.cpp: (WebCore::RenderLineBoxList::shiftLinesBy): Implement method to shift lines by delta. * Source/WebCore/rendering/RenderLineBoxList.h: Add API to shift lines by delta. * Source/WebCore/rendering/style/StyleContentAlignmentData.cpp: (WebCore::StyleContentAlignmentData::isStartward const): Add convenience method for 'start'-equivalent values. (WebCore::StyleContentAlignmentData::isEndward const): Add convenience method for 'end'-equivalent values. (WebCore::StyleContentAlignmentData::isCentered const): Add convenience method for 'center'-equivalent values. * Source/WebCore/rendering/style/StyleContentAlignmentData.h: (WebCore::StyleContentAlignmentData::isNormal const): Add convenience method to correctly check for 'normal'. Canonical link: https://commits.webkit.org/271818@main
a6c133a
to
4c9ed16
Compare
Committed 271818@main (4c9ed16): https://commits.webkit.org/271818@main Reviewed commits have been landed. Closing PR #21522 and removing active labels. |
4c9ed16
a6c133a
π§ͺ mac-wk2