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

Integrate "request permission to use" with user activation #401

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

marcoscaceres
Copy link
Member

@marcoscaceres marcoscaceres commented Nov 25, 2022

Allows "request permission to use" to accept an optional promise.

The promise gets rejected if the page doesn't have transient activation. Also, when the promise is passed, the algorithm now consumes the user activation. If passed, the promise is resolved with the user's choice.

closes #194


Preview | Diff

Allows "request permission to use" to accept an optional promise.

The promise gets rejected if the page doesn't have transient activation.
Also, when the promise is passed, the algorithm now consumes the user activation.
If passed, the promise is resolved with the user's choice.
@marcoscaceres
Copy link
Member Author

Expected use would be something like:

  1. Let |promise:Promise| be a [=newly created promise=] in |this|'s [=relevant realm=].
  2. Return |promise| and in |parallel|:
    1. [=Request permission=] to use "my-api" with |promise|.
    2. [=React=] to [=promise=]:
    • If it's the case that |promise| is fulfilled with value {{PermissionState/"granted"}}, then:
      1. ... do granted stuff...
    • If it's the case that |promise| is fulfilled with value {{PermissionState/"denied"}}, then:
      1. ... do denied stuff...

@marcoscaceres
Copy link
Member Author

I kinda like this, because it works for legacy APIs (e.g. Geolocation) and for new APIs.

@annevk
Copy link
Member

annevk commented Nov 25, 2022

The expected usage there is wrong. You cannot react to a promise while in parallel. You'd have to queue a task.

Also, I think Notifications is currently a permission-thingie for which no user activation is required in one implementation (Chromium), but maybe that should be considered a bug.

Comment on lines +663 to +667
<li>If |global| does not have [=transient activation=], [=queue a global task=] on
the [=user interaction task source=] given |global| to [=reject=] |promise| with a
"{{NotAllowedError}}" {{DOMException}} and abort these steps.
</li>
<li>Otherwise, [=consume user activation=] of |global|.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this really works if "requesting permission to use" is called while in parallel as the transient activation check really has to happen in the main thread.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, true...

As a shorthand, <a>requesting permission to use</a> a {{DOMString}} |name|, is the same
as <a>requesting permission to use</a> a {{PermissionDescriptor}} with its
As a shorthand, <a>requesting permission to use</a> a {{DOMString}} |name| and |promise|,
is the same as <a>requesting permission to use</a> a {{PermissionDescriptor}} with its
Copy link
Member

@miketaylr miketaylr Dec 1, 2022

Choose a reason for hiding this comment

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

Can you give an example of using this shorthand? Looking at https://notifications.spec.whatwg.org/#dom-notification-requestpermission... is it something like requesting permission to use "notifications" with |promise|? If so, s/and/with/?

(And probably {{PermissionDescriptor/name}} member set to |name| with |promise| below?)

Comment on lines +653 to +654
"export">request permission to use</dfn> a |descriptor:PermissionDescriptor|, with
optionally a |promise:Promise|:
Copy link
Member

Choose a reason for hiding this comment

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

We need to make call sites easy to read, and I don't think "request permission to use |descriptor| with |promise|" explains to readers what the promise is going to mean. Readers shouldn't need to click through in order to read the call site, which means the call site needs to mention [=transient activation=]. I think something like

Suggested change
"export">request permission to use</dfn> a |descriptor:PermissionDescriptor|, with
optionally a |promise:Promise|:
"export">request permission to use</dfn> a |descriptor:PermissionDescriptor|, optionally
with [=transient activation=] consumed through a |promise:Promise|:

would be somewhat better, although "consumed through" is still obscure, and you might come up with better ideas.

Copy link
Member

@annevk annevk Dec 2, 2022

Choose a reason for hiding this comment

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

In that case you should define a named parameter: https://infra.spec.whatwg.org/#algorithm-params. (It's not clear to me how a caller would invoke the suggestion. Transient activation is also a property of a Window object.)

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm... I wonder then if we should define an entirely new algorithm that returns a promise (is not passed one), then see how many specs we can migrate over.

That would allow geolocation (and maybe other specs) to keep working, but provide the right infra for this.

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

Successfully merging this pull request may close these issues.

Consider making request-permission-to-use aware of user activation
4 participants