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

Review request for ResizeObserver #187

Closed
3 of 5 tasks
atotic opened this issue Aug 10, 2017 · 17 comments
Closed
3 of 5 tasks

Review request for ResizeObserver #187

atotic opened this issue Aug 10, 2017 · 17 comments
Assignees
Labels
Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review

Comments

@atotic
Copy link

atotic commented Aug 10, 2017

Hello TAG!

I'm requesting a TAG review of:

Further details (optional):

Previous discussions/implementations:

Many of the interesting design decisions have been discussed on ResizeObserver github issues.

Chrome has implemented ResizeObserver behind a flag.
Firefox has worked on ResizeObserver implementation.
ResizeObserver has been discussed at TPAC meeting in Lisbon 2016.

We'd prefer the TAG provide feedback as (please select one):

  • open issues in our Github repo for each point of feedback
  • open a single issue in our Github repo for the entire review
  • leave review feedback as a comment in this issue and @-notify [@atotic]
@dbaron
Copy link
Member

dbaron commented Aug 15, 2017

@slightlyoff
Copy link
Member

In future it might be helpful to request API design feedback more than a few weeks before your intended branch cut; any changes we propose seem unlikely at this late date.

@dbaron
Copy link
Member

dbaron commented Aug 15, 2017

One thing that came up in today's TAG meeting was the relationship to the rendering pipeline description in the HTML spec (how this spec is patching it), which is also related to our #72.

@plinss plinss added this to the tag-telcon-2017-08-22 milestone Aug 15, 2017
@atotic
Copy link
Author

atotic commented Aug 15, 2017

@slightlyoff sorry about the short notice, I was unaware of the TAG until this week. We can delay the release...

@dbaron side note about rendering pipeline: I maintain an informal version of "what should happen in the event loop" explainer: https://github.com/atotic/event-loop/, with some "what actually happens" results https://rawgit.com/atotic/event-loop/master/rendering-events.html

@slightlyoff
Copy link
Member

Questions!

  • Why aren't the new & old sizes provided by the API directly? Is it always going to be cheap to call window.getComputedStyle() within the callback to get the new sizes?
  • The spec includes non-normative text along the lines of observation will fire when observation starts if Element has display, and Element’s size is not 0,0, but this seems to ignore "external" visual artifacts like box-shadow and outline rules that might still cause an element to paint without width/height...or is that incorrect?
  • What's the timing relationship between Resize Observer delivery and Intersection Observer records? I presume the latter happens (sometimes much) later?
  • Is there more example code? ISTM that the explainer should include much more example code to outline the various scenarios where RO will be used.
  • How do RO's work with proxies like the Offscreen Canvas proposal?
  • Were design alternatives considered? If so, why were they discarded?

Regarding Explainer style, we've put together a draft outline here that might assist you.

Thanks again for filing for TAG review and for providing an Explainer!

@atotic
Copy link
Author

atotic commented Aug 18, 2017

Abbreviation: RO - ResizeObserver

Answers:

Why aren't the new & old sizes provided by the API directly?

The need to report the old size never came up. We tried to keep the API simple.
If the old size is needed, we'd have to define what is meant by "old size". The simplest answer would be "last reported size". If that is the answer, it can be trivially implemented on top of the existing API. If so, there is no need to add it to existing API unless widely used.

Is it always going to be cheap to call window.getComputedStyle() within the callback to get the new sizes?

I read this question as "Is getComputedStyle() guaranteed not to trigger reflow".
Not in general case. If there are multiple ROs, and first observer's callback invalidates layout, calls to getComputedStyle() in second observer will trigger reflow.

What's the timing relationship between Resize Observer delivery and Intersection Observer records? I presume the latter happens (sometimes much) later?

ResizeObserver notifications are delivered after Intersection Observer: https://wicg.github.io/ResizeObserver/#html-event-loop

When RO spec was designed, Intersection Observer used to deliver records with delay, without specific delivery timing guarantees. Therefore, the ordering of notification delivery between Intersection Observer and Resize Observer was not an issue.

Looking at the existing Intersection Observer spec, I do not see anything about delayed delivery. If Intersection Observer has started providing stronger delivery guarantees, I should open an issue to discuss RO/IO relationship.

What is the current status of Intersection Observer delivery guarantees?

Is there more example code? ISTM that the explainer should include much more example code to outline the various scenarios where RO will be used.

I'll update the explainer shortly. So far, there are only 2 usage scenarios we've come upon, if more are discovered, I can add them to the explainer.

How do RO's work with proxies like the Offscreen Canvas proposal?

ResizeObserver only works with visible DOM elements. How is Offscreen Canvas related?

Were design alternatives considered? If so, why were they discarded?

Design alternatives for what? Different parts of RO API? We've had several design discussions:

  • What happens when you start observing an element? RO API here is unexpected: you get a notification immediately with the existing size. This is done to avoid forcing reflow when starting an observation. This has caused problems for one developer.
  • Observer vs event API. Observer API won because because of efficiency, consistency with existing APIs.
  • What to observe? Detailed discussion are issue 4 and issue 6. clientWidth/Height, borderBoxSize, contentSize were considered.
    • clientWidth cons: rounded, element move could trigger size change. Rejected.
    • borderBoxSize, cons: no notifications when border or padding change. Rejected.
    • contentSize: cons: size could change simultaneously with border/padding keeping content size same, resulting in no notifications.. Accepted.
  • How to prevent infinite loops. We considered timeout, hardcoded iteration limit, and depth limit. Decided upon depth limit. Detailed discussion at issue 7

@atotic
Copy link
Author

atotic commented Aug 18, 2017

1 more interesting usage scenario: iframe sizing, will provide example.

@atotic
Copy link
Author

atotic commented Sep 14, 2017

Explainer has been updated with 2 new examples: chat and iframe resizing.

All examples also have fully functional demos when you start Chrome with --enable-blink-features=ResizeObserver

Is there anything else I can help with. I'd love to ship this by M63, whose deadline is end of September.

@atotic
Copy link
Author

atotic commented Sep 26, 2017

In my last big reply, I've left one question open: interaction between ResizeObserver and IntersectionObserver. I've talked to IO authors, and now understand the relationship between the two:

  • IntersectionObserver runs observation loop after ResizeObserver. This is because IO cares about what is displayed on screen, and RO can change layout.
  • IntersectionObserver notifications are delivered on general task queue, with general task queue delivery guarantees.

@torgo torgo added the Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review label Sep 27, 2017
@torgo torgo modified the milestones: tag-telcon-2017-08-22, tag-telcon-2017-10-10 Sep 27, 2017
@slightlyoff
Copy link
Member

Hey @atotic; we had a chance to discuss at today's F2F in Nice. Overall:

  • We're generally happy with this. Thanks for the detailed feedback on the questions.
  • Thanks for updating the explainer! Sample code truly is the lifeblood of good API design.
  • Is this going to be launched as an Origin Trial? If so, this seems to be in good shape.
  • Great to see Web Platform Tests!
  • The question about "previous value" keeps coming up in our discussion. (Mutation Observer, e.g., enables developers to request this data for some value types](https://dom.spec.whatwg.org/#dictdef-mutationobserverinit). Given that it's cheap to track this in the engine on an opt-in basis, it might be a helpful addition.
  • Regarding which value to deliver, what's the argument against reporting any/all of border/client/content sizes?
  • thanks for the update on IO vs. RO timings. They make sense.

@atotic
Copy link
Author

atotic commented Sep 27, 2017

Is this going to be launched as an Origin Trial? If so, this seems to be in good shape.

I was not thinking Origin Trial, but after talking it over, seems like a good idea. This is the current plan.

The question about "previous value" keeps coming up in our discussion

I am unsure about the usefullness of "previous value". None of the current examples use it. My guiding principle was not to include anything that is not obviously useful. It is trivial to implement it on top of RO. Did you have any particular examples in mind?

Regarding which value to deliver, what's the argument against reporting any/all of border/client/content sizes?

This is an interesting question. In theory, RO handlers should not depend on other sizes. If it does, and other size changes, RO will not be notified (unless other size change also triggers content size change), and RO element will be out of sync. We cannot enforce not using other sizes, but not having other sizes reported discourages their use.

I've also noticed that TAG was wondering about 2nd implementation. In summer of 2016, an intern at FF started a ResizeObserver implementation. It never landed, but we did have spec clarification discussions, they did use my tests,

@plinss plinss added extra time and removed Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review labels Feb 1, 2018
@plinss
Copy link
Member

plinss commented Feb 2, 2018

There's a fundamental problem here that ROs are presuming a single rect per element. Elements can fragment and have multiple boxes that can span columns, pages, and (eventually) regions. Each of those boxes can resize independently.

In general I'm not happy with new APIs that continue to bake in conflation of DOM and layout. Ideally this would be based on a box tree API rather than the DOM.

I also wonder if many of the prospective use cases for this wouldn't be better handled with custom layout worklets.

@plinss plinss added Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review and removed extra time labels Feb 2, 2018
@atotic
Copy link
Author

atotic commented Feb 28, 2018

There's a fundamental problem here that ROs are presuming a single rect per element.

Yes, RO does not observe fragments. The reason is implementation constraints:

  • currently only Edge layout engine uses fragments internally. Without engine support, observing fragments cannot be done efficiently.
  • most of the web content does not involve fragments, and observing traditional css rect works well enough for this case.

I regret that we were also unable to observe other CSS rects (border box, padding box) for performance reasons. I can imagine v2 of the API being extended to support these and maybe fragments.

I also wonder if many of the prospective use cases for this wouldn't be better handled with custom layout worklets.

Layout worklets can solve the same problem, but would be more complex.

@plinss
Copy link
Member

plinss commented Mar 1, 2018

currently only Edge layout engine uses fragments internally. Without engine support, observing fragments cannot be done efficiently.

Gecko also has a notion of "prev/next in flow" which allows traversal of all fragmented boxes very efficiently.

most of the web content does not involve fragments, and observing traditional css rect works well enough for this case.

This is changing, given multicol layout, and will continue to change as paged overflow and regions get implemented. Expect custom layout to get fragmentation support as well (if not in V1) leading to all sorts of new ways to fragment content.

I'm very reluctant to have new APIs continue the incorrect assumption made in the older DOM APIs of presuming a single box for each node. This will be a disservice in the long run and will work to prevent adoption (and implementation) of newer page layout techniques. If nothing else, craft the API to handle multiple boxes per node and allow current implementations to only return the first one, or the bounding rect (but then be clear it's a bounding rect), so that multiple boxes can be seamlessly handled in the future.

@eeeps
Copy link

eeeps commented Apr 3, 2018

I also wonder if many of the prospective use cases for this wouldn't be better handled with custom layout worklets.

Layout worklets can solve the same problem, but would be more complex.

@atotic @plinss I was talking about this with Martin Auswöger this morning and he pointed out that while ResizeObserver callbacks have access to the DOM, layout worklets do not. Many container-query-type use cases for ResizeObserver modify properties of observed elements’ decendants which are not related to layout (e.g., font sizes and colors).

@cynthia cynthia mentioned this issue Apr 5, 2018
5 tasks
@cynthia
Copy link
Member

cynthia commented Oct 30, 2018

Please ignore the reference above, incorrectly linked from another issue.

@travisleithead
Copy link
Contributor

Thanks so much for the feedback! At our TAG f2f in Paris today we considered what the next steps should be, and believe that our initial review is complete. It does seem like there will be some new additions coming down the road, and we are excited to see what comes. For now, we'll close this issue, but hope that you consider filing a new issue or reactivating this one when you'd like to get some additional feedback from us! As always, thank you for the opportunity to provide feedback and thanks for flying TAG!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review
Projects
None yet
Development

No branches or pull requests

8 participants