1795536 - Setting dimensions in a ResizeObserver callback lead to some inconsistencies

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Calixte Denizet (:calixte)]

Attachment: resize_observer.html

Context:
There is this issue in pdf.js:
https://github.com/mozilla/pdf.js/issues/15571
where we use a ResizeObserver and in debugging I noticed that the width of my container was always in px when the height was always in percent.
So I wrote this test case which is pretty similar to what we've in pdf.js and the idea is to always have some dimensions expressed in percent.

STR:

• open the test case
• open devtools and inspect the pink div
• play with the resizer and observe the element width in devtools.

On Windows 11, the width is almost always a percent but from time to time it's in pixels.
On Mac 12.6, the width is almost always in pixels and from time to time in percents.

I've the feeling that the width I set myself in the callback is at some point overwritten and it shouldn't else it means that we can't reliably set some dimensions in the callback.

:dholbert, could you help here ?

Comment 1

[Jonathan Kew [:jfkthame]]

It kinda looks like trying to update the width/height properties from within the ResizeObserver is flaky because it may be racing somehow with the update that's already in progress.

If I make the ResizeObserver defer its update with e.g. a 100ms setTimeout, then it "sticks" (but gets laggy, obviously, as the delayed updates get applied). And a much shorter timeout like 10ms isn't enough to consistently work.

FWIW, in both Chrome and Safari, the percentage value set by the ResizeObserver does seem to consistently "win" here. Watching in their devtools I see intermittent px values appearing momentarily while dragging, but the final value is the % one.

@emilio, can/should we do something to ensure more consistently-ordered behavior here?

Comment 2

[Calixte Denizet (:calixte)]

Yep using a setTimeout is my workaround:
https://github.com/mozilla/pdf.js/pull/15578/files#diff-cfbcdf0fb56fd7886470d924fb749e533c16748b1bce9eb9809f0f8a97b2c44bR615
where I fix the dimensions once the resize is "finished".

Comment 3

[Emilio Cobos Álvarez [:emilio]]

I don't think this is a problem of ordering. This is a problem of the resizer setting a size that doesn't resize the element after all (because the element already has the right size). If it doesn't trigger a resize, the inline style change doesn't trigger the resize observer, and leaves the pixel size.

Something like this would fix it:

diff --git a/dom/xul/XULResizerElement.cpp b/dom/xul/XULResizerElement.cpp
index 4c437760b6a06..8bc776cd830a8 100644
--- a/dom/xul/XULResizerElement.cpp
+++ b/dom/xul/XULResizerElement.cpp
@@ -208,6 +208,14 @@ void XULResizerElement::PostHandleEventInternal(
 
         auto cssSize = CSSIntSize::FromAppUnitsRounded(newSize);
 
+        if (contentToResize->GetPrimaryFrame() &&
+            contentToResize->GetPrimaryFrame()->GetSize() ==
+                CSSPixel::ToAppUnits(cssSize)) {
+          // Avoid changing the size if not resizing after all. Make sure to use
+          // the rounded size.
+          break;
+        }
+
         SizeInfo sizeInfo, originalSizeInfo;
         sizeInfo.width.AppendInt(cssSize.width);
         sizeInfo.height.AppendInt(cssSize.height);

But it feels a bit hackish, at best. It seems to match what other browsers are doing, roughly, see:

• https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/paint/paint_layer_scrollable_area.cc;l=2218;drc=b3842582ae4610f1847d69076ad00570c2eafd79
• https://searchfox.org/wubkat/rev/1015ec14c506b81dcf3b161568c93af61a028229/Source/WebCore/rendering/RenderLayer.cpp#2603

So maybe it's fine?

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1795536 - Don't let resizer set the width/height properties of the target element if its size won't change. r=jfkthame

I'm not quite sure this is worth it, your call. I guess it needs a test
if we actually want to do this, of course...

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6535746d5911
Don't let resizer set the width/height properties of the target element if its size won't change. r=jfkthame

Comment 6

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/36618 for changes under testing/web-platform/tests

Comment 7

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/6535746d5911

Comment 8

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot