Include algorithms and concepts from Element Timing and LCP #100

clelland · 2023-11-16T18:51:33Z

This is part of a larger refactor of all three specs, making this spec (Paint Timing) the foundational spec for all paint-timing-related standards.

Preview | Diff

This is part of a larger refactor of all three specs, making this spec (Paint Timing) the foundational spec for all paint-timing-related standards.

yoavweiss

Thanks for working on this!!

yoavweiss · 2023-11-22T04:02:23Z

index.bs

+Recording paint timing {#sec-recording-paint-timing}
+--------------------------------------------------------
+
+<h4 id="sec-modifications-dom">Modifications to the DOM specification</h4>


I don't think we want this to monkey-patch DOM or other specifications.

I think it's fine to define the "associated image request" concept here, and then populate it in the various image loading algorithms in HTML, given the fact that this supports LCP, which has cross-implementer support. It's also fine to do this in multiple stages (land the vague language now, while PRing hooks into the processing model)

yoavweiss · 2023-11-22T04:05:06Z

index.bs

+
+When the user agent is executing the <a>painting order</a>, it must populate the <a>set of owned text nodes</a> of the painted {{Element|Elements}} so that the following is true:
+
+<div algorithm="text aggregation">


Can this be defined as an algorithm that would then be called from the processing model that populates the set of owned text nodes?

noamr

Thanks for working on this!
I feel that a lot of the wording here is vague... is it copied from the existing LCP/element-timing spec? I'd feel more comfortable with using this opportunity to make the spec language more rigorous.

noamr · 2023-11-22T13:34:55Z

index.bs

+
+<h4 id="sec-modifications-CSS">Modifications to the CSS specification</h4>
+
+When the user agent is executing the <a>painting order</a>, it must populate the <a>set of owned text nodes</a> of the painted {{Element|Elements}} so that the following is true:


This is a bit strange. How is the painting order (which is about stacking context) related here? Sounds like something that belongs in an implementation

Good question! (And yes, it was moved over from element timing, where it was added pretty early, and maybe could have had more review at the time).

"Executing the painting order" isn't really something that happens: things are painted, and the painting order defines the order that they are painted in, but nothing in that section appears to be an algorithm that gets run when painting actually happens.

index.bs

noamr · 2023-11-22T13:43:52Z

index.bs

+When the processing model for the {{Element}} <em>element</em>'s style requires a new image resource (to be displayed as background image), the <a>image request</a> created by the new resource is appended to <em>element</em>'s <a>associated background image requests</a>.
+Whenever an <a>image request</a> in an {{Element}} <em>element</em>'s <a>associated background image requests</a> becomes <a>completely available</a>, run the algorithm to <a>process an image that finished loading</a> with <em>element</em> and <a>image request</a> as inputs.
+
+NOTE: we assume that there is one <a>image request</a> for each {{Element}} that a <a>background-image</a> property affects and for each URL that the <a>background-image</a> property specifies.


This needs to be reworded as a clear example.

Reworded. Hopefully clear.

index.bs

noamr · 2023-11-22T13:49:06Z

index.bs

+        1. If |element|'s <a>set of owned text nodes</a> is empty, continue.
+        1. <a for="set">Append</a> |element| to |doc|'s <a>set of elements with rendered text</a>.
+        1. [=set/Append=] |element| to |paintedTextNodes|.
+    1. Run [=the paint timing steps=] given |document|, |paintTimestamp|,


We don't usually have this pluggable pattern in specs... it's better to call the specific ones

Is there a better spec pattern that can be used to provide a hook here for related specs?
LCP and ET both append steps to the paint timing steps in the current model, for instance. I'd like to have a way to make this extensible, without having to reference those specs here.

Referencing them here would be the pattern. That's how we do that in the HTML spec for instance, it references many other specs that might be at a "higher level".

Yea I would simply call:

Report paint timing given ...

Report element timing given...

index.bs

clelland · 2023-11-22T20:29:20Z

Thanks for working on this! I feel that a lot of the wording here is vague... is it copied from the existing LCP/element-timing spec? I'd feel more comfortable with using this opportunity to make the spec language more rigorous.

Agreed. Much of the text for this was moved around between specs, and I don't know when it was last reviewed on its own :)
Some of this can be tightened up, for sure. Thanks for pointing out the worst offenders there

index.bs

noamr · 2023-12-01T15:10:32Z

index.bs

+
+<h4 id="sec-modifications-CSS">Modifications to the CSS specification</h4>
+
+Whenever an <a>image request</a> in an {{Element}} <em>element</em>'s <a>associated background image requests</a> becomes <a>completely available</a>, run the algorithm to <a>process an image that finished loading</a> with <em>element</em> and <a>image request</a> as inputs.


Perhaps it's time to upstream these to CSS/HTML soon? LCP is implemented by Gecko so should be good to go...

index.bs

noamr · 2023-12-01T15:19:17Z

index.bs

+        1. If |element|'s <a>set of owned text nodes</a> is empty, continue.
+        1. <a for="set">Append</a> |element| to |doc|'s <a>set of elements with rendered text</a>.
+        1. [=set/Append=] |element| to |paintedTextNodes|.
+    1. Run [=the paint timing steps=] given |document|, |paintTimestamp|,


Yea I would simply call:

Report paint timing given ...

Report element timing given...

index.bs

This eliminates the dependency on the still-incubating ElementTiming spec, by using concepts newly-introduced into Paint Timing. The concept of LCP-eligibility is also added, to encapsulate the various heuristics used to filter out elements which are not the primary content of the page (though which would still be considered "contentful" for purposes of calculating First Contentful Paint time.) Closes: #86, #113

Include algorithms and concepts from Element Timing and LCP

08eb32e

This is part of a larger refactor of all three specs, making this spec (Paint Timing) the foundational spec for all paint-timing-related standards.

clelland marked this pull request as draft November 16, 2023 18:52

clelland requested a review from yoavweiss November 16, 2023 19:04

Add "Exposed for paint timing" algorithm"

be4f2b1

clelland marked this pull request as ready for review November 21, 2023 18:40

clelland added a commit to w3c/largest-contentful-paint that referenced this pull request Nov 21, 2023

Rebase LCP on top of w3c/paint-timing#100

b082015

This was referenced Nov 21, 2023

Rebase LCP on top of w3c/paint-timing#100 w3c/largest-contentful-paint#121

Merged

Refactor Element Timing on top of Paint Timing WICG/element-timing#78

Merged

yoavweiss reviewed Nov 22, 2023

View reviewed changes

noamr reviewed Nov 22, 2023

View reviewed changes

Updates

938d6d6

noamr reviewed Dec 1, 2023

View reviewed changes

clelland added 3 commits December 20, 2023 22:35

Review comments

04d5857

Addressing a few more review comments

6665f70

Add one more call to element timing

67cff83

noamr reviewed Jan 3, 2024

View reviewed changes

index.bs Show resolved Hide resolved

Fix numbering

374686c

noamr reviewed Jan 3, 2024

View reviewed changes

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Show resolved Hide resolved

clelland added 2 commits January 3, 2024 14:12

Remove extra line

935482a

Reword example

d50ac84

noamr approved these changes Jan 3, 2024

View reviewed changes

clelland merged commit ba58148 into main Jan 10, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include algorithms and concepts from Element Timing and LCP #100

Include algorithms and concepts from Element Timing and LCP #100

clelland commented Nov 16, 2023 •

edited by pr-preview bot

yoavweiss left a comment

yoavweiss Nov 22, 2023

yoavweiss Nov 22, 2023

noamr left a comment

noamr Nov 22, 2023

clelland Nov 22, 2023

noamr Nov 22, 2023

clelland Nov 29, 2023

noamr Nov 22, 2023

clelland Nov 22, 2023

noamr Nov 22, 2023

noamr Dec 1, 2023

clelland commented Nov 22, 2023

noamr Dec 1, 2023

noamr Dec 1, 2023


		When the user agent is executing the <a>painting order</a>, it must populate the <a>set of owned text nodes</a> of the painted {{Element\|Elements}} so that the following is true:

		<div algorithm="text aggregation">


		<h4 id="sec-modifications-CSS">Modifications to the CSS specification</h4>

		When the user agent is executing the <a>painting order</a>, it must populate the <a>set of owned text nodes</a> of the painted {{Element\|Elements}} so that the following is true:


		<h4 id="sec-modifications-CSS">Modifications to the CSS specification</h4>

		Whenever an <a>image request</a> in an {{Element}} <em>element</em>'s <a>associated background image requests</a> becomes <a>completely available</a>, run the algorithm to <a>process an image that finished loading</a> with <em>element</em> and <a>image request</a> as inputs.

Include algorithms and concepts from Element Timing and LCP #100

Include algorithms and concepts from Element Timing and LCP #100

Conversation

clelland commented Nov 16, 2023 • edited by pr-preview bot

yoavweiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noamr 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

clelland commented Nov 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clelland commented Nov 16, 2023 •

edited by pr-preview bot