Skip to content

Conversation

sammygill
Copy link
Contributor

@sammygill sammygill commented Mar 20, 2023

c69be37

Trimmed inline-start margins for flex items in horizontal writing mode should be reflected in computed style.
https://bugs.webkit.org/show_bug.cgi?id=253714
rdar://106559056

Reviewed by Alan Baradlay.

Currently, we trim the inline-start margin of a flex item when we call
flexItem.box.setMarginStart inside either RenderFlexibleBox::trimCrossAxisMarginStart
or RenderFlexibleBox::trimMainAxisMarginStart. Instead, we can replace
these calls with setTrimmedMarginForChild with an argument of
MarginTrimType::InlineStart to both trim the margin and set the
appropriate bit in the rare data object. In horizontal writing-mode
this bit should correspond to margin-left. Once layout has finished,
this bit can be used to determine if the box has a trimmed inline-start
margin.

When ComputedStyleExtractor tries to obtain the "margin-left," value, it
will first need to check if this margin has been trimmed. This can be
done by calling box->hasTrimedMargin(PhysicalDirection::Left).
hasTrimmedMargin will first map this PhysicalDirection to a MarginTrimType
and then check if the appropriate rare data bit has been set. If this
returns true, then the box should have a trimmed inline-start (left)
margin and ComputedStyleExtractor should consult the box's m_marginBox
to get the trimmed value.

* LayoutTests/imported/w3c/web-platform-tests/css/css-box/margin-trim/computed-margin-values/flexbox-column-inline-start-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-box/margin-trim/computed-margin-values/flexbox-column-inline-start.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-box/margin-trim/computed-margin-values/flexbox-column-multi-line-inline-start-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-box/margin-trim/computed-margin-values/flexbox-column-multi-line-inline-start.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-box/margin-trim/computed-margin-values/flexbox-row-inline-start-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-box/margin-trim/computed-margin-values/flexbox-row-inline-start.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-box/margin-trim/computed-margin-values/flexbox-row-multi-line-inline-start-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-box/margin-trim/computed-margin-values/flexbox-row-multi-line-inline-start.html: Added.
* Source/WebCore/css/ComputedStyleExtractor.cpp:
(WebCore::rendererContainingBlockHasMarginTrim):
(WebCore::isLayoutDependent):
(WebCore::ComputedStyleExtractor::valueForPropertyInStyle):
* Source/WebCore/rendering/RenderBlock.cpp:
(WebCore::RenderBlock::setTrimmedMarginForChild):
* Source/WebCore/rendering/RenderBox.cpp:
(WebCore::RenderBox::physicalToFlowRelativeDirectionMapping const):
(WebCore::RenderBox::hasTrimmedMargin const):
* Source/WebCore/rendering/RenderFlexibleBox.cpp:
(WebCore::RenderFlexibleBox::trimMainAxisMarginStart):
(WebCore::RenderFlexibleBox::trimCrossAxisMarginStart):

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

6e98230

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
✅ 🛠 tv ✅ 🧪 api-ios ✅ 🧪 api-gtk
✅ 🧪 mac-wk2 ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@sammygill sammygill self-assigned this Mar 20, 2023
@sammygill sammygill added the CSS Cascading Style Sheets implementation label Mar 20, 2023
@sammygill sammygill changed the title Trimmed Trimmed inline-start margins for flex items in horizontal writing mode should be reflected in computed style. Mar 20, 2023
@sammygill sammygill requested a review from alanbaradlay March 20, 2023 19:24
@sammygill sammygill force-pushed the eng/margin-trim-Trimmed-inline-start-margins-for-flex-items-should-be-reflected-in-computed-style branch from f19ab3e to 6a7346d Compare March 20, 2023 23:13
Copy link
Member

@shivamidow shivamidow left a comment

Choose a reason for hiding this comment

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

Duplicated with #11741?

@sammygill sammygill force-pushed the eng/margin-trim-Trimmed-inline-start-margins-for-flex-items-should-be-reflected-in-computed-style branch from 6a7346d to c1ef8a1 Compare March 29, 2023 23:45
@sammygill
Copy link
Contributor Author

Duplicated with #11741?

Not exactly, this patch contains similar logic but focuses on the inline-start margins specifically. I think I addressed the things you mentioned in #11741 here as well, but let me know if I missed anything!

@sammygill sammygill force-pushed the eng/margin-trim-Trimmed-inline-start-margins-for-flex-items-should-be-reflected-in-computed-style branch from c1ef8a1 to 6e98230 Compare April 7, 2023 06:06
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 7, 2023
@sammygill sammygill added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Apr 7, 2023
…e should be reflected in computed style.

https://bugs.webkit.org/show_bug.cgi?id=253714
rdar://106559056

Reviewed by Alan Baradlay.

Currently, we trim the inline-start margin of a flex item when we call
flexItem.box.setMarginStart inside either RenderFlexibleBox::trimCrossAxisMarginStart
or RenderFlexibleBox::trimMainAxisMarginStart. Instead, we can replace
these calls with setTrimmedMarginForChild with an argument of
MarginTrimType::InlineStart to both trim the margin and set the
appropriate bit in the rare data object. In horizontal writing-mode
this bit should correspond to margin-left. Once layout has finished,
this bit can be used to determine if the box has a trimmed inline-start
margin.

When ComputedStyleExtractor tries to obtain the "margin-left," value, it
will first need to check if this margin has been trimmed. This can be
done by calling box->hasTrimedMargin(PhysicalDirection::Left).
hasTrimmedMargin will first map this PhysicalDirection to a MarginTrimType
and then check if the appropriate rare data bit has been set. If this
returns true, then the box should have a trimmed inline-start (left)
margin and ComputedStyleExtractor should consult the box's m_marginBox
to get the trimmed value.

* LayoutTests/imported/w3c/web-platform-tests/css/css-box/margin-trim/computed-margin-values/flexbox-column-inline-start-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-box/margin-trim/computed-margin-values/flexbox-column-inline-start.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-box/margin-trim/computed-margin-values/flexbox-column-multi-line-inline-start-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-box/margin-trim/computed-margin-values/flexbox-column-multi-line-inline-start.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-box/margin-trim/computed-margin-values/flexbox-row-inline-start-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-box/margin-trim/computed-margin-values/flexbox-row-inline-start.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-box/margin-trim/computed-margin-values/flexbox-row-multi-line-inline-start-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-box/margin-trim/computed-margin-values/flexbox-row-multi-line-inline-start.html: Added.
* Source/WebCore/css/ComputedStyleExtractor.cpp:
(WebCore::rendererContainingBlockHasMarginTrim):
(WebCore::isLayoutDependent):
(WebCore::ComputedStyleExtractor::valueForPropertyInStyle):
* Source/WebCore/rendering/RenderBlock.cpp:
(WebCore::RenderBlock::setTrimmedMarginForChild):
* Source/WebCore/rendering/RenderBox.cpp:
(WebCore::RenderBox::physicalToFlowRelativeDirectionMapping const):
(WebCore::RenderBox::hasTrimmedMargin const):
* Source/WebCore/rendering/RenderFlexibleBox.cpp:
(WebCore::RenderFlexibleBox::trimMainAxisMarginStart):
(WebCore::RenderFlexibleBox::trimCrossAxisMarginStart):

Canonical link: https://commits.webkit.org/262708@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/margin-trim-Trimmed-inline-start-margins-for-flex-items-should-be-reflected-in-computed-style branch from 6e98230 to c69be37 Compare April 7, 2023 15:18
@webkit-commit-queue
Copy link
Collaborator

Committed 262708@main (c69be37): https://commits.webkit.org/262708@main

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

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Apr 7, 2023
@webkit-commit-queue webkit-commit-queue merged commit c69be37 into WebKit:main Apr 7, 2023
@sammygill sammygill deleted the eng/margin-trim-Trimmed-inline-start-margins-for-flex-items-should-be-reflected-in-computed-style 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

CSS Cascading Style Sheets implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants