Canvas with relative width does not repaint correctly on parent resize #44463

cathiechen · 2025-04-24T10:31:31Z

`7bbc1c0`

Canvas with relative width does not repaint correctly on parent resize
https://bugs.webkit.org/show_bug.cgi?id=267986

Reviewed by Alan Baradlay.

Elements with relative width do not trigger a full repaint when their size changes because they are not selfNeedsLayout.
For elements like <canvas> and <img>, a size change implies a scale change, so they should trigger a full repaint.
This issue is only revealed if the replaced element is on a tiled layer and its ancestor with selfNeedsLayout on other layer.
If they are on the same layer, the ancestor would trigger full repaint for itself, the repaint rect will cover the replace element.
If the replaced element is on an untiled layer, this layer triggers a full repaint on layer size change.
This patch triggers full repaint for elements like canvas and image in after layout.

* LayoutTests/compositing/tiling/huge-layer-canvas-resize-expected.txt: Added.
* LayoutTests/compositing/tiling/huge-layer-canvas-resize.html: Added.
* LayoutTests/compositing/tiling/huge-layer-img-resize-expected.txt: Added.
* LayoutTests/compositing/tiling/huge-layer-img-resize.html: Added.
* LayoutTests/platform/glib/TestExpectations:
* LayoutTests/platform/ios/compositing/tiling/huge-layer-canvas-resize-expected.txt: Added.
* LayoutTests/platform/ios/compositing/tiling/huge-layer-img-resize-expected.txt: Added.
* LayoutTests/platform/win/TestExpectations:
* Source/WebCore/rendering/RenderReplaced.cpp:
(WebCore::issueFullRepaintOnSizeChangeIfNeeded):
(WebCore::RenderReplaced::layout):

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

fae6baa

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	❌ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
✅ 🛠 🧪 merge	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2	✅ 🛠 playstation
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2025-04-24T10:31:38Z

EWS run on previous version of this PR (hash c899a18)

Details

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2	❌ 🧪 win-tests
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	❌ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	❌ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2	✅ 🛠 playstation
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2025-04-24T15:56:50Z

EWS run on previous version of this PR (hash 858d7a8)

Details

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2	❌ 🧪 win-tests
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	❌ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	❌ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	❌ 🧪 gtk-wk2
	✅ 🛠 vision-sim	❌ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2	❌ 🧪 mac-intel-wk2	✅ 🛠 playstation
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

alanbaradlay · 2025-04-24T18:18:59Z

Source/WebCore/rendering/RenderReplaced.h

Any reason why not just pass in shouldAlwaysIssueFullRepaint to LayoutRepainter in RenderReplaced::layout() when needed?

Yeah, shouldAlwaysIssueFullRepaint would work for replaced element without layer. For element hasLayer, repaintAfterLayoutIfNeeded is triggered in recursiveUpdateLayerPositions, we need to set NeedsFullRepaint to its RenderLayer.
I don't have a strong opinion. But if you prefer shouldAlwaysIssueFullRepaint + NeedsFullRepaint approach, I will adjust the patch later.

oh I just realized this requires post layout check. ok, I mean I am fine either way but isn't this just a repaint() call which I think could be achieved with a more focused change, something along the lines of

static void issueFullRepaintOnSizeChangeIfNeeded(RenderReplaced& renderer) { auto shouldRepaintOnSizeChange = [&] { if (is<RenderHTMLCanvas>(renderer)) return true; if (auto* renderImage = dynamicDowncast<RenderImage>(renderer); renderImage && !is<RenderMedia>(*renderImage) && !renderImage->isShowingMissingOrImageError()) return true; return false; }; if (shouldRepaintOnSizeChange()) renderer.repaint(); }

called by RenderReplaced::layout when replacedContentRect() != oldContentRect

or alternatively RenderImage/RenderHTMLCanvas::layout could just call repaint() after coming backing from base class layout when content box size changed.

True, it looks clearer.

webkit-early-warning-system · 2025-04-25T23:36:03Z

EWS run on previous version of this PR (hash 9344607)

Details

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 win-tests
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	❌ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	❌ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	❌ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	❌ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2	❌ 🧪 mac-intel-wk2	✅ 🛠 playstation
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2025-05-01T03:32:33Z

EWS run on previous version of this PR (hash 44f343d)

Details

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 win-tests
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	❌ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	❌ 🧪 gtk-wk2
	✅ 🛠 vision-sim	❌ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2	❌ 🧪 mac-intel-wk2	✅ 🛠 playstation
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2025-05-01T07:21:47Z

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

Details

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~	✅ 🧪 win-tests
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	❌ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	~~🧪 mac-AS-debug-wk2~~	~~🧪 gtk-wk2~~
	✅ 🛠 vision-sim	❌ 🧪 mac-wk2-stress	~~🧪 api-gtk~~
	✅ 🧪 vision-wk2	❌ 🧪 mac-intel-wk2	✅ 🛠 playstation
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2025-05-01T07:46:53Z

EWS run on current version of this PR (hash fae6baa)

Details

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	❌ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
✅ 🛠 🧪 merge	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2	✅ 🛠 playstation
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

https://bugs.webkit.org/show_bug.cgi?id=267986 Reviewed by Alan Baradlay. Elements with relative width do not trigger a full repaint when their size changes because they are not selfNeedsLayout. For elements like <canvas> and <img>, a size change implies a scale change, so they should trigger a full repaint. This issue is only revealed if the replaced element is on a tiled layer and its ancestor with selfNeedsLayout on other layer. If they are on the same layer, the ancestor would trigger full repaint for itself, the repaint rect will cover the replace element. If the replaced element is on an untiled layer, this layer triggers a full repaint on layer size change. This patch triggers full repaint for elements like canvas and image in after layout. * LayoutTests/compositing/tiling/huge-layer-canvas-resize-expected.txt: Added. * LayoutTests/compositing/tiling/huge-layer-canvas-resize.html: Added. * LayoutTests/compositing/tiling/huge-layer-img-resize-expected.txt: Added. * LayoutTests/compositing/tiling/huge-layer-img-resize.html: Added. * LayoutTests/platform/glib/TestExpectations: * LayoutTests/platform/ios/compositing/tiling/huge-layer-canvas-resize-expected.txt: Added. * LayoutTests/platform/ios/compositing/tiling/huge-layer-img-resize-expected.txt: Added. * LayoutTests/platform/win/TestExpectations: * Source/WebCore/rendering/RenderReplaced.cpp: (WebCore::issueFullRepaintOnSizeChangeIfNeeded): (WebCore::RenderReplaced::layout): Canonical link: https://commits.webkit.org/294401@main

webkit-commit-queue · 2025-05-01T23:23:41Z

Committed 294401@main (7bbc1c0): https://commits.webkit.org/294401@main

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

shallawa · 2025-05-05T18:48:28Z

LayoutTests/compositing/tiling/huge-layer-canvas-resize.html

+        }
+        if (window.testRunner)
+            testRunner.notifyDone();
+    }, 1000);


What exactly are you waiting 1000ms for? This makes the behavior of the test un-deterministic. Besides one second is too long time to wait. Can not it be replaced by

requestAnimationFrame(() => { ... });

We need to make sure the repaint rects before css change has flashed, so I added 1000 ms. requestAnimationFrame seems not long enough, especially in stress mode.

Rego suggested me to use two requestAnimationFrame, it works! I have created a PR to address the code review comments. Please take a look, thanks:) #45040

shallawa · 2025-05-05T18:49:23Z

LayoutTests/compositing/tiling/huge-layer-img-resize.html

+    img.src = canvas.toDataURL();
+}
+
+async function onImageLoad()


Why is this function marked as async? It does not have await statement.

shallawa · 2025-05-05T18:58:47Z

Source/WebCore/rendering/RenderReplaced.cpp

-    if (replacedContentRect() != oldContentRect)
+    if (replacedContentRect() != oldContentRect) {
        setPreferredLogicalWidthsDirty(true);
+        issueFullRepaintOnSizeChangeIfNeeded(*this);


I think this is a long name for a function. Could it be just?

if (shouldRepaintOnSizeChange(*this)) repaint();

cathiechen self-assigned this Apr 24, 2025

cathiechen added the Canvas Bugs related to the canvas element. label Apr 24, 2025

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

cathiechen removed the merging-blocked Applied to prevent a change from being merged label Apr 24, 2025

cathiechen force-pushed the canvas-paint branch from c899a18 to 858d7a8 Compare April 24, 2025 15:56

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

alanbaradlay reviewed Apr 24, 2025

View reviewed changes

cathiechen removed the merging-blocked Applied to prevent a change from being merged label Apr 25, 2025

cathiechen force-pushed the canvas-paint branch from 858d7a8 to 9344607 Compare April 25, 2025 23:35

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

cathiechen removed the merging-blocked Applied to prevent a change from being merged label May 1, 2025

cathiechen force-pushed the canvas-paint branch from 9344607 to 44f343d Compare May 1, 2025 03:32

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 1, 2025

cathiechen removed the merging-blocked Applied to prevent a change from being merged label May 1, 2025

cathiechen force-pushed the canvas-paint branch from 44f343d to 8f3a9e4 Compare May 1, 2025 07:21

cathiechen force-pushed the canvas-paint branch from 8f3a9e4 to fae6baa Compare May 1, 2025 07:46

alanbaradlay approved these changes May 1, 2025

View reviewed changes

cathiechen added the merge-queue Applied to send a pull request to merge-queue label May 1, 2025

webkit-commit-queue force-pushed the canvas-paint branch from fae6baa to 7bbc1c0 Compare May 1, 2025 23:23

webkit-commit-queue merged commit 7bbc1c0 into WebKit:main May 1, 2025

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 1, 2025

shallawa reviewed May 5, 2025

View reviewed changes

Canvas with relative width does not repaint correctly on parent resize #44463

Canvas with relative width does not repaint correctly on parent resize #44463

Uh oh!

Conversation

cathiechen commented Apr 24, 2025 • edited by webkit-early-warning-system Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webkit-early-warning-system commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-commit-queue commented May 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cathiechen May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cathiechen May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cathiechen commented Apr 24, 2025 •

edited by webkit-early-warning-system

Loading

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

edited

Loading

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

edited

Loading

webkit-early-warning-system commented Apr 25, 2025 •

edited

Loading

webkit-early-warning-system commented May 1, 2025 •

edited

Loading

webkit-early-warning-system commented May 1, 2025 •

edited

Loading

webkit-early-warning-system commented May 1, 2025 •

edited

Loading

cathiechen May 6, 2025 •

edited

Loading

cathiechen May 7, 2025 •

edited

Loading