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] Which box information should we pass to the callback #3329

Open
gregwhitworth opened this issue Nov 16, 2018 · 21 comments
Open

Comments

@gregwhitworth
Copy link
Contributor

We now allow authors to determine which box(es) they wish to observe on layout changes. Now we need to discuss which boxes we provide the information on. We have a few options:

Take the following example: If the author sets content-box and border-box to be observed, which box dimensions would you expect to receive passed to your callback?

  1. The box(es) that were requested to be observed: so in the example above, content-box and border-box information
  2. They receive all available CSS boxes that are observable: They will receive border-box, content-box, scroll-box dimensions.

Option 1 Pros is good because it makes sense that I requested a specific set of boxes to be observed and those are the ones that you returned information on.

Option 1 Cons If the sets the observer to the border-box but desires to know something about an inner box, they may request it in some other manner causing us to do additional work which defeats the purpose of this API altogether.

Option 2 Pros You can set one observation and get information about all of the boxes, so if you have a scenario in which you want to observe the border box but check the content or scroll box - this is possible without having to observe them.

Option 2 Cons It clutters up the response, if I only care about the border-box but I get results for every other box this may be a nuisance.

My personal opinion is to go with option 2 as I think people will either over observe to get the potential rects they care about or will ask for them in some other manner. That said, I don't have a strong opinion about this as I think it will be user preference. I have a twitter poll going about this in hopes of crowd sourcing some thoughts from web devs.

@atotic
Copy link
Contributor

atotic commented Nov 16, 2018

My vote is also for #2, report all boxes.

Observing content-box and border-box at the same time increases likelihood of "undelivered notifications error". We do not want developers observing multiple sizes just to be able to read their values out of the ResizeObserverEntry.

@gregwhitworth
Copy link
Contributor Author

increases likelihood of "undelivered notifications error"

Then what is the reasoning behind allowing a sequence of options to be passed in? Maybe we should limit it to only one option being passed in as to avoid this issue?

@atotic
Copy link
Contributor

atotic commented Nov 26, 2018

Then what is the reasoning behind allowing a sequence of options to be passed in? Maybe we should limit it to only one option being passed in as to avoid this issue?

Good point. I think single option will be vast majority of usage. For the rare case of multiple sizes being observed, developers can use multiple ResizeObservers.

@gregwhitworth
Copy link
Contributor Author

Good point. I think single option will be vast majority of usage. For the rare case of multiple sizes being observed, developers can use multiple ResizeObservers.

To be clear here - you're in agreement that we should change this:

dictionary ResizeObserverOptions {
    (ResizeObserverSizeOptions or sequence<ResizeObserverSizeOptions>) size = "content-box";
};

to this:

dictionary ResizeObserverOptions {
    (ResizeObserverSizeOptions) size = "content-box";
};

@atotic
Copy link
Contributor

atotic commented Nov 27, 2018

Yes :-)

I started off against the change. Then I wrote down my arguments for allowing multiples, and they were not convincing. So I changed my mind, and now agree with you.

@gregwhitworth
Copy link
Contributor Author

To summarize, the change that @atotic and I made is to be able to observe and single box at a time with all boxes being returned on callback.

@gregwhitworth
Copy link
Contributor Author

To be very clear, here are the boxes whos dimensions would be returned:

  • borderBoxSize
  • contentSize
  • scrollSize
  • devicePixelBorderBoxSize

@TremayneChrist
Copy link
Member

To be very clear, here are the boxes whos dimensions would be returned:

  • borderBoxSize
  • contentSize
  • scrollSize
  • devicePixelBorderBoxSize

If we can observe border-box, content-box, scroll-box and device-pixel-border-box, shouldn't all property names also include box in the name?

  • borderBoxSize
  • contentBoxSize
  • scrollBoxSize
  • devicePixelBorderBoxSize

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Which box information should we pass to the callback.

The full IRC log of that discussion <dael> Topic: Which box information should we pass to the callback
<dael> github: https://github.com//issues/3329#issuecomment-452477429
<dael> gregwhitworth: This comes down to we changed the spec because we resolved we wanted to watch more boxes. THen it was what information should we return? Author wants to observe border box, what do we return? We previously stated all that you could potentially observe so that we answer anything the author wants to watch.
<dael> gregwhitworth: WE don't want author putting multi observers to watch multiple boxes.
<dael> gregwhitworth: What would happen is in the issue. We would return border box, content size box, scroll box, and device pixel border box.
<dael> iank_: I'm observing border box size and the content box size changes, am I notified?
<dael> gregwhitworth: No. If you want the content box you should observe it
<dael> iank_: Worries me webdev will observe border box and then complain it's not working as expected. Did you consider getting people to list all boxes and nulling things not needed?
<dael> gregwhitworth: Initially we were going to allow them to observe multi. We can add an option to return us what you want to return us. We can add an option that says of the ones we have which do you want options returned.
<dael> gregwhitworth: It's kinda a shot in the dark right now. Even if they have the option they could still have the same problem of observe border and dimensions on content
<dael> iank_: Border box size would be null in that call
<smfr> q+
<dael> Rossen: Verbosity of proposed API. The entity in the platform is the css box. All other boxes are properties of that object. My worry is if we go down path to expose such models where you are creating an identity for a property you're complicating lifetime of the API. Sounds a bit odd to me to go down that path
<dael> Rossen: Almost eq. of pretending any other property of box is a scriptable object
<astearns> ack smfr
<dael> smfr: One of the things I find odd is that we don't have any other APIs that return content box stuff. It seems odd to get these boxes is only resize observer. Why not expose on the things that are an imparitive to get boxes
<dael> smfr: Also odd API only returns sizes. I'd like rect for all of these and they should be relative to top left of border box
<dael> smfr: To resolve what changed is you return all rectangles and an enum to say which changed
<dael> astearns: Would be which changed, it's which of them changed
<dael> smfr: Yes
<dael> gregwhitworth: Not opposed to having results return in other static areas. THis was just an issue for the resize observer. Not sure if you want to expose in CSSOM or whatever.
<dael> gregwhitworth: smfr are you saying youw ant shape is what would be return from getClietnRects? I had talked with Alec (sp?) that we'd have getBoundingClientRects be what's returned for these boxes. But we said it's about size change not position
<dael> smfr: These rect are in local coordinates. Which I think is fine. This API shouldn't be able abspos or client pos. If you're giving size of contents I'd also like offset from top left. That seems natural and cheap to compute
<dael> gregwhitworth: Makes sense. Ic an open that issue. Are we providing for sake or is that a thing authors want commonly? That they want to check the offset
<dael> smfr: I would imagine the users care about fitting thigns inside. I think it's about size and not position
<dael> gregwhitworth: hen why include offsets? Just in case?
<dael> smfr: If you tell me content size I might want to know how much is border and how much padding. If I have to call gCS it might trigger a layout which is expensive so that should be in resize observer
<dael> smfr: One final piece of feedback on algo, but we can come back to that
<dael> gregwhitworth: That is good F2F item.
<dael> gregwhitworth: Rossen to your point on model, any other rec. on achieving the end case?
<dael> Rossen: If we're going to discuss at F2F I'd defer to then. Providing all the information in a property bag that doesn't require additional readbacks would be the preferred model.
<dael> Rossen: The misalignment seems a bit odd here
<dael> gregwhitworth: Okay
<dael> Rossen: Let's table and discuss at length at F2F
<dael> astearns: I'll add F2F tag

@bfgeek
Copy link

bfgeek commented Jan 23, 2019

My feedback on the call was something like:

ro.observe(elem, {box: ['content-box', 'border-box']});
ro.observe(elem2, {box: 'content-box'});

function callback(entry) {
  // elem2
  entry.borderBoxSize === null; // This entry null'd out as not explicitly observed.
}

@gregwhitworth
Copy link
Contributor Author

@bfgeek There would need to be two different option sets, because we're trying to keep the API shape simple - what you have defined right now would create two tasks of the content-box and border-box which, as noted by @atotic in this comment that this can lead to missing notifications. In order to achieve what you want and avoid the missing notifications is to do something like this:

ro.observe(elem, {
                  boxToObserve: 'content-box', 
                  boxesToReturnResultsFor: ['border-box', 'content-box']
                  });

This would allow the author to trigger on a specific box's dimension changing and which dimensions the author wants returned.

@gregwhitworth
Copy link
Contributor Author

@TremayneChrist Thanks - Opened a seperate issue to track that

@atotic
Copy link
Contributor

atotic commented Jan 24, 2019

Had a F2F chat with @bfgeek on this topic. For the record,
here is a braindump of some of my thoughts, and our discussion:

Both sides agreed that the goal of this part of the API is to help developers write better code.

Lets call code that runs inside RO callback "RO callback algorithm". The code inside this algorithm:

  • might want to modify the Element, or its children.
  • the only geometry data considered by this algorithm should be observable data. If your algorithm depends on unobserved data, and unobserved data changes, algorithm would be out of sync.

Out of sync example: algorithm looks at borderTop, paddingTop, to absolutely position child near the top. When border/padding changes, child would no longer be in the right spot.

What can be safely modified inside callback depends on Element's box-sizing, width/height properties, and observed box.

There are 2 main geometry-changing use cases we've imagined:

  • modify Element's border/padding
  • modify children (add/remove/place).

Here a chart on how interaction of CSS properties affects what can be safely
modified inside the callback:

width/height box-sizing observed-box modify border/padding modify children
<length> border-box border-box safe safe
<length> border-box content-box not safe safe
<length> content-box border-box not safe safe
<length> content-box content-box safe safe
intrinsic border-box border-box not safe not safe
intrinsic border-box content-box not safe not safe
intrinsic content-box border-box not safe not safe
intrinsic content-box content-box safe not safe
extrinsic border-box border-box safe safe
extrinsic border-box content-box not safe safe
extrinsic content-box border-box not safe safe
extrinsic content-box content-box safe safe

You can see from this chart that interaction of observed boxes, and Element's CSS
properties constrain what can be safely modified in interesting ways. It'll
come in useful when discussing the question "is it useful to observe multiple
boxes?". I think the answer is no.

Back to our discussion, and the original question:

Should we report only observed size, or all sizes?

I argued that we should report all sizes to prevent webdevs from shooting
themselves in the foot by using multiple ResizeObservers to observe
more than 1 size on a single Element.

@bfgeek argued that using non-observed sizes inside the callback is fundamentally
wrong, and that we should not encourage it.

I was convinced, and now I am in the "observe only one size, and report only one size" camp.

@TremayneChrist
Copy link
Member

So now we would have something like this?

const ro = new ResizeObserver(entries => {
  entries.forEach(entry => {
    console.log(entry);
    // { target: Element, size: ResizeObserverSize }
  })
})

@atotic
Copy link
Contributor

atotic commented Jan 24, 2019

So now we would have something like this?
// { target: Element, size: ResizeObserverSize }

No. Our current thinking is to keep all reported sizes in ResizeObserverEntry, but only reported one would be non-null.

This reminds me of an exception I forgot to mention in my previous comment.

"Only one size can be safely used at a time" rule has an exception. If "device-pixel-border-box" is being watched, "border-box" size is useful, and can be used safely.

Example: in response to "device-pixel-border-box" size notification, Element would be resized to "border-box" size, and canvas context would be resized to "device-pixel-border-box".

@gregwhitworth
Copy link
Contributor Author

Ok, so after discussing this at length with @atotic and taking into account the valid feedback from @bfgeek regarding stale information we've come up with two potential shapes this API can take. The question comes down to:

Do we only return the information for the box being observed, or do we return all potentially observable boxes but only fill in layout information of the box actively observed?

Option A:

entry = {
        target: <Element>,
        contentRect: {/* v1 for back compat */},
        inline: 400,
        block: 400
 }

Option B:

entry = {
        target: <Element>,
        contentRect: {/* v1 for back compat */},
        contentBox: null,
        borderBox: {
            inline: 400,
            block: 400
        },
        scrollBox: null
    }

Note: This does not show offset information by design as that will be discussed in issue #3550 - if we resolve to add the offsets those will go alongside inline & block, no matter which shape we decide on.

Note 2: How fragmentation will be supported is being looked at in issue #3673 and we will adjust the shape further based on the outcome of that discussion

@TremayneChrist
Copy link
Member

Hey guys, just thought I'd share my initial feelings on both options.

Option A

Pros:

  1. Looks cleaner.
  2. Properties can always be expected and have value.
  3. Less nesting of values.
  4. Reduces complexity of callback handlers.

Cons:

  1. Confusion on which box is being returned.
  2. Potentially less flexible.
  3. Could restrict API extension in the future, or, start to clutter the global scope of the entry.

Option B

Pros:

  1. Value is tied to the observed box, reducing confusion.
  2. Potentially gives more flexibility for libraries using the API.

Cons:

  1. Gives the impression that you can observe multiple boxes, using the same observer. I assume not, but maybe you can?
  2. Potentially adds more logic to the observer callback, for value checks.

@TremayneChrist
Copy link
Member

Also, just to clarify...

  1. So devicePixelBorderBox is being scrapped?

  2. Sticking to the term box instead of size? I guess this is to support offsets if required ([resize-observer] Add offset position to rects #3550)?

  3. What happens if this occurs?

const ro = new ResizeObserver(() => {});

ro.observe(document.body, { box: 'border-box' });

// This one will be skipped?
// or will contentBox and borderBox be watched and returned?
ro.observe(document.body, { box: 'content-box' });

@gregwhitworth
Copy link
Contributor Author

@TremayneChrist We're going to be discussing all of these options today in a breakout session at the CSSWG meeting - all of them will hopefully lead to resolutions for each that are very clear. We'll have clear resolutions (if we get them) by tomorrow EOD in the minutes that get appended via the IRC bot.

What happens if this occurs?

Regarding your current question - that's a solid one but I would expect it to behave the same as other observers which effectively the latest observer's config. Using mutation observer as an example:

https://jsbin.com/cogusojiwo/edit?html,js,console,output

Here you can see that I am both adding a new element and adding an attribute but the callback only fires on the attribute. So I would expect RO to behave the same way and you would only get an observation of the content-box here. Also, you're not going to want to watch numerous boxes of the same element, which is why we removed the capability to pass in an array of boxes to observer as it can lead to an increase of missed events. @atotic just in case RO behaves differently from MutationObserver for some reason.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed ResizeObserverEntry shape, and agreed to the following:

  • RESOLVED: Option B, and remove scrollBox
The full IRC log of that discussion <heycam> Topic: ResizeObserverEntry shape
<gregwhitworth> Github: https://github.com//issues/3329
<gregwhitworth> https://github.com//issues/3329#issuecomment-466737070
<heycam> gregwhitworth: two options for the shape
<heycam> ... during the fragmentation discussion we might impact this
<heycam> ... the important part is that you can now watch N different types of boxes
<heycam> ... should we only pass back the dimensions of the dimensions of the box being observed? or provide the dimensions of other boxes as null?
<heycam> ... so inline/block would be the dimensions of the observed box. that's option A
<heycam> ... option B has separate properties for each box you could have observed
<heycam> ... benefit is that you can do e.g. `if (entry.borderBox)`
<heycam> ... overall most web devs I asked prefer B
<Rossen> heycam, is it possible to observe multiple boxes with the same observer?
<Rossen> gregwhitworth, no
<heycam> heycam: could also have a type property with values like "border"
<heycam> ... rather than having multiple properties where only one will ever be non-null
<heycam> gregwhitworth: the polls I did were like 65%-35%, 80%-20%
<heycam> ... so majority wanted option B
<heycam> iank_: only thought about the .type property is that often people don't check types
<heycam> ... option B does give you stronger type safety
<heycam> alex: option B could break easily
<heycam> florian: which is what you want
<heycam> Rossen: any other options?
<heycam> gregwhitworth: no
<heycam> ... what are the thoughts about the Houdini Box API
<heycam> Rossen: I mentioned last time, wedon't necessarily have Box structures for those things
<heycam> ... not that we couldn't have an identifier that has this
<heycam> ... but I'm curious what the lifetime expectaitons are
<heycam> ... if you take a reference to one of these boxes, and hold on to it
<heycam> ... and the element is removed from the DOM tree, or it's mutated in a way that this box no longer exists
<heycam> ... what are the expectations here?
<heycam> iank_: these aren't live objects
<heycam> ... we create a new one each time
<heycam> gregwhitworth: the observation life cycle takes care of this
<heycam> alex: the life cycle of resize observer ... the stuff we already released deals with the life cycle and it's complex
<heycam> ... the question is, what keeps the resize observer alive
<heycam> Rossen: my question is, in one case we're just returning pure data
<heycam> stefan: the entry has a reference to the element
<heycam> ... so even if the element is removed from the DOM, it still has a reference to the element
<heycam> iank_: we gather up all the obserations, and we snapshot at that time
<heycam> stefan: even if you have an element removed from the DOM, you still have a reference to the object
<heycam> smfr: CSS pixel units?
<heycam> gregwhitworth: we have an issue tomorrow to discuss device units
<heycam> [some discussion about the downsides of option B shape]
<heycam> gregwhitworth: the reason this API exists is to watch dimensions to do container query like things
<heycam> ... did width change? fire it
<heycam> smfr: what about observing scroll box on something not scrollable?
<heycam> gregwhitworth: not sure if scroll box is heavily defined
<heycam> Rossen: not *heavily*
<heycam> florian: which scroll box are we talking about?
<heycam> Rossen: say you want to observe the scroll box only of an element, and it's not scrollable -- let's say it changes overflow value
<heycam> florian: "scroll viewport" is defined, "scroll box" is not
<heycam> Rossen: are we talking about the scrollable box, the extent of all your contents, which you could scroll if you were scrollable?
<heycam> [some whiteboard drawing]
<heycam> Rossen: if this was just added there without a use case, we could just remove it
<heycam> gregwhitworth: I'm leaning towards that
<heycam> Rossen: so would dropping scroll box make people happy?
<heycam> stefan: only opinion is if if have it, it should be either the scroll port box or the scroll box that include the padding
<heycam> alex: the use case for scroll box was a chat box
<heycam> ... you always want to keep the the bottom, and if you are loading images you might want to scroll down
<heycam> smfr: scroll to MAXINT
<heycam> florian: scroll snapping will do that for you
<heycam> alex: how do you know if you've scrolled?
<heycam> stefan: just do it any time you get new content
<heycam> florian: scroll snap will do that for you declaratively
<gregwhitworth> Proposed resolution: remove scrollBox
<gregwhitworth> Proposed resolution: remove scrollBox from the observable boxes
<heycam> RESOLVED: Option B, and remove scrollBox

@gregwhitworth
Copy link
Contributor Author

To summarize the above - is to go with Option B with the removal of scrollBox - and taking into account fragments of course - so the shape will now look like this:

entry = {
        target: <Element>,
        contentRect: {/* v1 for back compat */},
        contentBox: null,
        borderBox: [{
            inline: 400,
            block: 400
        }]
    }

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

6 participants