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

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

Open
wants to merge 3 commits into
base: gh-pages
Choose a base branch
from

Conversation

youennf
Copy link
Collaborator

@youennf youennf commented Mar 28, 2023

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

@youennf youennf force-pushed the update-focus-disabling-rule branch from dd6f1e1 to 04ede15 Compare March 28, 2023 09:49
index.html Outdated
@@ -423,6 +423,24 @@ <h2>
abort these steps.
</p>
</li>
<li>
<p>If the <a>current settings object</a>'s's [=relevant global object=]'s [=associated Document=]'s
Copy link
Member

Choose a reason for hiding this comment

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

What if focus is lost after this step executes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

index.html Outdated Show resolved Hide resolved
index.html Outdated
@@ -423,6 +423,24 @@ <h2>
abort these steps.
</p>
</li>
<li>
<p>If the <a>current settings object</a>'s's [=relevant global object=]'s [=associated Document=]'s
Copy link
Member

Choose a reason for hiding this comment

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

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?

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated
@@ -423,6 +423,28 @@ <h2>
abort these steps.
Copy link
Member

Choose a reason for hiding this comment

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

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

index.html Outdated
@@ -423,6 +423,28 @@ <h2>
abort these steps.
</p>
</li>
<li>
Copy link
Member

Choose a reason for hiding this comment

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

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.

index.html Outdated
Comment on lines 427 to 428
<p>Let <var>toplevelTraversable</var> be the <a>current settings object</a>'s's [=relevant global object=]'s
[=navigable=]'s [=top-level traversable=].</p>
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 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)

index.html Outdated
Comment on lines 431 to 433
<p>Listen to <var>toplevelTraversable</var>'s change of <a data-cite="!HTML/#system-focus">system focus</a>.</p>
<p>The first time <var>toplevelTraversable</var> is losing <a data-cite="!HTML/#system-focus">focus</a>,
queue a global task on the <a data-cite="!HTML/#user-interaction-task-source">user interaction task source</a>
Copy link
Member

Choose a reason for hiding this comment

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

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

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