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

Export the definition for "completely loaded" #8382

Closed
wants to merge 1 commit into from

Conversation

cbiesinger
Copy link

@cbiesinger cbiesinger commented Oct 12, 2022

I want to reference this point in time in another spec (FedCM).

This is editorial only, so I'll skip the checkboxes below.

  • At least two implementers are interested (and none opposed):
  • Tests are written and can be reviewed and commented upon at:
  • Implementation bugs are filed:
    • Chrome: …
    • Firefox: …
    • Safari: …
    • Deno (only for timers, structured clone, base64 utils, channel messaging, module resolution, web workers, and web storage): …
    • Node.js (only for timers, structured clone, base64 utils, channel messaging, and module resolution): …

(See WHATWG Working Mode: Changes for more details.)


/browsing-the-web.html ( diff )

I want to reference this point in time in another spec (FedCM).
@domenic
Copy link
Member

domenic commented Oct 13, 2022

Can you outline the case in more detail? Generally the reason we don't export these sorts of things is to prevent monkey patching. So e.g. it might be the case that you should be modifying the HTML spec to reference something it needs to run in the FedCM spec at load time instead.

@cbiesinger
Copy link
Author

The use case is this:

We want to support multiple IDPs in the same dialog for FedCM, preferably in a way that does not require the different SDKs to cooperate with each other. To support things like Google's one tap dialog with its declarative API but also other SDKs that offer only an imperative API, we came up with this:

  • We merge all calls to the FedCM API that happen before or during onload, and process them immediately after onload
  • To also support multi IDP after onload, we merge calls that happen during the same task

So, I wanted to spec this by saying something like "If the call happens before the document is [=fully loaded=], delay further processing until right after the onload event is fired" (that last part still needs some spec wordsmithing).

Thoughts?

@cbiesinger
Copy link
Author

Adding @annevk in case they have thoughts on the above

@annevk
Copy link
Member

annevk commented Oct 14, 2022

I would recommend defining the full model as Domenic also hinted at. Based on some "load happened" boolean you'd either put calls in a list or run them directly. Then when load happens you process the calls in the list.

@cbiesinger
Copy link
Author

I would recommend defining the full model as Domenic also hinted at. Based on some "load happened" boolean you'd either put calls in a list or run them directly. Then when load happens you process the calls in the list.

Well yes, but the load happened boolean is not exported...

But OK, I will share the full text here when I have it

@cbiesinger
Copy link
Author

@annevk / @domenic , please see w3c-fedid/FedCM#351 for the full algorithm I'm defining where I'm trying to use this definition.

@domenic
Copy link
Member

domenic commented Oct 16, 2022

Having perused https://github.com/fedidcg/FedCM/pull/351/files, this looks like a legitimate use of "fully loaded" to me, in the sense of it querying the state as part of the external interface. So from that perspective I'm inclined to approve the export.

However, I do worry about other parts of that PR. Some minor things, like "the Document" and "the Window" (which one??) or "all promises are rejected" (by who?? with what??). And one major thing, which is that we generally don't want specifications using the event listener infrastructure, because it causes nondeterminism relative to other event listeners that have been attached by author code. That indicates that a better processing model would probably be exporting something from FedCM, and having HTML call it somewhere in https://html.spec.whatwg.org/#the-end , or maybe https://html.spec.whatwg.org/#completely-finish-loading.

Right now your processing happens sometime during step 9.5, but do you really want to count FedCM stuff the browser does as part of the load timing info that is passed to RUM providers? Are you sure you want load event handlers that happen to be attached by sync scripts to fire before the FedCM stuff, but those attached by defer scripts to fire after? Is it OK that none of these steps happen in about:blank iframes/opened windows, which don't get load events? (Probably that is actually OK, but just checking.)

If you were to flip the processing model that way, then FedCM would probably have its own per-Document "list of stuff to run after load", which it could add to. HTML would call FedCM's "stop delaying the X stuff", which would run through all the algorithms in the list.

What do you think of that alternate strategy?

@cbiesinger
Copy link
Author

If updating the HTML spec in that way is an option, I agree that that would be better. (Though implementing it would be harder in Chrome, since it is in a module, which can't directly be called from core/dom)

I don't understand why defer scripts would affect this, but perhaps I did not follow all the steps for them to the end. Don't event handlers they attach get called together with any other event handlers, and thus get executed before the event that gets queued in my draft PR?

Thanks for the other feedback on the PR, I will fix that.

@cbiesinger
Copy link
Author

That said, I believe I still need this definition to be exported (i.e. this PR merged) so I can know whether to delay processing in this way?

@domenic
Copy link
Member

domenic commented Oct 18, 2022

I don't understand why defer scripts would affect this, but perhaps I did not follow all the steps for them to the end. Don't event handlers they attach get called together with any other event handlers, and thus get executed before the event that gets queued in my draft PR?

In your draft PR, we would have three things adding event listeners, in the following order: (1) sync scripts; (2) FedCM; (3) defer scripts. Thus, when we fire the load event, first the sync scripts' event listeners would run; then the FedCM event listener would run; then the defer scripts' event listeners would run.

Your draft PR doesn't queue any events; it adds an event listener. And event listeners are executed in the order they are added.

That said, I believe I still need this definition to be exported (i.e. this PR merged) so I can know whether to delay processing in this way?

No, this would not be necessary. If HTML calls into FedCM, you can store and consult your own boolean.

@cbiesinger
Copy link
Author

I don't understand why defer scripts would affect this, but perhaps I did not follow all the steps for them to the end. Don't event handlers they attach get called together with any other event handlers, and thus get executed before the event that gets queued in my draft PR?

In your draft PR, we would have three things adding event listeners, in the following order: (1) sync scripts; (2) FedCM; (3) defer scripts. Thus, when we fire the load event, first the sync scripts' event listeners would run; then the FedCM event listener would run; then the defer scripts' event listeners would run.

Your draft PR doesn't queue any events; it adds an event listener. And event listeners are executed in the order they are added.

It adds an event listener and the listener then queues an event:

  1. [=Queue a global task=] where the global object is the current window
    and the task source is the federated credential management
    task source and steps is "go to the next step".

That said, I believe I still need this definition to be exported (i.e. this PR merged) so I can know whether to delay processing in this way?

No, this would not be necessary. If HTML calls into FedCM, you can store and consult your own boolean.

Is the suggestion that I have a boolean that defaults to false and the callback from HTML would then set it to true? That would work.

@domenic
Copy link
Member

domenic commented Oct 19, 2022

It adds an event listener and the listener then queues an event:

Ah right. So to see observable consequences we'd have to look at defer-script-added load event listeners that also queue a task.

Is the suggestion that I have a boolean that defaults to false and the callback from HTML would then set it to true? That would work.

Yep!

@cbiesinger
Copy link
Author

Thanks for all the advice here; I'll close this for now because after further discussion the API shape may change not to need this anymore. If we do end up going with this API shape I will add the call into FedCM in a new PR. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants