w3c / csswg-drafts

CSS Working Group Editor Drafts
https://drafts.csswg.org/
Other
4.47k stars 658 forks source link

[css-contain][css-sizing] `content-visibility: auto` kinda broken by not using last remembered size with `contain: size` #7807

Open Loirooriol opened 2 years ago

Loirooriol commented 2 years ago

5668 added contain-intrinsic-size: auto, whose purpose was addressing this problem:

One undesired effect of content-visibility: auto is that the scrollbar thumb tends to jump around when content enters and exits the visibility region. This is due to the fact that we keep adding and removing size containment in order for us to optimize rendering of skipped content. The contain-intrinsic-size property helps, but the size still keeps changing between the "real" intrinsic size and contain-intrinsic-size placeholder. The two sizes are frequently different.

contain-intrinsic-size: auto remembers the non-contained size, bringing stability.

Then #6308 restricted using the last remembered size to when the element skips its contents, so basic contain: size doesn't use the last remembered size. The reasoning was that the last remembered size can be out of date, so it can be bad to use it for elements which are visible in the screen.

So main cases have stability:

But imagine this corner case:

  1. An element has contain-intrinsic-size: auto 100px; content-visibility: auto
  2. It's visible in the screen, so it doesn't have containment, and its normal size is remembered, e.g. 15px.
  3. Then it gets contain: size. It's still not skipping its contents, so it's resized to 100px.
  4. The last remembered size is not updated, since it has size containment.
  5. The element moves off the screen, so it skips its contents, which changes the size into the remembered 15px.
  6. The element moves back into the screen, so it stops skipping its contents, and becomes 100px.

So the behavior is not stable at all! Here you have a testcase)%3B%0A%3C%2Fscript%3E), try scrolling from the top to the bottom and viceversa using the mouse wheel. The scrollbar thumb behaves strangely.

It's an edge case since mixing content-visibility: auto and contain: size is maybe a weird combination, and contain: size needs to be delayed in order to have a last remembered size. But should we try to improve it? Some ideas:

CC @vmpstr, @hamishwillee

vmpstr commented 2 years ago

I like either one of your proposals to have a better experience in this particular case. I'm slightly leaning towards the latter proposal to update the last remembered size when we're not skipping contents. That does tie the features even closer together, but since contain-intrinsic-size auto already only has an effect on content-visibility auto elements, I don't think that's a problem.

Loirooriol commented 2 years ago

A possible concern with the latter proposal is that at the time that the browser needs to decide whether to observe the element with a ResizeObserver, it may not be known yet whether the element skips its contents (in case of content-visibility: auto). I guess it can be observed anyways and then do the actual check in the callback, but this can imply some extra work.

vmpstr commented 2 years ago

Typically we deliver resize observations after we have decided whether the element skips its contents. Funny enough I have a Blink patch that I'm working on for an unrelated reason that ensures this https://chromium-review.googlesource.com/c/chromium/src/+/3926103.

I don't think the content-visibility spec says either way when we should decide that the element skips the contents, other than when it should synchronously do so and when it should be asynchronous

This could cause an element to be observed briefly but it shouldn't get to the point of actually dispatching the callback since it would be unregistered in the subsequent iteration when we determine the element is not locked.

Also you're right that even if we can't guarantee that, then we can always have a callback check this, although I'd prefer to avoid it. Did you have a case in mind where we wouldn't know at style time (or whenever we register for resize observation) whether or not the element skips its contents?

Loirooriol commented 2 years ago

Typically we deliver resize observations after we have decided whether the element skips its contents

https://github.com/whatwg/html/issues/8277 seems relevant.

Did you have a case in mind where we wouldn't know at style time (or whenever we register for resize observation) whether or not the element skips its contents?

content-visibility: auto only skips the contents when the element is off the screen by some UA-defined margin. So before determining the position and size of the element, we can't know whether it will be skipped or not, right?

In Gecko the code for observing or unobserving the size of contain-intrinsic-size:auto is called from nsIFrame::DidSetComputedStyle and nsIContent::SetPrimaryFrame. Possibly it could be called again when a content-visibility: auto element starts or stops skipping the contents, but not sure, I think this hasn't been implemented yet.

vmpstr commented 2 years ago

content-visibility: auto only skips the contents when the element is off the screen by some UA-defined margin. So before determining the position and size of the element, we can't know whether it will be skipped or not, right?

The way this should work is that when a content-visibility: auto lock is first created, we assume (in Blink) that it is skipping contents. It doesn't really matter what we assume at that time tbh, because the spec says

The initial determination of visibility for content-visibility: auto must happen in the same frame that determined an existence of a new content-visibility: auto element.

This means that whether or not we register for ResizeObservation at this point can only cause some churn, but no signals yet. Specifically in Blink, after the patch that I referenced above, we wait until style & layout is done, then determine the true state of the content-visibility. If our initial assumption was wrong, i.e. we should not be skipping contents, then we change the state and synchronously rerun style & layout. This would then unregister from resize observation. All this happens before we issue resize observer callbacks. I think this is what you mean by this can cause extra work, which I think is ok.

In the non-initial determination case, the spec says

If an element starts or stops skipping its contents, this change happens after the requestAnimationFrame callbacks of the frame that renders the effects of the change have run. Specifically, such changes will take effect between steps 13 and 14 of Update the Rendering step of the Processing Model (between “run the animation frame callbacks” and “run the update intersection observations steps”).

With a non-normative explanation:

Determining the viewport intersection of the element can be done with an internal version of an IntersectionObserver. However, since the observations from this are dispatched at step 14 of Update the Rendering, any changes to the skipped (and thus painted) state will not be visible to the user until the next frame’s processing. For this reason, updating the skipped state, including containment adjustments, is deferred to that frame as well. This ensures that script accessing, for example, the containment value of the element between these two events (internal intersection observation and skipped state update) will retrieve values consistent with current painted state and not cause any forced layouts.

This means that if we, for example, registered for the resize observation and later determine that the state should be skipping the contents, that state change doesn't take effect until the next frame so the resize observation should be correct, and we will unregister from the observation on the next frame.

So all in all, using "skips its content" as a condition to whether to do resize observations seems fine to me

Loirooriol commented 2 years ago

Yes, thinking better about it I guess it's fine, since with the current spec when an element stops skipping the contents then it can lose size containment so it may need to be observed to record its size anyways. So it shouldn't be adding complexity, it's just that I didn't implement this part since content-visibility: auto is not ready in Gecko.

css-meeting-bot commented 1 year ago

The CSS Working Group just discussed [css-contain][css-sizing] content-visibility: auto kinda broken by not using last remembered size with contain: size, and agreed to the following:

The full IRC log of that discussion <astearns> topic: [css-contain][css-sizing] content-visibility: auto kinda broken by not using last remembered size with contain: size
<astearns> github: https://github.com/w3c/csswg-drafts/issues/7807
<dandclark> oriol: content visibility: author, elements either enter screen or get out of the screren, can loose size containment, which chagnes size
<dandclark> oriol: Problem if you have scrollbar
<dandclark> oriol: Changing size of elements can change scrollbar thumb size. Added ???-auto. Stabilizeds content-visibility: auto
<dandclark> oriol: ...is triggering non-stable behaior. Have example in issue. <describes example in the issue>. Basically as elements gets in or out of the screen, is switching between 100-115 pixels. Getting instabilitty when shouold be stable.
<dandclark> oriol: So 2 possible solutions. 1st is changing requirement for using last remembered size. Now checking element is skipping contents. Instead check content visibility but still check size containment.
<dandclark> oriol: If content-visibility: auto ??? would still be able to use last remembered size. 2nd possibility is changing requirement for using last remembered size.
<dandclark> oriol: ???? So if the element is skipping contents, use last rememberd size. If not skipping contents, update last remembered size.
<dandclark> oriol: Vlad likes both but leans towards second.
<dandclark> oriol: I don't have strong opinion between the 2.
<dandclark> oriol: Anyone else have 3rd possibility to propose?
<dandclark> florian: I agree 2nd is the better one. But thinking longer may have different opinion.
<dandclark> astearns: proposed resolution: take second solution for this case. Update last remembered size if element doesn't skip its contents.
<dandclark> RESOLVED: Take second solution for this case. Update the last remembered size if element doesn't skip its contents.
Loirooriol commented 1 year ago

WPT: css/css-sizing/contain-intrinsic-size/auto-011.html and css/css-sizing/contain-intrinsic-size/auto-012.html