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.

Sign up for GitHub

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

Jump to bottom

Update RO loop to include content-visibility #9312

Merged
merged 9 commits into from
Aug 1, 2023
Merged

Update RO loop to include content-visibility #9312

Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
29a6644
Update RO loop to include content-visibility
vmpstr May 19, 2023
bd8fb54
review feedback
vmpstr May 23, 2023
2f06bd1
update formatting
vmpstr May 24, 2023
7514d0d
update
vmpstr Jun 28, 2023
443303b
update
vmpstr Jul 14, 2023
4b544ff
update
vmpstr Jul 19, 2023
094c574
update
vmpstr Jul 24, 2023
fd6f27d
var
vmpstr Jul 25, 2023
bc71e61
review
vmpstr Jul 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 28 additions & 12 deletions source
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101818,25 +101818,41 @@ import "https://example.com/foo/../module2.mjs";</code></pre>
<var>docs</var>:</p>

<ol>
<li><p>Recalculate styles and update layout for <var>doc</var>.</p></li>

<li><p>Let <var>depth</var> be 0.</p></li>

<li><p><span>Gather active resize observations at depth</span> <var>depth</var> for
<var>doc</var>.</p></li>
<li><p>Let <var>resizeObserverDepth</var> be 0.</p></li>
vmpstr marked this conversation as resolved.
Show resolved Hide resolved

<li>
vmpstr marked this conversation as resolved.
Show resolved Hide resolved
<p>While <var>doc</var> <span>has active resize observations</span>:</p>
<p>While true:</p>
vmpstr marked this conversation as resolved.
Show resolved Hide resolved

<ol>
<li><p>Set <var>depth</var> to the result of
<span data-x="broadcast active resize observations">broadcasting active resize
observations</span> given <var>doc</var>.</p></li>

<li><p>Recalculate styles and update layout for <var>doc</var>.</p></li>

<li><p><span>Gather active resize observations at depth</span> <var>depth</var> for
<li><p>If there are elements with 'content-visibility' used value of
vmpstr marked this conversation as resolved.
Show resolved Hide resolved
'auto' whose viewport proximity has not been previously determined for
the purposes of being
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very COMEFROM.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done on the single quotes.

What is COMEFROM?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<a href="https://drafts.csswg.org/css-contain-2/#relevant-to-the-user">relevant to the user</a>.:
vmpstr marked this conversation as resolved.
Show resolved Hide resolved

vmpstr marked this conversation as resolved.
Show resolved Hide resolved
vmpstr marked this conversation as resolved.
Show resolved Hide resolved
<ol>
vmpstr marked this conversation as resolved.
Show resolved Hide resolved
<li><p>Determine viewport proximity of all such elements.</p></li>

vmpstr marked this conversation as resolved.
Show resolved Hide resolved
<li><p>If any such determination resulted in an element being relevant to the user, <span>continue</span>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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.

  2. Determine proximity to the viewport for the element.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

</ol>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These cannot have the same indentation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

</li>

<li><p><span>Gather active resize observations at depth</span> <var>resizeObserverDepth</var> for
<var>doc</var>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possibly? <_<


<li><p>If <var>doc</var> <span>has active resize observations</span>:</p>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<ol>
vmpstr marked this conversation as resolved.
Show resolved Hide resolved
<li><p>Set <var>resizeObserverDepth</var> to the result of
<span data-x="broadcast active resize observations">broadcasting active resize
observations</span> given <var>doc</var>.</p></li>

<li><span>continue</span></li>
vmpstr marked this conversation as resolved.
Show resolved Hide resolved
</ol>
</li>

<li><span>break</span></li>
vmpstr marked this conversation as resolved.
Show resolved Hide resolved
</ol>
</li>

Expand Down