Update RO loop to include content-visibility #9312

vmpstr · 2023-05-19T16:56:53Z

This PR updates the resize observer loop to include content-visibility steps. Fixes #9300

At least two implementers are interested (and none opposed):
- Blink
- Gecko (@emilio to confirm)
Tests are written and can be reviewed and commented upon at:
- https://wpt.fyi/results/css/css-contain/content-visibility/content-visibility-auto-first-observation-immediate.html
Implementation bugs are filed:
- Chromium: N/A
- Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1807253
- WebKit: N/A

/infrastructure.html ( diff )
/webappapis.html ( diff )

vmpstr · 2023-05-19T16:57:41Z

Hi, I'd appreciate wordsmithing help on how to reference content-visibility properly

annevk

It's hard to review the substance due to the editorial issues.

Might also benefit from someone that knows the rendering loop well, but I'm not sure who that is these days. @dbaron? @emilio? @dholbert?

source

annevk · 2023-05-22T14:27:03Z

source

+         <li><p>If there are elements with <code data-x="">content-visibility</code>
+         used value of <code data-x="">auto</code> whose viewport proximity has not been
+         previously determined for the purposes of being
+         <a href="https://drafts.csswg.org/css-contain-2/#relevant-to-the-user">relevant to the user</a>.:


This is very COMEFROM.

Also, I'm pretty sure we use single quotes to delimit CSS properties and values.

Done on the single quotes.

What is COMEFROM?

COMEFROM is the opposite of GOTO... and even more confusing. In other words, it's an algorithm saying that you should enter it from some other algorithm, without anything in that other algorithm saying so (which makes it less discoverable than GOTO, and equally spaghetti-like).

(I'm not sure whether that helps understand @annevk's comment, though.)

Thanks, that makes sense!

FWIW, there is a line in css-contain-2 that says what relevant to the user means, and one of the criteria is viewport proximity. Also in css-contain-2, there is a line that says that the first determination of viewport proximity must happen in the same visual frame that determined the existence of that element. This (somewhat confusing) phrasing is what caused us to debate of how to properly specify the behavior we're after. We settled on changing the event loop (this PR) as possibly the best choice.

I do think that if we make this change, we should also adjust css-contain-2 to at least do the GOTO part of this spaghetti and reference the event loop as a clarification of how to determine the initial viewport proximity. I'm unsure of how else to go about specifying this without cross-referencing between the two specs

Having dug into this a little more than when I made my previous comment, I agree this is (in the state currently in this PR) somewhat confusing, at least assuming I'm not missing a relevant piece of spec. In particular, it seems like it would help if there were a definition somewhere for determining viewport proximity that clearly described the states of "proximate", "not proximate", or "not yet determined" (or something like that) and something that clearly stated when transitions between those states occur.

@vmpstr can you say something about how this is implemented in Chromium? Is there a bit of code in the "update the rendering" loop that does the "determine viewport proximity" part, or does it actually happen elsewhere?

A lot of the time matching the structure of the implementation (but discarding optimizations) makes things fall into place, and makes COMEFROM impossible, since implementations can't do that.

I agree with @dbaron that we lack some definitions that I'm using liberally here. And I agree that there are three states: it's proximate, not proximate, or not yet determined.

The question I have is should these be defined in the css spec or in html? If their only use is in this algorithm, then it seems like we should define it here. However, https://www.w3.org/TR/css-contain-2/#relevant-to-the-user references "on-screen" which should be the definition of "proximate", so maybe it should be in css instead.

4.4.4 (https://www.w3.org/TR/css-contain-2/#cv-notes) should also reference that if the proximity state is not yet determined, then it should be determined synchronously at resize observer timing, and GOTO this algorithm?

Any thoughts?

@vmpstr it sounds like you need an algorithm that will be called from two places, HTML and https://www.w3.org/TR/css-contain-2/. How about starting to write down the algorithm/definition and see if it's mostly defined in terms of other HTML concepts or other CSS concepts, and then deciding where to put it?

The algorithm that is called from both spots can be summarized as "determine proximity to the viewport". The method used for this determination is specifically left to the user-agent. In Chromium, that's intersection observer with some modifications for making it asynchronous, but that is not the only (or the best?) method.

In a later comment you asked for now Chromium does this, and for the most part it follows the algorithm outlined here. I really do think that the only terms missing here is proximity-to-viewport related things. css-contain-2 is written declaratively, so it sounds like it would be the best spot to specify and reference those in places where we talk about "on-screen"-ness

source

annevk · 2023-05-22T14:28:34Z

source

-         <li><p>Recalculate styles and update layout for <var>doc</var>.</p></li>
+          <li><p>If any such determination resulted in an element being relevant to the user, continue.</p></li>
+         </ol>
+         </li>


These cannot have the same indentation.

What would be the correct way to indent this? I'm following an example from above, where closing tags of ol and li have the same indentation: https://github.com/whatwg/html/pull/9312/files#diff-41cf6794ba4200b839c53531555f0f3998df4cbb01a4d5cb0b94e3ca5e23947dL101809-R101810

That looks like a bug. Typically each nested element that needs to be on its own line is one space more indented. And then as you close them they are one space less indented.

That makes sense. I've updated the formatting. Let me know if it looks better

dbaron

A few comments here, and also a somewhat more general one: it's not clear to me whether the interaction with the resize observer depth is the most desirable one. There was some discussion in w3c/csswg-drafts#8542 (comment) of a relationship to depth. However, it seems like what's being specified here differs from that, since I think the current spec text doesn't imply any redo as a result of nesting of content-visibility: auto, but only redo as a result of changes made in resize observations. But maybe it does, if something not being relevant to the user means that its descendants remain in the "undetermined proximity" state -- which gets back to the comment about more clearly defining that state.

source

dbaron · 2023-05-30T18:21:25Z

source

+         <li><p>If there are elements with <code data-x="">content-visibility</code>
+         used value of <code data-x="">auto</code> whose viewport proximity has not been
+         previously determined for the purposes of being
+         <a href="https://drafts.csswg.org/css-contain-2/#relevant-to-the-user">relevant to the user</a>.:


Having dug into this a little more than when I made my previous comment, I agree this is (in the state currently in this PR) somewhat confusing, at least assuming I'm not missing a relevant piece of spec. In particular, it seems like it would help if there were a definition somewhere for determining viewport proximity that clearly described the states of "proximate", "not proximate", or "not yet determined" (or something like that) and something that clearly stated when transitions between those states occur.

source

foolip · 2023-05-31T13:27:46Z

source

+         <li><p>If there are elements with <code data-x="">content-visibility</code>
+         used value of <code data-x="">auto</code> whose viewport proximity has not been
+         previously determined for the purposes of being
+         <a href="https://drafts.csswg.org/css-contain-2/#relevant-to-the-user">relevant to the user</a>.:


@vmpstr can you say something about how this is implemented in Chromium? Is there a bit of code in the "update the rendering" loop that does the "determine viewport proximity" part, or does it actually happen elsewhere?

A lot of the time matching the structure of the implementation (but discarding optimizations) makes things fall into place, and makes COMEFROM impossible, since implementations can't do that.

vmpstr · 2023-07-14T17:46:00Z

I've reworked the wording a bit with more references to CSSCONTAIN now that it defines viewport proximity. I would appreciate another round of reviews.

dbaron

This looks good to me, though I have a few comments below on things that could make it a little clearer.

dbaron · 2023-07-14T21:44:20Z

source

+          <ol>
+           <li><p>Determine <span>proximity to the viewport</span> for the element.</p></li>
+
+           <li><p>If prior to this determination, the element's <span>proximity


I needed to read this sentence about five times to understand what it meant. Perhaps it would be clearer if the "prior to this determination" part is refactored into a separate item in the list (adding a new first item before the current two items) that sets a variable that is then used in this item?

I changed this by breaking it up over a couple of lines with ULs instead of OLs. I'm not sure if that's idiomatic. It looks and reads better to me, but I have author bias. Let me know how this looks.

I think it's maybe a little bit clearer, but what I was suggesting was to turn the 2-item list into:

Let checkForInitialDetermination be true if the element's proximity to the viewport is not determined and it is not relevant to the user. Otherwise, let checkForInitialDetermination be false.

Determine proximity to the viewport for the element.

If checkForInitialDetermination is true and the element is now relevant to the user, then set hadInitialVisibleContentVisibilityDetermination to true.

Ah, yeah that makes it clearer. I was avoiding an extra var, but that's silly.

dbaron · 2023-07-14T21:51:25Z

source

+          </ol>
+         </li>
+
+         <li><p>If <var>hadInitialVisibleContentVisibilityDetermination</var> is true, <span>Continue</span>.


I'm not sure if it's clear that this "Continue" refers to the inner-most loop. (It's not clear to me from reading what "Continue" links to.) I'm not sure what the convention is for this, though. Hopefully @annevk can help.

It does seem clear in the compiled output, since there's less text and higher indentation, so I would say it's ok, but happy to change it in any way

I don't think it's a question about formatting, it's a question about what "Continue" means according to the infra spec. (That is, it is (correctly) continuing the inner "While true" loop or is it (incorrectly) continuing the outer "For each fully active Document doc in docs" loop?)

I don't know the answer, I'm just pointing out that I'd like the HTML editor who reviews this to double-check this since I'm pretty sure all the HTML editors will know whether this is correct.

I filed whatwg/infra#601 this, so unless the response there disagrees with my suggestion, I think this is ok.

Gotcha, I agree with you that since most programming language's continue and break affect the immediate scope, I would be very surprised if that isn't the case in the html spec, but it'd be good to resolve this.

dbaron · 2023-07-21T18:10:32Z

source

+          </ol>
+         </li>
+
+         <li><p>If <var>hadInitialVisibleContentVisibilityDetermination</var> is true, <span>Continue</span>.


I don't think it's a question about formatting, it's a question about what "Continue" means according to the infra spec. (That is, it is (correctly) continuing the inner "While true" loop or is it (incorrectly) continuing the outer "For each fully active Document doc in docs" loop?)

I don't know the answer, I'm just pointing out that I'd like the HTML editor who reviews this to double-check this since I'm pretty sure all the HTML editors will know whether this is correct.

dbaron · 2023-07-21T18:18:15Z

source

+          <ol>
+           <li><p>Determine <span>proximity to the viewport</span> for the element.</p></li>
+
+           <li><p>If prior to this determination, the element's <span>proximity


I think it's maybe a little bit clearer, but what I was suggesting was to turn the 2-item list into:

Let checkForInitialDetermination be true if the element's proximity to the viewport is not determined and it is not relevant to the user. Otherwise, let checkForInitialDetermination be false.

Determine proximity to the viewport for the element.

If checkForInitialDetermination is true and the element is now relevant to the user, then set hadInitialVisibleContentVisibilityDetermination to true.

dbaron · 2023-07-24T19:41:22Z

source

-            </ul>
-           </li>
+           <li><p>If <var>checkForInitialDetermination</var> is true and the element is now <span>relevant to the user</span>,
+           then set hadInitialVisibleContentVisibilityDetermination to true.</p></li>


You lost the <var>...</var> around hadInitialVisibleContentVisibilityDetermination here (possibly by copying my text!).

possibly? <_<

dbaron

Looks good to me, with one further comment.

domenic

Some editorial improvements possible

source

domenic

LGTM!

Can you restore the PR template and fill it out with appropriate tests, browser bugs, etc.?

This adds a test which directly tests the changes in whatwg/html#9312 [email protected] Change-Id: I2108e67d0444220cb75e43121dbe6076052a2b5b

This adds a test which directly tests the changes in whatwg/html#9312 [email protected] Change-Id: I2108e67d0444220cb75e43121dbe6076052a2b5b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4725605 Reviewed-by: Chris Harrelson <[email protected]> Commit-Queue: Vladimir Levin <[email protected]> Cr-Commit-Position: refs/heads/main@{#1176127}

This adds a test which directly tests the changes in whatwg/html#9312 [email protected] Change-Id: I2108e67d0444220cb75e43121dbe6076052a2b5b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4725605 Reviewed-by: Chris Harrelson <[email protected]> Commit-Queue: Vladimir Levin <[email protected]> Cr-Commit-Position: refs/heads/main@{#1176127} Co-authored-by: Vladimir Levin <[email protected]>

vmpstr · 2023-07-31T16:42:00Z

Can you restore the PR template and fill it out with appropriate tests, browser bugs, etc.?

I've done that. Let me know how it looks

…rmination, a=testonly Automatic update from web-platform-tests CV: Add a test for first visibility determination (#41205) This adds a test which directly tests the changes in whatwg/html#9312 [email protected] Change-Id: I2108e67d0444220cb75e43121dbe6076052a2b5b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4725605 Reviewed-by: Chris Harrelson <[email protected]> Commit-Queue: Vladimir Levin <[email protected]> Cr-Commit-Position: refs/heads/main@{#1176127} Co-authored-by: Vladimir Levin <[email protected]> -- wpt-commits: 45dca2414d3e73cc0329e1e6f1ae3e9abfb43021 wpt-pr: 41205

https://bugs.webkit.org/show_bug.cgi?id=259825 Reviewed by Tim Nguyen. Implement initial visibility determination as specified in [1]. This replaces the custom onscreen bookkeeping with the specified proximity to viewport concept. The updateIntersectionObservations logic is refactored a bit so it can be used for updating any given collection of intersection observers. This PR also changed behaviour to not dispatch ContentVisibilityAutoStateChange on disconnected elements, this problem is now exposed with the different timing of initial visibility determination. [1] whatwg/html#9312 * LayoutTests/imported/w3c/web-platform-tests/css/css-contain/content-visibility/content-visibility-auto-first-observation-immediate-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-contain/content-visibility/content-visibility-auto-first-observation-immediate.html: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-sizing/contain-intrinsic-size/auto-012-expected.txt: * Source/WebCore/dom/ContentVisibilityDocumentState.cpp: (WebCore::ContentVisibilityDocumentState::unobserve): (WebCore::ContentVisibilityDocumentState::checkRelevancyOfContentVisibilityElement const): extract per element content relevancy check logic (WebCore::ContentVisibilityDocumentState::updateRelevancyOfContentVisibilityElements const): bail out early if no update is specified (WebCore::ContentVisibilityDocumentState::determineInitialVisibleContentVisibility const): (WebCore::ContentVisibilityDocumentState::updateContentRelevancyStatusForScrollIfNeeded): (WebCore::ContentVisibilityDocumentState::updateViewportProximity): keep track of per element viewport proximity (WebCore::ContentVisibilityDocumentState::removeViewportProximity): (WebCore::ContentVisibilityDocumentState::updateRelevancyOfContentVisibilityElements): Deleted. (WebCore::ContentVisibilityDocumentState::updateOnScreenObservationTarget): Deleted. * Source/WebCore/dom/ContentVisibilityDocumentState.h: * Source/WebCore/dom/Document.cpp: (WebCore::Document::updateIntersectionObservations): allow specifying the intersection observers (WebCore::Document::updateResizeObservations): integrate according to [1] * Source/WebCore/dom/Document.h: * Source/WebCore/dom/Element.cpp: (WebCore::Element::contentVisibilityViewportChange): Deleted. * Source/WebCore/dom/Element.h: Canonical link: https://commits.webkit.org/267779@main

…#41205) This adds a test which directly tests the changes in whatwg/html#9312 [email protected] Change-Id: I2108e67d0444220cb75e43121dbe6076052a2b5b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4725605 Reviewed-by: Chris Harrelson <[email protected]> Commit-Queue: Vladimir Levin <[email protected]> Cr-Commit-Position: refs/heads/main@{#1176127} Co-authored-by: Vladimir Levin <[email protected]>

Update RO loop to include content-visibility

29a6644

annevk reviewed May 22, 2023

View reviewed changes

vmpstr added 2 commits May 23, 2023 12:39

review feedback

bd8fb54

update formatting

2f06bd1

dbaron reviewed May 30, 2023

View reviewed changes

foolip reviewed May 31, 2023

View reviewed changes

vmpstr mentioned this pull request Jun 1, 2023

[css-contain-2] Define proximity to the viewport w3c/csswg-drafts#8904

Merged

vmpstr added 2 commits June 28, 2023 13:18

update

7514d0d

update

443303b

vmpstr requested review from dbaron, foolip and annevk July 14, 2023 17:45

dbaron approved these changes Jul 14, 2023

View reviewed changes

update

4b544ff

dbaron approved these changes Jul 21, 2023

View reviewed changes

update

094c574

dbaron reviewed Jul 24, 2023

View reviewed changes

dbaron approved these changes Jul 24, 2023

View reviewed changes

var

fd6f27d

dbaron approved these changes Jul 25, 2023

View reviewed changes

domenic added topic: rendering integration Better coordination across standards needed labels Jul 26, 2023

domenic reviewed Jul 26, 2023

View reviewed changes

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

review

bc71e61

vmpstr requested a review from domenic July 26, 2023 16:02

domenic approved these changes Jul 27, 2023

View reviewed changes

chromium-wpt-export-bot mentioned this pull request Jul 27, 2023

CV: Add a test for first visibility determination web-platform-tests/wpt#41205

Merged

domenic merged commit ed99e00 into whatwg:main Aug 1, 2023

rwlbuis mentioned this pull request Aug 21, 2023

[content-visibility] Implement initial visibility determination WebKit/WebKit#16391

Merged

Update RO loop to include content-visibility #9312

Update RO loop to include content-visibility #9312

Conversation

vmpstr commented May 19, 2023 • edited by pr-preview bot Loading

vmpstr commented May 19, 2023

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmpstr commented Jul 14, 2023

dbaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbaron left a comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

vmpstr commented Jul 31, 2023

vmpstr commented May 19, 2023 •

edited by pr-preview bot

Loading