Update how FocusChangeDisabled is set to true in case of change of focus

[youennf]

The change covers potential race conditions where the change of focus task is already enqueued at the time the getDisplayMedia promise is resolved. Add a note to talk about the requirement behind these steps and that it should apply to any type of picker, whether user agent level or OS level.

Fixes #262

---

Preview | Diff

[eladalon1983]

What if focus is lost after this step executes?

[youennf]

The idea is that we start listening to focus loss for that controller at this exact point in time. And enqueue a task when that happens.
The If is indeed misleading. How about Whenever , or The first time .
Or maybe rephrase to Listen to ... losing focus. The first time it happens, queue...

[eladalon1983]

The "listen to..." or "start listening to..." variants seem promising.
And should this perhaps be done immediately after the Prompt the user to choose step?

[youennf]

Updated to listen to.
As of moving the step, I think this is good where it is.
Otherwise, we would potentially set FocusChangeDisabled to true before the user chose the surface (say getDisplayMedia is called, picker is shown, and user decides to unfocus/refocus the page before answering the prompt).

[eladalon1983]

The "listen to..." or "start listening to..." variants seem promising.
And should this perhaps be done immediately after the Prompt the user to choose step?

[eladalon1983]

The "resolve and abort these steps" bullet right before the new bullets, seems like it ensure they'd never be reached.

[jan-ivar]

We never get here because the previous step says to (Resolve p and) "abort these steps", so this doesn't work.

The subsequent statements also access realm-exposed objects in parallel which is a no-no.

Since (my understanding is) "Resolve p" from in-parallel steps is really short for: queue a task that resolves p, perhaps we meant to queue a task for the remaining steps? That would put them on main thread.

But since this seems time critical, it might be better to extract these variables ahead of the in parallel steps.

[jan-ivar]

This is mixing current and relevant concepts. Whenever we have a DOM object we should use what's relevant to that.

We also need to move these ahead of the in-parallel steps. Something like:

• Let mediaDevices be [=this=].
• Let toplevelTraversable be the mediaDevice's [=relevant global object=]'s
  [=navigable=]'s [=top-level traversable=].

(The mediaDevices temporary simplifies referencing it in prose below)

[jan-ivar]

Mixing async instructions into sequential steps can be tricky to understand. At what point does this listening begin? i.e. what is it synced to? For how long should it listen? If I call getDisplayMedia() twice, are there two listeners? etc. There's also no such concept as being able to "listen" to changes like this.

In the end the user agent is listening to itself here, so it's not clear to me what the requirements actually are. Focus changes seem highly UA specific, so maybe it's better to just spell out the intent?

Rather than constrain the user agent, it sounds like what we're trying to describe here is actually a constraint on the feature. Might this be described as an after-thought to the previous step using something like:

• Resolve p with stream.

  If a subsequent user action causes the toplevelTraversable to lose [=system focus=] before a
  focus decision has been finalized, the User Agent MUST set controller.[[FocusChangeDisabled]] to true.

• Abort the remaining steps.

Also, I don't think the User Agent should queue a task here, since the task to finalize the focus decision may already be in the queue, and if the user already switched focus, then we want the user to always win. AFAICT the [[FocusChangeDisabled]] internal slot isn't otherwise observable to JS, so this seems fine.

I also see we already have this (is it redundant now?):

"The user agent MAY set [[FocusChangeDisabled]] to true at any moment based on its own logic."