[IFC][Integration][Line clamp] `LineClamp.currentLineCount` should be propagated to the parent layoutState. #12973

GetToSet · 2023-04-20T15:35:39Z

`59088ba`

[IFC][Integration][Line clamp] `LineClamp.currentLineCount` should be propagated to the parent layoutState.
https://bugs.webkit.org/show_bug.cgi?id=255487

Reviewed by Alan Baradlay.

Since multiple children share the same parent context for line clamp,
`LineClamp.currentLineCount` set to the individual child should be propagated to
their parent layoutState. Otherwise, subsequent children may not see the current
state of this value during layout, causing incorrect line clamp results.

* LayoutTests/fast/block/line-clamp-nested-blocks-with-layout-state-expected.html: Added.
* LayoutTests/fast/block/line-clamp-nested-blocks-with-layout-state.html: Added.
* Source/WebCore/page/LocalFrameViewLayoutContext.cpp:
(WebCore::LocalFrameViewLayoutContext::popLayoutState):

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

4865a76

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

webkit-early-warning-system · 2023-04-20T15:38:59Z

EWS run on previous version of this PR (hash 8f4a9f4)

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

webkit-early-warning-system · 2023-04-20T15:50:17Z

EWS run on previous version of this PR (hash 193e4c2)

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~~
⏳ 🧪 webkitpy	~~🧪 ios-wk2-wpt~~	~~🧪 mac-wk1~~	~~🧪 gtk-wk2~~
⏳ 🛠 🧪 jsc	~~🧪 api-ios~~	~~🧪 mac-wk2~~	~~🧪 api-gtk~~
	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~
⏳ 🧪 services	~~🛠 tv-sim~~	~~🧪 mac-wk2-stress~~
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-04-20T15:54:01Z

EWS run on previous version of this PR (hash 4483e29)

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
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-04-22T04:06:01Z

EWS run on previous version of this PR (hash 209cde3)

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
	⏳ 🛠 watch
	✅ 🛠 watch-sim

GetToSet · 2023-04-22T04:09:06Z

Rebased and updated this patch to accommodate to the latest changes.

alanbaradlay

Now we are diverging from legacy -webkit-line-clamp rendering, but I think it's a good change.

LayoutTests/fast/block/line-clamp-nested-blocks-with-layout-state-expected.html

LayoutTests/fast/block/line-clamp-nested-blocks-with-layout-state.html

Source/WebCore/page/LocalFrameViewLayoutContext.cpp

alanbaradlay · 2023-04-23T14:39:10Z

Source/WebCore/page/LocalFrameViewLayoutContext.cpp

+                currentLineClamp->currentLineCount,
+            }));
+        }
+    }


I think this could be collapsed to

if (!layoutState()) return; auto currentLineClamp = layoutState()->lineClamp(); m_layoutStateStack.removeLast(); if (auto* layoutState = this->layoutState(); layoutState && layoutState->lineClamp()) layoutState->setLineClamp(currentLineClamp);

maybe asserting on not mutating the maximum line clamp value

I chose to assert for maximumLineCount since it should be an immutable state established at applyModernLineClamp.

webkit-early-warning-system · 2023-04-24T13:26:06Z

EWS run on current version of this PR (hash 4865a76)

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

GetToSet · 2023-04-25T09:06:41Z

@Ahmad-S792 Could you help try landing this patch?

Ahmad-S792 · 2023-04-25T09:38:20Z

@Ahmad-S792 Could you help try landing this patch?

Added merge-queue to land this and also confirmed that the failing tests are not related to this change but flaky failures.

… propagated to the parent layoutState. https://bugs.webkit.org/show_bug.cgi?id=255487 Reviewed by Alan Baradlay. Since multiple children share the same parent context for line clamp, `LineClamp.currentLineCount` set to the individual child should be propagated to their parent layoutState. Otherwise, subsequent children may not see the current state of this value during layout, causing incorrect line clamp results. * LayoutTests/fast/block/line-clamp-nested-blocks-with-layout-state-expected.html: Added. * LayoutTests/fast/block/line-clamp-nested-blocks-with-layout-state.html: Added. * Source/WebCore/page/LocalFrameViewLayoutContext.cpp: (WebCore::LocalFrameViewLayoutContext::popLayoutState): Canonical link: https://commits.webkit.org/263360@main

webkit-commit-queue · 2023-04-25T10:20:47Z

Committed 263360@main (59088ba): https://commits.webkit.org/263360@main

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

Ahmad-S792 · 2023-04-25T10:43:00Z

@alanbaradlay - It seems that the bug landed against closed bug, should I ask 'webkit-bot' to revert it and ask @GetToSet to update Bug URL in commit message and land with correct bug number? I left comment on bugzilla as well.

alanbaradlay · 2023-04-25T14:52:34Z

@alanbaradlay - It seems that the bug landed against closed bug, should I ask 'webkit-bot' to revert it and ask @GetToSet to update Bug URL in commit message and land with correct bug number? I left comment on bugzilla as well.

oops. I wouldn't revert it. I think it's ok as long as bugzillas are commented and connected accordingly.

GetToSet requested a review from cdumez as a code owner April 20, 2023 15:35

GetToSet force-pushed the eng/bug-255728-pr branch 2 times, most recently from 193e4c2 to 4483e29 Compare April 20, 2023 15:47

nt1m requested a review from alanbaradlay April 20, 2023 17:12

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 21, 2023

GetToSet force-pushed the eng/bug-255728-pr branch from 4483e29 to 209cde3 Compare April 22, 2023 04:05

GetToSet changed the title ~~[IFC][Integration][Line clamp] visibleLineCountForLineClamp should be propagated to the parent layoutState.~~ [IFC][Integration][Line clamp] LineClamp.currentLineCount should be propagated to the parent layoutState. Apr 22, 2023

Ahmad-S792 assigned GetToSet Apr 23, 2023

Ahmad-S792 added Layout and Rendering For bugs with layout and rendering of Web pages. and removed merging-blocked Applied to prevent a change from being merged labels Apr 23, 2023

Ahmad-S792 requested review from smfr and removed request for cdumez April 23, 2023 06:25

alanbaradlay approved these changes Apr 23, 2023

View reviewed changes

GetToSet force-pushed the eng/bug-255728-pr branch from 209cde3 to 4865a76 Compare April 24, 2023 13:22

GetToSet requested a review from alanbaradlay April 24, 2023 14:34

alanbaradlay approved these changes Apr 24, 2023

View reviewed changes

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 24, 2023

Ahmad-S792 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 25, 2023

webkit-commit-queue force-pushed the eng/bug-255728-pr branch from 4865a76 to 59088ba Compare April 25, 2023 10:20

webkit-commit-queue merged commit 59088ba into WebKit:main Apr 25, 2023

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Apr 25, 2023

GetToSet deleted the eng/bug-255728-pr branch April 26, 2023 02:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IFC][Integration][Line clamp] `LineClamp.currentLineCount` should be propagated to the parent layoutState. #12973

[IFC][Integration][Line clamp] `LineClamp.currentLineCount` should be propagated to the parent layoutState. #12973

GetToSet commented Apr 20, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Apr 20, 2023 •

edited

webkit-early-warning-system commented Apr 20, 2023 •

edited

webkit-early-warning-system commented Apr 20, 2023 •

edited

webkit-early-warning-system commented Apr 22, 2023 •

edited

GetToSet commented Apr 22, 2023

alanbaradlay left a comment

alanbaradlay Apr 23, 2023

GetToSet Apr 24, 2023

webkit-early-warning-system commented Apr 24, 2023 •

edited

GetToSet commented Apr 25, 2023

Ahmad-S792 commented Apr 25, 2023

webkit-commit-queue commented Apr 25, 2023

Ahmad-S792 commented Apr 25, 2023

alanbaradlay commented Apr 25, 2023

[IFC][Integration][Line clamp] LineClamp.currentLineCount should be propagated to the parent layoutState. #12973

[IFC][Integration][Line clamp] LineClamp.currentLineCount should be propagated to the parent layoutState. #12973

Conversation

GetToSet commented Apr 20, 2023 • edited by webkit-early-warning-system

59088ba

webkit-early-warning-system commented Apr 20, 2023 • edited

webkit-early-warning-system commented Apr 20, 2023 • edited

webkit-early-warning-system commented Apr 20, 2023 • edited

webkit-early-warning-system commented Apr 22, 2023 • edited

GetToSet commented Apr 22, 2023

alanbaradlay left a comment

Choose a reason for hiding this comment

alanbaradlay Apr 23, 2023

Choose a reason for hiding this comment

GetToSet Apr 24, 2023

Choose a reason for hiding this comment

webkit-early-warning-system commented Apr 24, 2023 • edited

GetToSet commented Apr 25, 2023

Ahmad-S792 commented Apr 25, 2023

webkit-commit-queue commented Apr 25, 2023

Ahmad-S792 commented Apr 25, 2023

alanbaradlay commented Apr 25, 2023

[IFC][Integration][Line clamp] `LineClamp.currentLineCount` should be propagated to the parent layoutState. #12973

[IFC][Integration][Line clamp] `LineClamp.currentLineCount` should be propagated to the parent layoutState. #12973

GetToSet commented Apr 20, 2023 •

edited by webkit-early-warning-system

`59088ba`

webkit-early-warning-system commented Apr 20, 2023 •

edited

webkit-early-warning-system commented Apr 20, 2023 •

edited

webkit-early-warning-system commented Apr 20, 2023 •

edited

webkit-early-warning-system commented Apr 22, 2023 •

edited

webkit-early-warning-system commented Apr 24, 2023 •

edited