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

Allow gUM prompt ahead of focus + deterministic "visible" enumeration wo/focus #912

Merged
merged 9 commits into from
Dec 15, 2022

Conversation

jan-ivar
Copy link
Member

@jan-ivar jan-ivar commented Oct 27, 2022

Fixes #752:

  1. Relax document (iframe) focus to tlbc system focus (a step in the right direction)
  2. Allow Safari prompts in foreground tabs, while maintaining focus requirement on allow, per Broken foreground detection #752 (comment)
  3. Remove focus requirement on enumerateDevices, per Broken foreground detection #752 (comment)
  4. Fix race spotted in Why does navigator.mediaDevices.enumerateDevices() require that Document must have active keyboard focus? #905 (comment) which prevented deterministic visibility-success checks:
    if (document.visibilityState == "visible") {
      await navigator.mediaDevices.enumerateDevices(); // won't block on hidden thanks to if
    }

Preview | Diff

Copy link
Member

@eladalon1983 eladalon1983 left a comment

Choose a reason for hiding this comment

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

Generally LGTM.

Comment on lines 2879 to 2883
<p>If <var>proceed</var> is `false`, the [=User Agent=]
MUST wait to proceed to the next step until a task queued
to set <var>proceed</var> to the result of
[=device enumeration can proceed=], sets
<var>proceed</var> to `true`.</p>
Copy link
Member

Choose a reason for hiding this comment

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

The prose here was difficult for me to process. How about simplifying it with something along the lines of "...queue a task to do X. Once the task resolves, proceed to the next steps."

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm open to simpler ways to say it, but queued tasks don't "resolve" AFAIK. Also, waiting for the task alone is not enough, since that might proceed on false. The intent here is to describe the same criteria as before without the cross-thread access.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've done a tweak with s/If/While/ combined with broken out algorithms that are hopefully easier on the eyes. PTAL.

getusermedia.html Outdated Show resolved Hide resolved
getusermedia.html Outdated Show resolved Hide resolved
getusermedia.html Outdated Show resolved Hide resolved
getusermedia.html Show resolved Hide resolved
Comment on lines 3479 to 3481
the [=relevant global object=]'s [=associated `Document`=] is
[=Document/fully active=] and its [=Document/visibility state=]
is `"visible"`.</p>
Copy link
Member

Choose a reason for hiding this comment

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

As a follow up, I wonder if we could refine our concept of focus to accommodate the possibility that UA-level elements might override the app's focus even when visible, e.g. if the user is interacting with DevTools, the omnibar, bookmarks drop-down lists, or any future UA-level element of arbitrary complexity which could be prominently displayed and commanding the user's attention.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's whatwg/html#6211, which is hopefully not a blocker for this PR which moves us a step closer.

getusermedia.html Outdated Show resolved Hide resolved
getusermedia.html Outdated Show resolved Hide resolved
@jan-ivar jan-ivar marked this pull request as ready for review November 16, 2022 15:04
@alvestrand
Copy link
Contributor

@guidou is this deployable?

@jan-ivar jan-ivar requested a review from guidou December 8, 2022 15:30
@eladalon1983 eladalon1983 self-requested a review December 8, 2022 15:30
@guidou
Copy link
Contributor

guidou commented Dec 13, 2022

@guidou is this deployable?

I think this is an improvement over the focus requirement. In terms of deployability, we'll have to implement and evaluate it. The focus requirement introduced serious regressions for many of Chromium users and we had to roll it back.

jump to the step labeled <em>Permission Failure</em> below.</p>
</li>
<li>
<p>Let <var>hasSystemFocus</var> be `false`.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean we still need to have system focus?

Copy link
Member Author

Choose a reason for hiding this comment

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

In getUserMedia, yes. The focus requirement here is just moved down to allow prompting ahead of focus, like Safari does.

Copy link
Contributor

@alvestrand alvestrand left a comment

Choose a reason for hiding this comment

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

This seems to be a more deployable change than the change from present behavior that is currently in the spec, so it's an improvement. We'll see if it can deploy.

jan-ivar added a commit to jan-ivar/mediacapture-main that referenced this pull request Mar 3, 2023
jan-ivar added a commit to jan-ivar/mediacapture-main that referenced this pull request Mar 3, 2023
henbos added a commit that referenced this pull request May 11, 2023
Remove focus from mute/unmute SHOULD prose to match #912 best practice.
youennf added a commit that referenced this pull request May 11, 2023
Remove focus requirement on enumerateDevices, for real this time #912.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken foreground detection
4 participants