Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement ResizeObserver #2859

Merged
merged 6 commits into from
Jun 14, 2023
Merged

Conversation

daxpedda
Copy link
Member

@daxpedda daxpedda commented Jun 7, 2023

Problems

Accuracy

We have two different forms of accuracy here, which I will name "pixel perfect" and "precise".

"Pixel perfect" information about the size of the canvas is really important, see this blog post for more details, which includes a nice visual demo of the problems that can arise without such information.
But to summarize, the user absolutely requires this information to determine at what resolution to render to the canvas, otherwise they will get visual artifacts. This is important because we calculate physical pixels by converting CSS pixels with the scale factor, if the scale factor is fractional result, we have to round, which may or may not match the actual size in physical pixels on the users screen.
This is only a problem with fractional scale factors or if the canvas is positioned with fractional CSS pixel values.
In any case, the only way to accurately get information about the actual physical pixel size of the canvas is devicePixelContentBoxSize.

"Precise" describes the fact that some APIs report accurate CSS pixel values, while others just don't return fractional values. We want precise values to be able to accurately convert to physical pixels with the scale factor. Again, this is only a problem with fractional scale factors or if the canvas is positioned with fractional CSS pixel values.

Consistency between Window::set_inner_size(), Window::inner_size() and WindowEvent::Resized

Preferably the size set by set_inner_size() should be the same as the one returned by inner_size() and the one received by the WindowEvent::Resized event. This is difficult because a lot of the APIs are inconsistent with each other or require imprecise conversions.

To clarify why conversions are relevant here: there is no API to set the size by physical pixels in the Web, you can only set the size of elements in logical pixel units (plenty of other unit types, but they aren't relevant here, see MDN).

devicePixelContentBoxSize can never be accurately converted to or from, because it reports pixel perfect physical sizes, any conversion can only round results. But being a pixel off might be an acceptable tradeoff.

getBoundingClientRect() respects transform, which would require significant effort on our side to calculate conversions to or from. See the MDN documentation for some examples what transform can do. So we will either have to document that using transform might significantly alter the output or just refrain from using this API.

Sync Window::set_inner_size() and Window::inner_size()

Unfortunately ResizeObserver cannot be queried, we receive it's values only through callbacks. So we will have to decide if we would rather give the user inaccurate information through Window::inner_size() until they receive a WindowEvent::Resized or just not update Window::inner_size() until we get accurate values, which the user will be notified of by WindowEvent::Resized.

API Overview

API: Name of the API.
queryable: Can be queried at any point in time, in contrast to having to wait for a callback.
transform: Will adjust the reported size depending on any transform properties.
precise: Reports CSS pixel values without rounding to the nearest integer.
pixel perfect: Reports the size of the element with exact physical pixels.

API queryable transform precise pixel perfect
devicePixelContentBoxSize πŸ―€ πŸ―€ πŸ―€ βœ…
contentRect1 πŸ―€ πŸ―€ βœ… πŸ―€
getBoundingClientRect() βœ… βœ… βœ… πŸ―€
getComputedStyle() βœ… πŸ―€ βœ… πŸ―€
scrollWidth/Height2 βœ… πŸ―€ πŸ―€ πŸ―€

API Analysis

We definitely want to use devicePixelContentBoxSize somehow. If we don't make it the value returned by Window::inner_size() and WindowEvent::Resized, we could at least expose it through WindowExtWebSys.

If we really want Window::set_inner_size() and Window::inner_size()/WindowEvent::Resized to be consistent, we can use contentRect.

Making sure that Window::inner_size() is able to return the correct/same value right after using Window::set_inner_size() is problematic. getBoundingClientRect() respects transform and scrollWidth/Height is imprecise.

Conclusion

I think using devicePixelContentBoxSize has the proper tradeoffs for Winit. We get pixel perfect sizes and I find the tradeoffs acceptable.

We can treat Window::set_inner_size() as a request, so it isn't fulfilled until WindowEvent::Resized is received. It being "just a request", I can further extrapolate on saying that it's fine if the received size by Window::inner_size()/WindowEvent::Resized isn't always the same as the one passed into Window::set_inner_size(), which will commonly be the same anyway or at most a pixel off.

Implementation

Changes

  • Removed fullscreen detection completely, as it was only changing the size of the canvas.
  • We now don't change the size of the canvas at all unless the user uses Window::set_inner_size() or changes WindowEvent::ScaleFactorChanged::new_inner_size.
  • Window::inner_size() now always returns the precise physical pixel size of the canvas, although not in sync with Window::set_inner_size(), which now is treated as a request only, see Return the applied size for synchronious set_inner_sizeΒ #2868.
  • Scale factor changes will always be reported on the Web before resize events, so we wouldn't get the current size of the canvas after a scale factor change. This PR introduces a mechanic to create oneshot ResizeObservers, that report the size back when needed. Therefore the resize and scale factor handlers are now merged, as they are closely intertwined.
  • Safari doesn't support devicePixelContentBoxSize, which had to be accounted for when scale factor changes, as that could affect the physical size without being reported by contentRect, the fallback to devicePixelContentBoxSize.
  • Introduced a new EventWrapper type, that allows synchronous event execution without requiring global handlers. Now that resize and scale factor event handlers are merged, they can't be global, but have to be made per canvas.
  • To account for Safari not supporting the initial event in ResizeObserver if the size of the element is zero, we had to do some internal size tracking to inhibit duplicate Resized events.

New scale factor flow

When we notify the resize handler, it will re-observer the canvas with the ResizeObserver to get the initial event and get the size from there, if the browser supports devicePixelContentBoxSize. If the browser doesn't, it will just use getComputedStyle() to calculate the inner size of the canvas without any callbacks involved.

  1. Scale factor change was detected.
  2. Notify the resize handler.
  3. When we receive the new current size, we send ScaleFactorChanged to the user.
    • If the size didn't change from before and the user didn't change new_inner_size, we do nothing.
    • If the size changed and the user didn't change new_inner_size, we send a Resized event.
    • If the user changes new_inner_size, we notify the resize handler again and report Resized from there.

Fixes #1661.
Fixes #2864.
Replaces #2074.

Footnotes

  1. It is marked for deprecation in the future, but is the same as contentBoxSize. Used here because it's the only option in ResizeObserverEntry that is supported by Safari. ↩

  2. There is also clientWidth/Height, which doesn't include content hidden by a scrollbar and offsetWidth/Height, which is similar to clientWidth/Height but includes borders. ↩

@daxpedda daxpedda marked this pull request as draft June 7, 2023 20:08
@daxpedda daxpedda marked this pull request as ready for review June 7, 2023 20:44
@daxpedda daxpedda marked this pull request as draft June 7, 2023 21:00
@Liamolucko
Copy link
Contributor

... resulting in:

  • Scale factor is not zoom level.
  • There is no API to query the zoom level.
  • devicePixelContentBoxSize always reports exactly the physical size in pixels an element occupies on the screen, ignoring scale and zoom, but not pinch to zoom.
  • Converting between any size and devicePixelContentBoxSize by using the scale factor would work with zoom, but would be wrong without zoom, e.g. a mobile device.

Therefor there is no way to set devicePixelContentBoxSize on an element and there is no way to calculate a conversion reliably.

I don't understand why that prevents converting between devicePixelContentBoxSize and the CSS size of the canvas. My understanding is that:

  • Zooming with ctrl +/- properly updates both devicePixelRatio and devicePixelContentBoxSize so that you can calculate the regular size from devicePixelContentBoxSize (except in Safari, actually, where devicePixelRatio is unchanged, but it doesn't have devicePixelContentBoxSize anyway so we can ignore that for now).
  • Pinch-to-zoom affects neither devicePixelRatio nor devicePixelContentBoxSize, and so pretty much has no effect on the page - it literally just zooms into the screen without touching anything else. I've tested that this is the case in desktop Firefox and Chrome, but I haven't been able to test it on mobile - is that where the problem comes from?

So, I think it should be perfectly possible to convert between devicePixelContentBoxSize and the CSS size using devicePixelRatio. Am I missing something?

@mcclure
Copy link

mcclure commented Jun 8, 2023

In my testing #2074 seems to work perfectly well with "normal" ctrl-+ browser zoom.

src/platform_impl/web/event_loop/runner.rs Outdated Show resolved Hide resolved
src/platform_impl/web/event_loop/runner.rs Outdated Show resolved Hide resolved
src/platform_impl/web/web_sys/canvas.rs Outdated Show resolved Hide resolved
@daxpedda
Copy link
Member Author

daxpedda commented Jun 9, 2023

I rewrote large parts of the OP because of a misconception I had about what exactly CSS pixels represent. Thank you @Liamolucko for pointing it out to me in #2858 (comment).

So, I think it should be perfectly possible to convert between devicePixelContentBoxSize and the CSS size using devicePixelRatio. Am I missing something?

Even after I rewrote the OP and cleared up any misconceptions I had, it's actually not possible to perfectly convert between devicePixelContentBoxSize and the CSS size. It might or might not be off by a pixel, which may have some very unfortunate consequences as pointed out in the OP, see this demo for example.

So I've reworked the PR three times now, any review is appreciated. In the meantime we will be waiting for a new version of web-sys anyway.

@mcclure
Copy link

mcclure commented Jun 10, 2023

Can I ask some questions about using this from an end-user perspective?

My usual way of interacting with Winit in browser is I create a window corresponding to a canvas (either full screen or in a layout) and then I draw to its entire extent with WebGL or WebGPU.

Naively, if the users uses CTRL-+ or CTRL-MINUS on desktop browser I would expect to continue to be told my width and height in actual screen pixels. But naively, if the user is on mobile, and they pinch to zoom in, I think I'd expect to be treated like an image and for my output to simply be rescaled. If (as in this patch) in that circumstance you instead "do the right thing" and continue to report my updating pixel size continuously, that's a nice surpriseβ€” I guess that means I can just keep presenting ever-higher resolution versions of my imagery. But it creates a problem.

If the user continues to zoom in on my window, very easily my window could wind up being several times the size of the screen. Mobile screens are already very high resolution. So resource costs to draw a frame could become very high, especially if I am doing multipass rendering (IE allocating an offscreen texture the size of the windowβ€” now potentially much larger than the screen), because I'm rendering lots of screen that is in fact offscreen.

Besides just tracking WindowEvent::Resized, is there any hope of being informed what the "visible area" of my window is in this scenario? (I'm not sure if this is something CSS/JS even expose.) That way I could draw only in the visible part of the screen. Alternately, can I get the screen size (I think browser tech calls this the "viewport") in winit, so I can cap out there?

@Systemcluster
Copy link

Besides just tracking WindowEvent::Resized, is there any hope of being informed what the "visible area" of my window is in this scenario? (I'm not sure if this is something CSS/JS even expose.)

You can use IntersectionObserver for intersection with the viewport, but I think that is outside of the scope of this PR.

@daxpedda daxpedda marked this pull request as draft June 10, 2023 21:00
@daxpedda
Copy link
Member Author

daxpedda commented Jun 10, 2023

Naively, if the users uses CTRL-+ or CTRL-MINUS on desktop browser I would expect to continue to be told my width and height in actual screen pixels.

Yes, that's what this PR does.

But naively, if the user is on mobile, and they pinch to zoom in, I think I'd expect to be treated like an image and for my output to simply be rescaled.

That is how it currently works in Winit already, pinch-to-zoom has no effect on Winit at all.

If (as in this patch) in that circumstance you instead "do the right thing" and continue to report my updating pixel size continuously, that's a nice surpriseβ€” I guess that means I can just keep presenting ever-higher resolution versions of my imagery.

There isn't a reliable way to detect pinch-to-zoom on Web. You could detect when two pointers are moving away or towards each other and use a bunch of hacks to calculate the zoom factor, but this isn't something really supported or reliable.

Besides just tracking WindowEvent::Resized, is there any hope of being informed what the "visible area" of my window is in this scenario? (I'm not sure if this is something CSS/JS even expose.) That way I could draw only in the visible part of the screen.

Winit has WindowEvent::Occluded, but that only tells you if it's completely occluded or not and isn't implemented on Web. Giving more information would require separate design work to cover other backends. Though getting this information will be complicated and inaccurate on Web.

Alternately, can I get the screen size (I think browser tech calls this the "viewport") in winit, so I can cap out there?

I'm not sure what you mean with that, that's only useful if the canvas is bigger then the window, which sounds niche to me, but no, we don't have an API that returns that. In any case, sounds like an extended version of Occluded covers the use case here.

@daxpedda daxpedda marked this pull request as ready for review June 11, 2023 16:04
@daxpedda
Copy link
Member Author

I had to redo the logic again to account for Safari not sending an initial resize event if the canvas has a size of zero.

The PR is ready again. Still waiting for a new web-sys release though.

@daxpedda daxpedda mentioned this pull request Jun 11, 2023
18 tasks
@mcclure
Copy link

mcclure commented Jun 12, 2023

I am attempting to re-orient my test code, which was previously running on top of PR #2074 , to run on top of this. It appears to work exactly the same as #2074 (including ctrl-+ zoomingβ€” I have not tested on mobile as my app doesn't work on mobile). Two points of confusion for me:

  • The previous iteration of this patch required the "css-size" feature. No such feature exists in the new patch. I guess css-size is just always on now?

  • window.canvas() now returns an Option instead of an HtmlCanvasElement. I assume this is due to upgrading winit rather than the PR?

@Liamolucko
Copy link
Contributor

I guess css-size is just always on now?

Yep.

  • window.canvas() now returns an Option instead of an HtmlCanvasElement. I assume this is due to upgrading winit rather than the PR?

Yep, that was already the case before this PR.

@daxpedda
Copy link
Member Author

daxpedda commented Jun 13, 2023

With the latest web-sys release this is now ready to be merged. I will let this stew a little bit longer and see if we discover any more issues or edge-cases.

I requested a review from @Liamolucko, if you want to review this please take your time, we are in no hurry.
Your input was invaluable so far, thank you ❀️.

Copy link
Contributor

@Liamolucko Liamolucko left a comment

Choose a reason for hiding this comment

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

All looks good to me!

Your input was invaluable so far, thank you ❀️.

No worries, I'm just happy to see this finally coming to fruition. Thank you also for all your work on this! ❀️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 participants