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.

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

Add document.{parsed,contentLoaded,loaded} promises #1936

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

domenic
Copy link
Member

@domenic domenic commented Oct 19, 2016

This closes #127, although that thread also contains discussion about
adding other "loaded" promises for various elements, which will move
elsewhere. For a discussion of the names, including why jQuery's "ready"
naming was not used, see
#127 (comment).

I have heard rumblings of implementer interest for this, which is why I put in the effort. However, I'd like to make sure we get confirmed review from 2+ implementers before I proceed to write tests. So:

Also /cc @jakearchibald and @tabatkins as they have both championed this in https://lists.w3.org/Archives/Public/public-whatwg-archive/2014Mar/0091.html and in #127, respectively.

Finally, I'd like to add some developer-facing examples of using these. Let's discuss some ideas for that over in #127 though, not here.


💥 Error: Wattsi server error 💥

PR Preview failed to build. (Last tried on Jan 15, 2021, 7:57 AM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.

🔗 Related URL

Command failed: /home/noderunner/wattsi/bin/wattsi /tmp/upload_f9d6eef3e48bc910170199c6ba11b51a (sha not provided) lr6ugexgo4j default /tmp/upload_984f69a2e5019ab86d4130b73d7b76ed

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

@domenic domenic added addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest needs tests Moving the issue forward requires someone to write tests labels Oct 19, 2016
source Outdated
data-x="concept-document-loaded-promise">loaded promise</span> all to distinct new promises.</p>

<p class="note">This can occur due to <code
data-x="dom-document-open">document.open()</code>.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Is that the only reason? And would that still happen if we fixed #1698?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it would, unless you change more of the document.open/write/close model.

@annevk
Copy link
Member

annevk commented Oct 25, 2016

Mozilla would be interested in implementing this, but can't give you an ETA.

@dominiccooney
Copy link

I will implement this in Blink behind a flag to get feedback from developers.

Adding any property to document is fraught because it can shadow bindings on the global in inline event handlers. We'll try to gather feedback if not data on that.

@annevk
Copy link
Member

annevk commented Oct 25, 2016

@dominiccooney good point, why not just mark them [Unscopable] from the get go?

@domenic
Copy link
Member Author

domenic commented Oct 28, 2016

Great! I'll mark them [Unscopeable], and with two implementers in support, I'll start writing tests next week.

@rniwa
Copy link

rniwa commented Oct 31, 2016

I'm not certain I like the idea of adding three new attributes on document. How about something like document.whenReady('interactive')? I'm particularly concerned about document.interactive because that seems like the name we can use for something else.

@domenic
Copy link
Member Author

domenic commented Oct 31, 2016

Some more discussion in #whatwg at http://logs.glob.uno/?c=freenode%23whatwg&s=31%20Oct%202016&e=31%20Oct%202016#c1010329 with @rniwa. It emerged that he's mostly concerned about document.interactive using up a name that we might want in the future for a hypothetical "Interactive API". The other two he's not as concerned with.

So if we can come up with an alternate name that would help. Some suggestions were interactiveReady, interactionReady, readyToInteractive.

But apart from that, @rniwa is indifferent to the API, so we shouldn't really take this as a sign of support.

@domenic domenic removed the needs implementer interest Moving the issue forward requires implementers to express interest label Oct 31, 2016
@zcorpan
Copy link
Member

zcorpan commented Nov 1, 2016

How about a common prefix for these (nice for autocomplete etc), like:

  • whenInteractive
  • whenContentLoaded
  • whenLoaded

@domenic
Copy link
Member Author

domenic commented Nov 1, 2016

We haven't done that for other promise-returning properties, and it hurts usability. Worth considering though.

domenic added a commit to web-platform-tests/wpt that referenced this pull request Nov 4, 2016
@domenic
Copy link
Member Author

domenic commented Nov 4, 2016

Tests up at web-platform-tests/wpt#4171. [Unscopable] added. Ready for editor review.

@domenic domenic removed the needs tests Moving the issue forward requires someone to write tests label Nov 4, 2016
@bzbarsky
Copy link
Contributor

bzbarsky commented Nov 7, 2016

I'm a bit confused by the document.open handling here. Say I have a DOMContentLoaded event listener which calls document.open(). What will the state of the various promises involved be after https://html.spec.whatwg.org/#the-end step 4? As far as I can tell, the document will be in the "loading" state but its "content loaded promise" will be resolved...

@domenic
Copy link
Member Author

domenic commented Nov 7, 2016

Ah, yes, I thought that the order between events and promise operations did not matter due to microtask handling, but you're right and have found a case where it does. I think the fix is to resolve the promise first there, before firing the event. Then if firing the event causes it (and the others) to reset to a new promise via document.open, nothing bad happens.

Will fix and add such a test case.

@addyosmani
Copy link

addyosmani commented Nov 25, 2016

Briefly discussed this with @jakearchibald and he didn't feel it would cause too much confusion, but adding a note for reference:

When speaking to developers about document.interactive their first gut instinct has been that this refers to a moment during load when the document is actually interactive (layout has stabilized and the main thread is available enough to handle user input as most scripts have settled). Not when readyState becomes interactive. This definition of interactivity is closer to the WebPerf notion of "time to interactivity" as covered by something like https://github.com/tdresser/time-to-interactive (cc @tdresser).

I think loading states as promises add value to the platform, but am concerned about the confusion the interactive wording might have. readyState already had a notion of interactive prior to "time to interactive" being cared about as a metric in webdev circles, so if others don't have this concern that's fine. I know {interactive, loaded} are pretty terse and are also attractive in that regard.

@annevk
Copy link
Member

annevk commented Nov 26, 2016

Do you have an alternative term?

@rniwa
Copy link

rniwa commented Nov 26, 2016

How about document.contentLoaded?

@zcorpan
Copy link
Member

zcorpan commented Nov 28, 2016

@rniwa that's taken (the promise for DOMContentLoaded). 😊

Do web developers know when readyState changes to interactive? Looking at github search it appears it is not uncommon to assume it is when DOMContentLoaded fires. It's not clear to me that it is the best option to use the terminology of document.readyState.

The contentLoaded and loaded promises use wording for events rather than document.readyState, and nobody complains about these, right?

In #127 (comment) document.parsed was suggested as a possible name. While it doesn't match up with document.readyState, I think it is probably closer to what most people will think it does. Explaining it would be along the lines of "document.parsed resolves when the document is parsed, before deferred and async scripts have run. document.readyState is changed to interactive at the same time."

@jakearchibald
Copy link
Contributor

jakearchibald commented Nov 28, 2016

@zcorpan very few developers know this stuff. I made it a question at Chrome Dev Summit https://youtu.be/aWGU4qLtHEw?t=1h40m28s

When asked "What comes first? document.onload, readyState == 'interactive', or DOMContentLoaded?" here were the results:

  • 40% said document.onload
  • 40% said DOMContentLoaded
  • 20% said readyState == 'interactive'

@jakearchibald
Copy link
Contributor

Btw I like the general rule of following event names for equivalent promises, but given developer confusion around this, and @rniwa's concerns (although I don't share them), document.parsed feels much better.

@addyosmani
Copy link

document.parsed was suggested as a possible name. While it doesn't match up with document.readyState, I think it is probably closer to what most people will think it does.

I agree with @jakearchibald that document.parsed does feel better and would help minimize confusion.

@domenic
Copy link
Member Author

domenic commented Dec 9, 2016

I'll switch the tests and spec to document.parsed then. Thanks for the feedback all!

This closes #127, although that thread also contains discussion about
adding other "loaded" promises for various elements, which will move
elsewhere. For a discussion of the names, including why jQuery's "ready"
naming was not used, see
#127 (comment).
@hexalys
Copy link

hexalys commented Oct 19, 2018

However, as far as the HTML Standard goes there appear to be some misunderstandings. "The end" doesn't control painting

Nods. My reference to it is simply for the end point of parsing. Chrome stop in the middle of parsing and start painting in the middle of it indeed. I just can't make any good sense out of that...

Thanks for pointing out the event loop. Does 'rendering' mean 'paint' or 'layout' or either?

@annevk
Copy link
Member

annevk commented Oct 19, 2018

Both, basically. And parsing routinely queues tasks to update the node tree, at which point the browser could decide to render.

@jakearchibald
Copy link
Contributor

jakearchibald commented Oct 19, 2018

@hexalys I wasn't intending to be dismissive. I thought I'd provided enough evidence in a form you could verify, and no further info was needed.

I've updated it to show the document ready state at the top of the page: https://progressive-render.glitch.me/.

Can you see (like I can in Chrome, Edge, Safari and Firefox) that the document.readyState remains loading, but content is rendered on the page? Browsers have behaved like this for decades.

At the moment, there is no recourse to prevent such unpredictable early live painting on screen.

Sure there is. At the top of your document:

<style>html { display: none; }</style>

Then at the bottom of your document:

<style>html { display: block; }</style>

(or change the value with JavaScript if you want to reveal the content some other time)

@jakearchibald
Copy link
Contributor

Here's the server code for the demo, in case it isn't clear: https://glitch.com/edit/#!/progressive-render?path=server.js

@jakearchibald
Copy link
Contributor

@hexalys

Here's the demo running in Firefox 3 (2008):

screen shot 2018-10-19 at 12 19 19

Seems like Firefox didn't support readyState back then, but the deferred script on that page turns the background of the page red. Since it isn't red, it's rendering before deferred scripts are executed.

Here's IE8 (2009):

screen shot 2018-10-19 at 12 21 20

Note that readyState is interactive, which is a bug. However, the defered script still hasn't executed.

@hexalys
Copy link

hexalys commented Oct 20, 2018

@jakearchibald
You keep looking at it from a purely theoretical perspective ignoring my updated context... The repro use cases or exceptional cases like the long HTML spec are besides my point. I was aware of chunked transfer rendering before, or this kind of evidence. And if the UA has nothing to do and no events are coming up. Obviously it should paint, no question.

I am speaking of a more practical sense in the context of an average page, with only a few scripts and early DOM manipulations, like my example; which validates my case of a unique Chrome exception "in practice".

Ask yourself. How are you suppose to insert critical dynamic content into a page if you can’t predict consistently whether the page is already shown or not? That's a non-starter for me, in the way I think about showing a page, or the way I think about the best performance and experience for my users.
What makes Chrome paint early in this case, are things I am more interested in...

The CSS display method is one hack. Thanks for mentioning! I might use that if I must. But I am hoping for a more standard long term solution. I'd be foolish to expect getting very far in convincing Chrome to change any of this in the short term.

@jakearchibald
Copy link
Contributor

jakearchibald commented Oct 20, 2018

@hexalys you made claims about the spec and implementations that have been shown to be false. I realise that can be embarrassing, especially since you made those claims so aggressively, and boasted about your experience.

You've seen the evidence. Instead of trying to find ways to dismiss it, do some more research of your own. Throttle your connection, browse the web, observe the details in dev tools. Consider that you may not be infallible.

I was aware of chunked transfer rendering before

This has nothing to do with chunked transfer encoding.

How are you suppose to insert critical dynamic content into a page if you can’t predict consistently whether the page is already shown or not?

The best way is to use your frontend skills to make a stable progressive render. I've written about this before: https://jakearchibald.com/2014/dont-use-flexbox-for-page-layout/.

The CSS display method is one hack. Thanks for mentioning! I might use that if I must. But I am hoping for a more standard long term solution.

Using CSS to control the display of content isn't a hack. That's what it's made for.

I'd be foolish to expect getting very far in convincing Chrome to change any of this in the short term.

After all these examples, in multiple browsers, over 10 years, you're still clinging to this being a recent Chrome thing?

I believe you may have an example that has changed in Chrome from your perspective (in a way that's fully spec compliant). But you cannot flip a coin once and conclude "it's always heads!", especially when others have demonstrated otherwise.

@jonathantneal
Copy link
Contributor

First, I’m usually wrong, so I’m sorry if I have errantly believed that the document's readyStateChange event (where the readyState is complete) and the window's load event are effectively synonymous. That’s my presupposition as you continue reading.

I received a report (in jonathantneal/document-promises#14) that browsers are handling complete and load differently. My speculative polyfill relies on the document being complete rather than the window's load event.

The reports states that, in Safari, executing lots of code on complete causes it to overstep the load event, thus failing to execute load code. In Chrome, the same scenario executes load at a later point in time due to the code execution on complete.

Reviewing this PR again, I could not figure out what should happen when lots of code is executed from document.loaded. Should it have the same effect as what was reported? Or is it just me providing a faulty implementation? Should there be guidance in this PR to reduce browser discrepancies (if there are in fact discrepancies)?

@bzbarsky
Copy link
Contributor

bzbarsky commented Dec 5, 2018

Per spec, https://html.spec.whatwg.org/multipage/parsing.html#the-end step 7 fires the two events one right after the other. Doing stuff in the readystatechange event listeners should not affect "load" firing, apart from the listeners needing to finish running before it fires.

As far as I can tell, that's what Firefox implements. I can't speak for other implementations...

In terms of what this PR does, it's resolving the promise right before firing the readystatechange event. What that means in terms of when the promise handlers run depends on whether there are readystatechange listeners. If there are, it will run after the first such listener, afaict, because that will be the next microtask checkpoint. If there are not, it will run after the first load listener, because that will be the next microtask checkpoint. If there are no listeners for either event, they will run after this all unwinds, I think.

Maybe we want an explicit microtask checkpoint after we resolve the promise and before we fire readystatechange?

@annevk
Copy link
Member

annevk commented Dec 6, 2018

It should probably resolve after firing. I think that's the usual pattern for these kind of things.

@bzbarsky
Copy link
Contributor

bzbarsky commented Dec 6, 2018

After firing just the readystatechange, or also after firing "load"?

@smaug----
Copy link

Sprinkling microtask checkpoints to different places would be a bit unfortunate. Breaks the generic concept of microtasks. I think we've managed to avoid that quite well. (script element execution is a special case). In my mind if one thinks there is a missing microtask checkpoint, it usually means certain tasks should be split to two pieces (and then the checkpoint would naturally happen at the end of the first task). But not sure how that applies in this case.

@bzbarsky
Copy link
Contributor

bzbarsky commented Dec 6, 2018

In this case, that would look like separate tasks for firing the "readystatechange" and "load" events or something like that. Then other tasks could run in between those, but that might be ok.

Another option is to move the firing of "load" into the readystate change, since now we explicitly have it take different actions based on what state is being changed to, and resolve the promise after both are fired.

@smaug----
Copy link

Yeah, I guess conceptually there could be two tasks, but those would be in a way guaranteed to run sequentially and no tasks between them, so in practice implementation could have just one task and an extra checkpoint. That is quite close to how I've been thinking script element handling.

@bzbarsky
Copy link
Contributor

bzbarsky commented Dec 6, 2018

but those would be in a way guaranteed to run sequentially and no tasks between them

No tasks from the same task source. Other task sources could run in between.

@annevk
Copy link
Member

annevk commented Dec 6, 2018

If they're in the same task, I'd expect to first fire all events, and then resolve any promises. We should document that pattern somewhere.

@dead-claudia
Copy link

@hexalys Just wanted to point out that on Safari 12.1, I get the same issue as on Chrome (the flash of one button instead of 2 on first few paints) about 90% of the time when loading from cache but 0% of the time when loading from network (forced via dev tools), and this is identical to what I experience on Chrome. I can confirm I don't experience this issue on Firefox, however. It's not Chrome-specific, but might have to do with code common between Webkit and Blink (which started as a Webkit fork).

In either case, it's probably more appropriate to file bugs against Safari and Chrome than to discuss it here.

@jakearchibald
Copy link
Contributor

If they're in the same task, I'd expect to first fire all events, and then resolve any promises. We should document that pattern somewhere.

fwiw w3ctag/promises-guide#71

@WickyNilliams
Copy link

Hey all. I'm curious, what is the status of this? Has it stalled?

@dead-claudia
Copy link

Is there anything I could do to help move this along?

@annevk
Copy link
Member

annevk commented Jan 9, 2024

A preffed-off implementation in a browser engine would help move this along.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements
Development

Successfully merging this pull request may close these issues.

Add document.ready promise