Closed Bug 1774135 Opened 2 years ago Closed 2 years ago

ResizeObserver devicePixelContentBox does not work

Categories

(Core :: Layout, defect)

Firefox 103
defect

Tracking

()

VERIFIED FIXED
104 Branch
Tracking Status
firefox104 --- verified

People

(Reporter: mozilla2, Assigned: dshin)

References

Details

Attachments

(2 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/102.0.0.0 Safari/537.36

Steps to reproduce:

Run this sample on a Mac

https://jsgist.org/?src=1b1186ddf5178871e16b98e131b2ea7b

Actual results:

The same makes a 299px width div with 3 children. Checking the sizes using ResizeObserver devicePixelContentBox it reports all 3 as 199 device pixels. But if you actually measure them (take a screenshot) the center div is 200 device pixels.

Further, 199 * 3 = 597 but Firefox reports the parent of all 3 boxes as 598 pixels. The missing pixel is in the center div

Expected results:

Firefox should have reported 200 for one of the divs (the center div in this case as it actually drew it 200 device pixels).

Just to be clear, the entire point of devicePixelContentBox is that, at least in some browsers, the DOM code doesn't know what size will ultimately be chosen. It passes 99.66666667px css width for each of the 3 children up to the rasterizer and the rasterize figures out which of the 3 children get the extra pixel. That's why devicePixelContentBox was needed, because from the DOM's info alone it's impossible to know which element will get that extra pixel.

I didn't look into the implementation in detail in Firefox but if it's not getting the correct info it should arguably be disable until it does. You can already get the same wrong answer by just using Math.round(getBoundClientRect().width * devicePixelRatio) Returning the wrong info just makes people try to find workarounds, workaround that will break when it starts providing the correct info.

The Bugbug bot thinks this bug should belong to the 'Core::CSS Parsing and Computation' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Component: Untriaged → CSS Parsing and Computation
Product: Firefox → Core
Component: CSS Parsing and Computation → Layout
Severity: -- → S3
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(boris.chiou)

It looks like the relevant code here is in ResizeObserver.cpp CalculateBoxSize:

https://searchfox.org/mozilla-central/rev/4654ce1e24a6af17bc96ab60f1f70c218755142f/dom/base/ResizeObserver.cpp#115-127

case ResizeObserverBoxOptions::Device_pixel_content_box: {
  // This is a implementation-dependent for subpixel snapping algorithm.
  // Gecko relies on LayoutDevicePixel to convert (and snap) the app units
  // into device pixels in painting and gfx code, so here we simply
  // convert it into dev pixels and round it.
  //
  // Note: This size must contain integer values.
  // https://drafts.csswg.org/resize-observer/#dom-resizeobserverboxoptions-device-pixel-content-box
  const LayoutDeviceIntSize snappedSize =
      LayoutDevicePixel::FromAppUnitsRounded(
          GetContentRectSize(*frame),
          frame->PresContext()->AppUnitsPerDevPixel());
  return gfx::Size(snappedSize.ToUnknownSize());

Our simply convert it into dev pixels and round it behavior (quoting the comment above) is the thing that's causing trouble for the reporter's testcase here. i.e. we're guessing at the device pixel box size, but not necessarily guessing correctly (possibly off-by-1 depending on pixel-snapping)

We probably need to get information from our painting code about how the device-pixel-snapping worked out and feed that into our reported value here.

Depends on: 1587973

The relevant spec text is:

  • device-pixel-content-box : size of content area as defined in CSS2, in device pixels, before applying any CSS transforms on the element or its ancestors. This size must contain integer values.

<note>
The device-pixel-content-box can be approximated by multiplying devicePixelRatio by the content-box size. However, due to browser-specific subpixel snapping behavior, authors cannot determine the correct way to round this scaled content-box size. How a UA computes the device pixel box for an element is implementation-dependent. One possible implementation could be to multiply the box size and position by the device pixel ratio, then round both the resulting floating-point size and position of the box to integer values, in a way that maximizes the quality of the rendered output.
</note>
https://drafts.csswg.org/resize-observer/#dom-resizeobserverboxoptions-device-pixel-content-box

Our current implementation is essentially equivalent to the "...can be approximated..." piece of the note here (which is something that authors could do on their own). The rest of the note is saying that the intent of the API is to actually report the device-pixel-snapped value that we use (however the browser arrives at that value).

I'm not sure offhand where to get the pixel-snapped size, and whether we have it available at this point in the code vs. if we need to somehow leave a breadcrumb that our display-list code can detect and fill-in (or something like that). CC'ing tnikkel and miko who are closer to our painting code & might have thoughts here or might be good reviewers.

FWIW: taking a screenshot on the attached testcase on my monitor (using 100% pixel scaling) and pasting it into Gimp and zooming in, I see:

  • the boundaries between the rects are indeed crisp (they're drawn in a pixel-snapped way)
  • the actual observed widths are (from left to right): 100px, 99px, 100px

(In reply to Daniel Holbert [:dholbert] from comment #5)

I'm not sure offhand where to get the pixel-snapped size, and whether we have it available at this point in the code vs. if we need to somehow leave a breadcrumb that our display-list code can detect and fill-in (or something like that).

Hand-wavy idea about how to approach this:

  1. We should add a boolean member-var on nsIFrame: a new bool mResizeObserverNeedsDPCB : 1 added after the others here). We should set this bit to true when a resize observer is registered with "device-pixel-content-box" in its options. (As a hack to simplify the initial/prototype impl, we could just have the bit be always-true, to be extra-greedy. Though of course don't want to land in this greedy state.)

  2. At the point in our painting/display-list code where we make pixel-snapping decisions, we should add an if (MOZ_UNLIKELY(frame->ResizeObserverNeedsDPCB())) { ... } block of code which checks this new bool and (if it's true) computes the appropriate box size and stores it on the frame in a frame-property (similar to how we store UsedPaddingProperty).

  3. When we reach the code that I quoted in comment 4, we should just check for the new frame-property and use the rect that we expect to be stored there. (We might also need/want to preserve the existing code as a fallback, e.g. if we happen to skip a paint and want to return something reasonable? But really/ideally we want to always ensure that we're reporting an actual rect that we computed during painting.)

I've suggested that dshin might want to take a look at fixing this, so I'll redirect Boris's needinfo to him. (Though Boris says he's happy to help with ResizeObserver-internals questions, and as above I think Timothy/Miko might be good folks to ask about painting stuff i.e. step 2 in my hand-wavy approach. And I'm happy to try to fill in details as well. :) )

Flags: needinfo?(boris.chiou) → needinfo?(dshin)

(If it's not clear: I'm using DPCB as an abbreviation for "devicePixelContentBox" in my dummy-code above. Other abbreviations/namings might be better though. :) Just a placeholder for discussion purposes.)

I'm not sure it needs to be so complex. Display items snap in coordinates relative to the closest reference frame, so the current code but computing the position relative to the closest ancestor reference frame should do I think.

Flags: needinfo?(dshin)
Assignee: nobody → dshin
Status: NEW → ASSIGNED
Pushed by dshin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/762f97ad7458
`ResizeObserver`: Take subpixel snapping into account when reporting `devicePixelContentBoxSize`. r=emilio
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 104 Branch
Flags: qe-verify+

I was able to reproduce this issue on Firefox 103.0(build ID: 20220718155818) on macoS 12 using the testcase from Comment 2. Verified as fixed on Firefox 104.0(build ID: 20220816115024) and Nightly 105.0a1(build ID: 20220821185924) on macOS 12.

Status: RESOLVED → VERIFIED
Flags: qe-verify+

this is not actually fixed. It passes the test above. Here's another that it fails

Steps:

  1. On a Mac, Go to https://webglfundamentals.org/webgl/webgl-resize-the-canvas-comparison.html
  2. Zoom in to 67% (press Command+Minus 3 times)
  3. Resize the window

What should happen:

The bottom example should always show black and white lines with no gradient

What happens instead:

Often a gradient appears (the rectangle will appear as though a gradient of black to white was drawn across a portion of it)

Firefox version: 107.0a1 (2022-09-23) 64-bit
OS: macOS Monterey 12.6

Works in Chrome

Hm. Could possibly be the behaviour discussed in bug 1723618.
:boris, is there something specific at play with canvas?

Flags: needinfo?(boris.chiou)

Here's the same example using putImageData instead of fillrect with a pattern. It shows the same issue

https://jsgist.org/?src=018b43801cf87e75eb52e1e446dd7a6f

And via WebGL if you don't trust that putImageData doesn't get masssaged

https://jsgist.org/?src=efa3ba6ee0984684340ba0ee0e2af779

They all show the same issue

(In reply to David Shin[:dshin] from comment #15)

Hm. Could possibly be the behaviour discussed in bug 1723618.
:boris, is there something specific at play with canvas?

Looks like we need an expert for Canvas.

Hi lsalzman, per comment 14 and comment 16, we would like to have a more precise way to know the device pixel of the content box in ResizeOberserver. However, it seems there are still some issues when using Canvas2D. I suspect this may be caused by some subpixl antialiasing calculation or something like that. Do you have any suggestion to improve the calculation for Canvas2D for ResizeObserver?

Here is the implementation of devicePixelContentBox for ResizeObserver: https://searchfox.org/mozilla-central/rev/a26af613a476fafe6c3eba05a81bef63dff3c9f1/dom/base/ResizeObserver.cpp#124-153.

Flags: needinfo?(boris.chiou) → needinfo?(lsalzman)

you're right, it does appear to have something to do with the canvas.

Here's a test with a canvas and a div

https://jsgist.org/?src=c2e4cdca322620bdaf4e46764d6ce761

The div works, the canvas doesn't, both work in Chrome

(In reply to Boris Chiou [:boris] from comment #17)

(In reply to David Shin[:dshin] from comment #15)

Hm. Could possibly be the behaviour discussed in bug 1723618.
:boris, is there something specific at play with canvas?

Looks like we need an expert for Canvas.

Hi lsalzman, per comment 14 and comment 16, we would like to have a more precise way to know the device pixel of the content box in ResizeOberserver. However, it seems there are still some issues when using Canvas2D. I suspect this may be caused by some subpixl antialiasing calculation or something like that. Do you have any suggestion to improve the calculation for Canvas2D for ResizeObserver?

Here is the implementation of devicePixelContentBox for ResizeObserver: https://searchfox.org/mozilla-central/rev/a26af613a476fafe6c3eba05a81bef63dff3c9f1/dom/base/ResizeObserver.cpp#124-153.

This seems more like a DOM issue to me. I can't really fathom how anything related to rasterization in canvas would effect this, so I don't have much insight to offer.

Flags: needinfo?(lsalzman)

I would think it's a DOM (or compositor issue) too.

I added a "save canvas to png" button

https://jsgist.org/?src=c2e4cdca322620bdaf4e46764d6ce761

It verifies the canvases are rendered correctly.

I set the zoom to 67% and took screenshot (Cmd-Shift-4) and then loaded the resulting screenshot into an image editor (photoshop) and measured the sizes. Sometimes the size rendered matches the size reported but the compositing of the canvas is wrong.

https://i.imgur.com/235BSiA.png

The canvas png from this run

https://i.imgur.com/KqC8Qth.png

Other times the size report does not match the size rendered.

https://i.imgur.com/xnnKvmF.png

The canvas png from that run

https://i.imgur.com/NudQjLf.png

My guess would be there's a round/floor/ceil in one place and a different round/floor/ceil in another

Sorry, the screenshot above of the canvas size matching the reported size was bad. Here's the correct screenshot

https://i.imgur.com/oFI93To.png

We should move the discussion into a new bug in any case. nsDisplayCanvas seems to do some interesting (non) snapping, so perhaps the issue lies there.

See Also: → 1792285
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: