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

[resize-observer] Consider a mode to indicate that a single pass is all that is needed / perform double settlement pass #6173

Open
hilts-vaughan opened this issue Apr 2, 2021 · 9 comments

Comments

@hilts-vaughan
Copy link

Spec: https://www.w3.org/TR/resize-observer/#deliver-resize-error

I am sure many of us are familiar with the spec, but the tldr; is that for various reasons, if you change the element you're observing during a ResizeObserver event in such a way that it's box changes, then in the next very tick when doing dirty checking it will again be marked for a "change". This deficiency has been known for quite some time, you can see early asks about it here: https://groups.google.com/a/chromium.org/g/blink-dev/c/z6ienONUb5A/m/lDU3VppIAgAJ

However, this is actually desirable at times. For example, in #5488 (comment) I outline an example from MDN that does some cool stuff with font-scaling: https://jsfiddle.net/Lxzmh40u/ -- it's the very example that shows off the observer!

I also have some examples where I have a component that I want to make "more compact" when it gets into a container that is too small. This probably means shrinking some padding, maybe reducing the font-size a tiny bit in some cases, perhaps applying negative margins, which changes the width.

Another concrete example, per the spec:

Observation will fire when observation starts if Element is being rendered, and Element’s size is not 0,0.

This is perfect for you have a web component hidden behind a tab that is not visible. Then, once it comes into view you can do your JS work to get it looking "right" for the container, this might involve once again adjusting it or making it more "compact" to get the job done.

You can find an example here that I just wrote: https://jsfiddle.net/cq4oh6f1/19/

The example is contrived since it could be done with a media query as-is since we only have a single thing, but imagine this is a component being placed into some random page and it's very information dense and we want it to be fully responsive. And of course, I could easily fix it by specifying border-box as the box-sizing model -- but that is not practical in all cases and there is more than just padding you can change to change an elements size; perhaps sometimes you want to extend the height just a little bit since the width has gotten too small.

Negative margins are another, they can change the width of the box since they're not included in border-box. This i s what happens with virtuoso actually; you can find their source here: https://github.com/petyosi/react-virtuoso/blob/1ee6bff1fc2b9d5e61f03271df0f3ff95870882a/src/hooks/useWindowViewportRect.ts

In their case, a single tick of the error is all they get, and there is no infinite loop, which you would expect if the next "change" that gets delivered next frame would cause the change again.

The following solutions are presented on the mailing list and around the web:

There is no 1 size fits all. I think the answer is:

- if your component is changing in response to window resize, post an idle task, DOM is pretty messy during window resizes, no one will notice.

- if you are animating the component, be careful with component design. Either design it so its response to resize is fast enough and skip idle task, or post an idle task but draw your component so that bad layout will not be jarring to the user. (

- the two-years from now answer is use CustomPaint instead :-)

"There is no 1 size fits all. ", agreed.

  • Putting things into window resize isn't practical all the time -- if you are resizing something coming back from a change in a visibility, you have to know about it. If the user clicks a tab, then the visibility changes and if you just RAF it, you get some jank. ResizeObserver is perfect here -- you can do a sync. update and change styles before paint. However, if you resize (this may be the case for example if the state of things have changed since your component came back into view, for example, it's container has been resized) then some internal bookkeeping may have to be performed.

  • Either design it so its response to resize is fast enough and skip idle task

  • Do it in RAF
    People can catch 16ms flickers. This is where I've noticed this, not a good solution IMO

You get a jank frame. #5488 (comment) has a ton of examples of misunderstanding, but a real life one that is recent here is from Virtuso as well: petyosi/react-virtuoso#269

The video there shows what happens; users will notice.

How can we fix this? I am not positive but I can propose the following things to get the discussion started:

  1. To begin with, let's not trigger every bug logging libraries handler by default when we run into these scenarios: [resize-observer] Should "resize loop error notification" be a warning instead of an error? #5488. If we solved this, folks would be less irate with the issue overall, since it would be a simple nuissance "warning" instead of worming it's way in. Still, we want a way to capture and find out if we're dropping actual observations vs. ones we don't want to -- thus...

Let's find a way to fix these cases, so that developers can rationalize when they see a warning that "this is something I should look into, some resize events have been dropped and that's bad because jank will happen instead of "better filter these out"

Ideas:

  1. Let's define a mechanism to instruct the engine that it's OK to bail after a single dispatch. To options when performing an observation, we could add deliverOnce that would instruct that handlers running from this will only run once and will not perform layout rechecks. Developers would be responsible for understanding when they turn on this flag on that it means that the layout won't be rechecked after their callbacks run. For many of the above cases, this is desirable and perfectly fine. You would turn this on because you know it's a safe optimization to make.

Counter arguments:

  • But then users may miss some updates and not realize what's going on. This can be solved with good documenation; this should not be a default. A user should understand that when they turn this on, the subsequent run should not change the state again or cause other cascading changes on the same element, and if it does, you're on your own. It will be delivered next frame; no worse than what people are doing today with RAF or simply ignoring the error.
  1. We could also change the bail mechanism to be O(2N) instead of O(N) with the following algorithim adjustment to the specification (3.4.1. Gather active observations at depth):
  • If targetDepth is greater than depth then add observation to activeTargets.
  • If targetDepth is greater than depth OR (targetDepth is equal to depth AND this observation.target has not been visited at "depth" already) then add observation to activeTargets.

The language could use with some wordsmithing but the outcome is the same:

  1. User makes change such as one the ones above.
  2. User has the same node checked again once more, and if it 'settles', in that no other change is detected, then it it won't trigger another notifcation and business will go on like usual

Bad stuff:

  • The runtime complexity is now 2N: Yeah, but if you don't need this behaviour nothing will change. In addition, something that isn't going to spam updates constantly is likely going to read the width / height / box dimensions and immediately realize that nothing else has to be done and so it won't do expensive DOM write / read operations. And of course, many things won't even be a bother.

Perhaps even both could be done in tandem; there are other proposals that could be done to get more value from this, though, but I think that sums up my feelings.

Happy to hear thoughts.

@hilts-vaughan
Copy link
Author

cc @astearns since they were active on the other issue as well.

@hilts-vaughan
Copy link
Author

cc @atotic as well -- I know you've not been involved in resize-observer for some time but it would be good if you had any input about why maybe this wasn't done before (or gaps I may have missed), and also about how it's reported in browsers: #5023

There has been literally thousands of codebases checking for the errors / suppressing them and dozens and dozens of Github issues -- so anything you can do to help move this along would be greatly appreciated! :)

@atotic
Copy link
Contributor

atotic commented Apr 2, 2021

Preface: ResizeObserver v2, when used correctly, should make this problem a lot less common. v1 only allowed you to observe content box, so changing own border/padding triggered the error. In v2, you can observe either a content box, or border box.

#5023: csswg has already taken a look, but there has not been a resolution. Standard processes can take a while. If you are very passionate about this, I'd suggest coming up with a detailed proposal. This will give csswg a cleaner path towards resolution. Something like this:

  1. Why this is a problem.
    Problem description, something like: RO error handling makes it hard for developers to use RO correctly. Some errors can be completely ignored, other should not be. There is no way to tell inside the error handler which ones are which.
    Examples of the problem in the wild: how are the developers handling it not by fixing the core of the problem, but doing inefficient things: "they just ignore all RO errors", "work is delayed to rAF". These examples should demonstrate why is the existing API failing.

  2. Proposal for fixing the problem
    Turning this into a warning is probably a non-starter. windows.onwarning API does not exist, and all the problems inherent in this being an error still remain. I have not seen any proposal that made me think "yeah, we should do that!". Maybe something like ResizeObserver.ignoreError() that gets called inside the RO event handler? You'd probably want ignoreError to be selective (ignore only errors underneath me?)

  3. Show how this proposal would fix problems shown in 1)

  4. Bonus points: polyfill showing off the solution today

@hilts-vaughan
Copy link
Author

hilts-vaughan commented Apr 2, 2021

Thanks @atotic. I agree that v2 makes it easier to work with but still doesn't address things like "I made my height bigger in response to being squeezed width wise" or "I use negative margins", but they are in use by many web apps today (and unfortunately why I am now faced with the dilemma of finding resolutions now...) My understanding is correct, yes? The border-box solution simply solves the "I made my padding and/or border-box more compact to accomadate my size and and my real size didn't actually change" problem, right?

As for #5023, I don't mind taking a look but it will likely take some time for me to figure out what's best here. Is it so bad to just simply print a verbose warning to the console like Chrome does today and call it a day? ;) I know these things are nuanced.

I think you have the gist of it though, we need a way to selectively configure the notifications being dropped. Part of this comment ("Let's define a mechanism to instruct the engine that it's OK to bail after a single dispatch") is what I was trying to express in many words, that you did in a few. This problem is a lot invasive if on the corner cases a developer had a way of just saying "yeah, I can guarantee the layout will have settled and the dropped notification is no big deal"

I appreciate you taking the time to reply.

@atotic
Copy link
Contributor

atotic commented Apr 2, 2021

Is it so bad to just simply print a verbose warning to the console like Chrome does today and call it a day?

Console warnings are outside of the spec. The error warning was explicitly designed to get developer's attention. The theory behind it was that without the error, it would be easy to develop badly performing code.

We've had some hallway conversations about this. The core of the problem might be:

  • sometimes RO limit error is acceptable, and there is no way for web developers to distinguish between acceptable and unacceptable errors inside the error handling routine.

What can we do about it? Some options are:

  1. Provide "suppressNextROError" function inside their RO callback which suppresses next error.
    Pros: easy for developers to do.
    Cons: easy to just always call it, and completely ignore this error.

  2. Provide "annotateNextError(msg)" function inside RO callback which would add msg to the error string. Developers could use this to filter out errors in the error reporting routine.

@hilts-vaughan
Copy link
Author

hilts-vaughan commented Apr 4, 2021

Console warnings are outside of the spec. The error warning was explicitly designed to get developer's attention. The theory behind it was that without the error, it would be easy to develop badly performing code.

Yeah, that is understandable. I can see why you would want to get the users attention since it is possible that the errors are a serious problem. I took mine very seriously as well until I fully understood what was going on; I think we definitely want to allow for that level of awareness still.

sometimes RO limit error is acceptable, and there is no way for web developers to distinguish between acceptable and unacceptable errors inside the error handling routine.

That is basically correct. The common solution today is silence everything; since it's much cheaper and "mostly correct" since an infinite loop is often "obviously" wrong (though, not always)

Aside from your previous suggestions (which I think are good; though I lean more towards a suppression callback). I hinted at this above with the flag, it's just a different level of abstraction that it's done at. Honestly, I think it's the most flexible option and would allow us the most accordance to do the "right thing" but would require more education on the scenarios that it's acceptable.

The most common scenario where you want this to be "OK" is I think the case where you did a self modification and it won't trigger any other changes. This makes up the majority of the cases I've seen, though I admit my exposure is a single one at the companies I've worked.

What is your take on:

We could also change the bail mechanism to be O(2N) instead of O(N) with the following algorithim adjustment to the specification (3.4.1. Gather active observations at depth):

from above? To try and simplify it, we would basically check allow the callback to run again on the same node once more to accommodate the fact that sometimes it may settle after a 2nd iteration. I don't love it because:

  1. It's obviously more expensive; specially if the operation doesn't terminate / settle
  2. It's a breaking change technically because we would begin running users code more than once and this could potentially introduce timing related bugs.

Still, it is appealing in it's own way because it's basically like saying "do a 2nd pass with an iteration limit of N = 2", which AFAIK from the original repo notes (from what I can tell it was talked about here: WICG/resize-observer#7)

The con listed there "cons: limit is arbitrary." is legitimate, but if N = 2 then it's less arbitrarily, it's basically "did the layout converge on an acceptable state?"

So, it is tempting since it would reduce developer friction and make it do the "right thing" from time to time. To make it a non-breaking change, I guess it could be an optional flag.

As someone that did the original implementation I would love your thoughts on this and if it was talked about (I am sure it was!) -- I don't mind doing the leg work of fleshing out the actual proposal, I just want to ensure I have the facts right and my naive interpretation isn't bad.

If we allow this, I think you can even make the argument that "error" is actually the correct thing to do in this case, because a layout that did not converge after a single pass is probably very definitely not performant and atypical. We should still probably tackle that problem -- but it would at least make it more obvious to developers what you're doing "I am not simply silencing it; I am simply allowing another single iteration and I can only do that if I understand what what my code is doing and why that is important"

@atotic
Copy link
Contributor

atotic commented Apr 4, 2021

Sorry, I do not have any more time to dedicate to this, so this is my last reply. I'd suggest to write up a descriptions of the problem like I've suggested few comments above (maybe a new issue, or maybe here), and try to get it on csswg agenda.

N=2 is an interesting proposal. My worry about it is that it is solves some percentage of the cases. What percentage is this? Are there valid use cases it does not solve? Should we go with a more general mechanism? In standards, solving a problem so that we do not have to talk about it again is preferable.

@hilts-vaughan
Copy link
Author

atoic@, ACK thanks for your time! I will do as you propose; the committee can then review it.

@hilts-vaughan
Copy link
Author

#6185 adds a proposal to make these use cases tenable (for those interested -- atotic@ -- I respect that I've used up enough of your time!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants