-
Notifications
You must be signed in to change notification settings - Fork 28
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
base: gh-pages
Are you sure you want to change the base?
Conversation
dd6f1e1
to
04ede15
Compare
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
@@ -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 |
There was a problem hiding this comment.
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?
3f69219
to
99f2b39
Compare
index.html
Outdated
@@ -423,6 +423,28 @@ <h2> | |||
abort these steps. |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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
<p>Let <var>toplevelTraversable</var> be the <a>current settings object</a>'s's [=relevant global object=]'s | ||
[=navigable=]'s [=top-level traversable=].</p> |
There was a problem hiding this comment.
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
<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> |
There was a problem hiding this comment.
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."
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