w3c / csswg-drafts

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

[css-contain] content-visibility: auto visibility check timing needs details #8542

Closed emilio closed 1 year ago

emilio commented 1 year ago

https://drafts.csswg.org/css-contain/#cv-notes has:

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.

When and how, exactly? This is not really implementable without more details. Since the visibility check is usually done with an intersection observer, but intersection observer callbacks happen async per spec (https://w3c.github.io/IntersectionObserver/#queue-intersection-observer-entry-algo), how is this supposed to happen in the same frame?

Does that mean that computing the intersections of the frame can change layout, and thus can change whether other stuff intersects? I hope not...

cc @chrishtr @asurkov @mrobinson @Loirooriol

Loirooriol commented 1 year ago

@vmpstr How does Blink do that?

vmpstr commented 1 year ago

It is only the initial observation that has to be synchronous. This is to avoid a flash of "skipped" content for onscreen elements with c-v auto. This does indeed mean that sizes can change and other elements may become intersecting... I'm unsure why this is bad other than the inefficiency it can introduce on some specifically crafted pages. The common case here is one repetition of style/layout.

In Blink, we use IntersectionObserver (internal class) which implements all the same behaviours as the spec for IntersectionObserver with two exceptions: the notification delivery happens synchronously, and the time at which we determine intersections is within the lifecycle (right before ResizeObserver steps). When we get the notification callback, we check if the context it affects has been notified before. (context is a class that implements content visibility logic). If it has been notified, we schedule a new animation frame at which to deliver this notification. If it has not been notified, then we determine the c-v state synchronously and, if the skipped state changed, repeat the lifecycle phases

vmpstr commented 1 year ago

I was thinking more about this: I think the language in the spec should change to say that it is in fact the first visibility computation that needs to be synchronous -- if it results in changes to the skipped state, those changes need to be incorporated into the presented frame by re-computing rendering synchronously. This is different from what the spec says, since it's fine to let the second computation to be asynchronous even if that second computation happens as a result of re-computing rendering due to the first computation.

I'm guessing that's the worry here: that we may construct a page where the state is unstable (e.g. #8407), so we still want to make progress. The way the text is phrased right now implies that the stable state needs to be found in the same frame as c-v was first discovered.

emilio commented 1 year ago

I'm unsure why this is bad other than the inefficiency it can introduce on some specifically crafted pages. The common case here is one repetition of style/layout.

Well, depending on when this is defined to happen (which right now is "somewhere, somehow"), it can cause bogus ResizeObserver or IntersectionObserver notifications, for example (if we define it to happen at the regular "intersection observer" step).

It seems at the time blink does it prevents that, but it also causes other issues like scroll events not being able to see the unskipped content etc. Which is probably fine, if it is well defined.

There's also the fact that if you have two nodes, unskipping the first one might cause the second one to be unskipped as well (or skipped, but because of the remembered size shenanigans that probably doesn't matter). Seems that would still be able to cause a flash? Or various sync layout flushes, which seems rather unfortunate.

notification delivery happens synchronously, and the time at which we determine intersections is within the lifecycle (right before ResizeObserver steps).

Those are rather significant behavior differences from a regular IntersectionObserver, if you ask me :)

vmpstr commented 1 year ago

Well, depending on when this is defined to happen (which right now is "somewhere, somehow"), it can cause bogus ResizeObserver or IntersectionObserver notifications, for example (if we define it to happen at the regular "intersection observer" step).

It seems at the time blink does it prevents that, but it also causes other issues like scroll events not being able to see the unskipped content etc. Which is probably fine, if it is well defined.

I agree, we should elaborate on the timing in the spec

There's also the fact that if you have two nodes, unskipping the first one might cause the second one to be unskipped as well (or skipped, but because of the remembered size shenanigans that probably doesn't matter). Seems that would still be able to cause a flash? Or various sync layout flushes, which seems rather unfortunate.

Yeah, the flashes are certainly still possible. This particular point was meant to help with the common case of c-v auto element in the viewport on load, but if those changes cause other elements to become visible, then the flash would still happen. This would be on developers to fix (better sizing etc), whereas the initial flash that we're avoiding is really hard for developers to fix.

Those are rather significant behavior differences from a regular IntersectionObserver, if you ask me :)

This is absolutely fair. In Blink, we're using the one and the same implementation except skipping a post task which causes asynchrony. The spec also didn't mean to imply that you can or should use the javascript intersection observer for these calculations, rather that the work necessary to determine this intersection can be done by a version of an intersection observer

emilio commented 1 year ago

Adding to the agenda as a call for feedback... I'm not a particular fan of the magic sync intersection observer, I wonder if other folks from WebKit have other ideas, cc @smfr

vmpstr commented 1 year ago

What do you think of the following proposal for a spec edit?

Spec edits for step 3 and 4 of the clarifications:

During step 14.1 (“recalculate styles and update layout”) of Update the Rendering, if an element has an auto used value of the content-visibility property and is not skipped, it computes its on-screen status (i.e. on-screen for relevant to the user) at that time. If this has never been done for an element, it has an indeterminate on-screen status. Elements with indeterminate on-screen status are considered not on-screen for the purpose of determining whether they are relevant to the user.

Note: on-screen status belongs to an element, not a box in the render tree, and becomes determined only once in its lifetime.

If any elements with indeterminate on-screen status change their on-screen status from indeterminate to on-screen during step 14.1, then style and layout must be subsequently updated synchronously before step 14.2. User Agents may limit the number of repeated style and layout updates during this step.

Note: this may cause style and layout to update as many times as the depth of content-visibility:auto nesting in the document. (However, content-visibility:auto elements not nested within others that started offscreen do not cause a subsequent style and layout update.) This recursion is similar to resize observer, Step 14.4.2 of Update the Rendering, which also has a way for User Agents to limit work.

Implementation notes:

In Blink, this is implemented as a separate delivery behavior for IntersectionObserver.

As an aside, IntersectionObserver in Blink is used both as an internal class and a script exposed API. Delegates registered there can specify their own delivery behavior. The intersection observer delegate that we create for content-visibility specifies this different delivery time.

This delivery always happens during the lifecycle, before the resize observer steps.

To implement the effect behavior in steps 3 and 4, and determine whether it’s synchronous or asynchronous, we query the display lock context state (this is an optional per-element state) and either update it or queue an internal task to update it at the next lifecycle. This is, of course, only one possible implementation. The synchronous on-screen status determination doesn’t have to use an IntersectionObserver, and can be done in other ways. We just found it convenient to do that in Blink, because IntersectionObserver has all of the relevant geometry code.

css-meeting-bot commented 1 year ago

The CSS Working Group just discussed [css-contain] content-visibility: auto visibility check timing needs details.

The full IRC log of that discussion <TabAtkins> vmpstr: emilio raised that the language in the current Contain spec is hard to implement
<TabAtkins> vmpstr: specifically, the behavior of content-visiblity:auto elements when determining visibility
<TabAtkins> vmpstr: intended behavior is that the very first time we determine vis, that determination happens sync, and if it changes the state of the visibility, the style/layout pass happen sync with it so the first frame is consistent and correct
<TabAtkins> vmpstr: Adding content-vis:auto on an element alreayd in the viewport would otherwise cause a flash of blank, because the typical (non-first) determination of visibility is deferred to next frame
<TabAtkins> vmpstr: In my last comment in issue, I tried to propose spec changes to capture this rigorously
<TabAtkins> vmpstr: Proposal is to adopt that text
<TabAtkins> emilio: I agenda+'d befor eyou added that comment, thanks for writing it
<TabAtkins> emilio: Specifics of timing are observable in different ways
<TabAtkins> emilio: iiuc, the way chrome does it fires right before ResizeObserver
<TabAtkins> emilio: scroll events, anchoring, etc all see this
<TabAtkins> emilio: I'd love if someone from WK could go over this and confirm it's fine
<TabAtkins> emilio: I'm not opposed to doing this just before RO, just want to make sure it's interop
<flackr> https://github.com/w3c/csswg-drafts/issues/8694 is also related
<flackr> q+
<TabAtkins> emilio: I find it a bit unfortunate we can't just use an IO to explain how this behavior, but I understand the use-case of appending
<astearns> ack flackr
<TabAtkins> flackr: I linked a related issue for scroll anims, the determination of frames can depend on alyout and we don't want the flash either
<TabAtkins> flackr: We should think thru before/after RO for that other use-case too. I specifically put it *after* because the user-defined sizing might affect things we want to respond to
<TabAtkins> flackr: But for c-v i'm concerned it might go both ways. An RO might make something visible, but also people might want RO positioning to rely on correct c-v.
<TabAtkins> emilio: Right, and in Chrome right now if you append a c-v:auto element within an RO, it just won't work, right?
<TabAtkins> vmpstr: If you make a new element in an RO callback it works in the same way, it hits the RO for that call in the same loop
<TabAtkins> vmpstr: The RO itself causes a flash of rendering, so if it changes things we do sync run style/layout
<TabAtkins> emilio: Oh so this also happens if you gBCR() on the element, you get new size?
<TabAtkins> vmpstr: I don't.... believe so? At least not per spec
<TabAtkins> vmpstr: To address why we wanted it before, we just don't really want to add a lot of script determining visibility/rendering. But if you put RO before, you get script running that can observe it.
<TabAtkins> emilio: It feels like this needs a bit more care. Maybe done along the RO loop somehow?
<TabAtkins> emilio: From what you wrote, I understood - in partiuclar the second-to-lst sentence you wrote "this always happens in lifecycle before RO steps", it sounded like a new lifecycle step, but it seem syou're not doing that if appending an el in an RO causes this to run somehow
<TabAtkins> astearns: Take back to the issue, then?
<TabAtkins> emilio: think that's fine, and again if someone from WK could look it would be great
<TabAtkins> smfr: I saw it. Agree that the interactions are hard to understand, maybe just have to implement to see what's happening.
<TabAtkins> smaug: I agree that using IO in a way that differs from the web version is weird - makes me question why authors can't get a non-flashing IO too?
<TabAtkins> emilio: that's fair
<TabAtkins> astearns: Let's close discussion and take it back to the issue, please add comments on what you'd like to see changed
vmpstr commented 1 year ago

Hi all,

Thank you for providing feedback at the working group meeting, it was valuable! i indeed missed the interaction between determining content-visibility visibility state and resize observer potentially creating new elements. I’ve also learned (from @flackr) that this type of change to the Update the Rendering loop was also used by the animation timelines.

We have elements with some CSS properties (like scroll timelines and content-visibility) that may need a second style+layout update on first load to avoid a flash of uninitialized content. Also, ResizeObserver callbacks can mutate the DOM and bring more such elements into existence. That means we need a loop that considers both. In essence, these scroll timeline and content-visibility aspects can be viewed as a special kind of internal lifecycle observer that are usually async, but are sync on first load.

In light of these things, I think we should update the wording for Update the Rendering “resize observer loop”, i.e step 14 of Update the Rendering, to admit the possibility of steps unrelated to resize observer into the loop that updates style and layout.

Specifically, we could write something like the following. Note that I’ve only pulled some wording from the animations spec here, I’m not an expert in those steps. IOW, the exact wording is something we can debate, but I wanted to propose this as a general idea for updating the loop language:

14. For each fully active Document doc in docs:
    1. Initialize state tracking variables:
        1. Let `resize observer depth` be 0.
        2. Let `stale animation timelines updated` be a boolean with value false
    2. Recalculate styles and update layout for doc.
    3. If there are stale animation timelines created during step 14.2 (https://drafts.csswg.org/scroll-animations-1/#html-processing-model-event-loop; related: https://github.com/w3c/csswg-drafts/issues/8694 for possibly changing this) and `stale animation timelines updated` is false:
        1. Update the stale animation timelines
        2. Set `stale animation timelines updated` to true
        3. Go back and resume the algorithm from step 14.2
    4. If there are content-visibility elements with undetermined visibility:
        1. Synchronously determine those element’s visibility
        2. If the visibility update is from undetermined to visible, then go back and resume the algorithm from step 14.2
    5. Gather active resize observations at depth `resize observer depth` for doc. If doc has active resize observations:
        1. Set `resize observer depth` to the result of broadcasting active resize observations given doc.
        2. go back and resume the algorithm from step 14.2
    6. If doc has skipped resize observations, then deliver resize loop error given doc.

(sorry about formatting, the numbered lists are hard with indents)

Another item that may belong here is the contain-intrinsic-size last remembered size changes that went through the group, since a lot of the last remembered size changes, like “forgetting the size” are specced to be “at resize observer timing”. However, this one wouldn’t require a repeat of the loop.

foolip commented 1 year ago

cc HTML editors @annevk @domenic @zcorpan FYI that some HTML spec changes are being considered here.

flackr commented 1 year ago

@vmpstr the algorithm changes we were going for (see https://github.com/w3c/csswg-drafts/pull/8704) for these other extra passes were intended to only allow a single extra pass. I think rather than having each step recalculate styles and layout I'd would rather have a single additional pass if either of the things changed. This means language more like:

  1. If there are stale animation timelines or there are content-visibility elements with undetermined visibility:
    1. Recalculate styles and update layout for doc.

I also think for scroll driven animations (and possibly this case as well), that it may make more sense to run after ResizeObserver as that is the time developers typically size / position sub-components based on ancestor components so it is likely to introduce visible content.

vmpstr commented 1 year ago

I think rather than having each step recalculate styles and layout I'd would rather have a single additional pass if either of the things changed.

For content-visibility, we repeat the loop because of possible nesting so we can run more than once here. Of course, we can change that, but this is currently intentional.

I also think for scroll driven animations (and possibly this case as well), that it may make more sense to run after ResizeObserver as that is the time developers typically size / position sub-components based on ancestor components so it is likely to introduce visible content.

Since content-visibility can (and frequently does) change the size of the element, for that case I prefer to have this happen before resize observer callbacks. It would allow resize observer to see the visible size right away. If this is run after the resize observer callbacks, then because of the visibility state change, the resize observer callbacks have to run again for the shallower depth which without any other changes causes a resize observer loop error. It feels like for c-v specifically, running the steps before resize observer is more correct

css-meeting-bot commented 1 year ago

The CSS Working Group just discussed [css-contain] content-visibility: auto visibility check timing needs details.

The full IRC log of that discussion <dael> astearns: Anyone on the call want to put something on the record to help with discussion next week?
<dael> vmpstr: I'm happy to introduce the issue. Not asking for resolution because emilio isn't here
<dael> astearns: At the beginning of call chrishtr asked for people to take a look and speak up now or add comments
<dael> astearns: We'll come back next week
vmpstr commented 1 year ago

Based on the resolution in #8694 , the algorithm in https://github.com/w3c/csswg-drafts/issues/8542#issuecomment-1515216159 would omit the scroll timeline updates, and just have content-visibility and resize observer steps.

The distinction between this and the timelines is that whereas timelines (afaiu) can change the size of elements but are discouraged from doing that, content-visibility is very likely to change the size because of the size-containment change and this is not discouraged. Thus, it's ok to defer the resize observation for any scroll timeline updates side-effects, but I would still prefer content-visibility output to feed into resize observer input for the same frame.

css-meeting-bot commented 1 year ago

The CSS Working Group just discussed [css-contain] content-visibility: auto visibility check timing needs details, and agreed to the following:

The full IRC log of that discussion <bramus> rossen: who can pick up?
<emilio> Trying to unmute webex on my phone sigh
<bramus> emilio: general proposed idea of merging with resize observer makes sense
<bramus> emilio: sounds like a good plan
<bramus> emilio: needs to be done in the HTML spec instead of CSS I think
<bramus> rossen: ok
<bramus> rossen: not hearing any objections from google side
<bramus> rossen: proposed resolution to proceed with ResizeObserver solution in HTML spec
<bramus> emilio: HTML spec would have to reference contain spec but dont think extra work in css specs is needed
<bramus> rossen: objections?
<bramus> RESOLVED: Proceed with ResizeObserver solution in HTML spec
CyberAP commented 1 year ago

I wonder if this is the reason why Blink's (Chromium) implementation causes scroll stutter while Firefox' one does not.

rwlbuis commented 1 year ago

It would be good to get this resolved, the WebKit implementation has not implemented this so far, but some content-visibility WPT tests assume this in place and so right now those fail in the WebKit implementation.