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-1] Proposal for more robust per element error handling #6185

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

Comments

@hilts-vaughan
Copy link

Proposal to augment ResizeObserver error handling

The problem

Today, ResizeObserver will throw an error that is emitted on window if a potential loop is detected / notifications were emitted again that could cause a loop. The event has the following characteristics that
are problematic:

  1. The target and currentTarget (at least in Chrome) is defined to be window and thus has no context of what thew
  2. The exception will trigger most default window.onerror handlers installed by various reporting systems today, such as Sentry for every single instance of this
  3. The error cannot be handled on a per element / observation level
  4. It assumes that more that a single iteration implies that the layout would not converge

Sometimes, this error is acceptable or even expected. A good example of this from things that change to be depending on their content size using negative margins / padding (the latter is solved in a later version of the spec to read padding boxes) This can change their width and thus, fire another event. However, there are other cases such as the one from MDN where the font-size of the item inside is being changed.

  1. Font-size changing example
  2. Open-source library that viewport changes
  3. Negative-margins for a "full bleed behaviour"

In all the above cases, it is expected that the layout will converge and stop delivering more notifications on the 2nd loop through since it has "converged" on a single layout choice (a new font-size, new width, etc). For example, in the case of the font-size change, there is a one:one mapping of size to font-size, and thus the callback is idempotent. Today, there is no way to actually acknowledge this in code and have a programmer annotate and let the browser know that the layout will indeed converge and that the later notifications are not needed.

We need a way for a programmer to programatically identify and handle these exceptions with enough information for the various use cases. Such cases would be:

  1. I expect my layout to converge, and a 2nd iteration isn't neccessary
  2. I didn't expect any additional notification and would like to know about them
  3. Provide enough flexibility that users can handle other cases without more intervention from the standards team
  4. The exception may be thrown from some 3rd party code on a global window that the user does not control

How users are dealing with this today

Drawing inspiration from Stackoverflow:

  1. Ignore the error all together @ 241 votes
  2. Wrap the change in rAF @ 41 votes
  3. Swallow the exception

It is clear that #1 and #3 is bad.

It is less clear that #2 is bad, but it is subtly wrong since the rAF would not run until the next frame, causing jank, and actually completely removing the "loop until settled" layout logic and causing a potential judder since it would not run until the next frame (since rAF running in an RO does not run until the next frame, since it is too late to run it for the current frame -- causing jank)

What we can do about it

Let's add an API to the callback on observer called allowUndeliveredFor(element : SVGElement|HTMLElement, {selfOnly: true|false = true}). That is,
the API would then look like this:

callback : (entries : ResizeObserverEntry [], observer : ResizeObserver, allowUndeliveredFor : (element : SVGElement|HTMLElement, {selfOnly: true|false = true}))

The semenatics of invoking the callback would be the following: when the developer invokes allowUndeliveredFor for a given element, that node would not be considered when gathering skipped notifications as "not skipped"

This would amend step 6 of the spec that reads:

If Document has skipped observations then deliver resize loop error notification

to become:

If Document has skipped observations that were not marked as "allowed" then deliver resize loop error notification

The definition of "allowed" is then any node marked allowUndeliveredFor (all it's descendants if allowUndeliveredFor has the flag selfOnly set to false) the node hasn't been revisited again by another observer since. This means that if you had two ResizeObservers that were tracking the same element -- you could ignore the error from either of them, however a notification firing another observer elsewhere would undo the internal set flag set by allowUndeliveredFor.

This is important since it prevents developers from supressing an error that they didn't intend to -- from within their own handler they know it's safe, but perhaps not others. It is still possible that something other than the RO handler changes the resizing in another observer, the developer must manually handle this.

You can find use cases below.

I expect my layout to converge on a single call

Consider a case where font-size is adjusted in the RO:

        const resizeObserver = new ResizeObserver((entries, observer, allowUndeliveredFor) => {
          for (let entry of entries) {
              h1Elem.style.fontSize = Math.max(1.5, entry.contentRect.width/200) + 'rem';
              pElem.style.fontSize = Math.max(1, entry.contentRect.width/600) + 'rem';
              allowUndeliveredFor(entry.target, {selfOnly: false})
            }
          }
        });

        resizeObserver.observe(container);

In this case, the user has specified that they expect that the target element will have changed in size and it's fine that it will finish with an undelivered notifcation.

It was also considered to add a new callback that would be invoked with a set of all the undelivered notifications right before the delivery step. It could be summarized as:

        const ignore = new Set();
        const resizeObserver = new ResizeObserver((entries, observer) => {
          set.clear();
          for (let entry of entries) {
              h1Elem.style.fontSize = Math.max(1.5, entry.contentRect.width/200) + 'rem';
              pElem.style.fontSize = Math.max(1, entry.contentRect.width/600) + 'rem';
              set.add(entry.target);
            }
          }
        }, (undeliveredNotifications : UndeliveredNotification[], allowUndeliveredFor...) => {
          // UndeliveredNotification {
          //   target : Element;
          // }
          for(const notification of undeliveredNotifications) {
            if(ignore.has(notification.target)) {
              allowUndeliveredFor(entry.target)
            }
          }
        });

        resizeObserver.observe(container);

However, it is more clunky than the above option. It would also have to be per observer. However, it does allow you to post-process more easily if the elements to ignore depends on the final set of undelivered notifications.
Thus, it may be more desirable to implement the latter or both, even if slightly more clunky and leave it up to library authors to provide a good abstraction over top of it.

Users may just ignore everything

This is signficantly harder to justify in this case -- since it has a named semantic meaning and you have to do it for each element. Some users may still do this but even so, it is better than supressing the entire error.

Users will naturally ignore everything when they don't understand the issue and have no other escape hatch.

I didn't expect an additional notification

This is trivial, by not invoking the callback then the default is to continue to deliver the notification.
You can then handle it like you do today.

Provide flexibility to handle without more invervention

You can technically handle any level of errors; if you would be fine with ignoring all errors
then you could mark every node on the DOM tree down by labeling it with selfOnly: false very explicitly
indiciating that you are fine with ignoring everything down.

With this, users can make it very clear about what is an expected error and what is not at the granular node level which is then level at which an undelivered notification would happen.

The exception may be thrown from some 3rd party code on a global window that the user does not control

This is harder to deal with -- since JS errors can be thrown all the time in extensions and other things that developers don't control. A simple proposal that would fix this would be to change target to be an element that had an undelivered notification to assist in filtering it out but this change is seperate from allowing developers to avoid throwing their own errors in the first place.

However, since multiple can have undelivered notifications a new field may be needed, such as undeliveredElements and users could check against this if they really wanted to know.

I've omitted a detailed design on this since it is less critical than allowing users to handle their own in my eyes, but I think we could handle it in another proposal. Doing so would give users more information as to what element was the one that was resized as well and make for easier debugging.

@hilts-vaughan
Copy link
Author

cc @fantasai for the tag; if you can provide any guidance about getting the above reviewed, I would appreciate that as well.

@hilts-vaughan
Copy link
Author

It dawns on me that selfOnly: true has overlapping implications with other observers and could have a high technical level of complexity. It may be best to remove this completely and simply have the expectation that users can maintain a list of elements since they originally "observed" them in the first place and simply iterate this list -- it is much cheaper than probably enumerating the entire descendant tree and determining them like that + avoids the overlap issue.

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

2 participants