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

Move user activation to obtain permission #449

Merged

Conversation

beaufortfrancois
Copy link
Collaborator

@beaufortfrancois beaufortfrancois commented Nov 26, 2019

This PR aims to move the "triggered by user activation" bit from scan and push operations to the "obtain permission" section.

w3c/permissions#194 tracks the progress of updating the permissions API algorithm to state that user activation is required for showing a permission prompt if the permission state is "prompt".
@mustaqahmed what's the current status?


Preview | Diff

@zolkis
Copy link
Contributor

zolkis commented Nov 26, 2019

+1 for moving it to the obtain permission steps.

Copy link
Contributor

@kenchris kenchris left a comment

Choose a reason for hiding this comment

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

Will let it up to you to merge when it is the right time

@zolkis
Copy link
Contributor

zolkis commented Nov 26, 2019

Now the user activation clauses are removed, but not explicitly present in the obtain permission steps.

@mustaqahmed
Copy link
Member

mustaqahmed commented Nov 26, 2019

We have w3c/permissions#194 to add checks for user activation in the Permission API; my understanding is that it is currently waiting for UAv2 HTML change to land. @jyasskin?

@beaufortfrancois
Copy link
Collaborator Author

@mustaqahmed Now that User Activation v2 model is in the HTML spec now, how should we change the Web NFC API spec?

@marcoscaceres
Copy link
Member

See my comments in the related HTML bug. Let’s figure out the wording to use across specs and then fix a bunch of affected ones all at once (including wake lock).

It’s worth us trying to do a first round as a pull request (see my attempt in Payment Request as a pull request) so then HTML can give better guidance.

@marcoscaceres
Copy link
Member

My go at this from Payment Request w3c/payment-request#885

@beaufortfrancois
Copy link
Collaborator Author

beaufortfrancois commented Dec 6, 2019

@marcoscaceres It looks good. Hopefully @mustaqahmed will LGTM.

@mustaqahmed
Copy link
Member

@beaufortfrancois: Correct me if I missed anything but I thought the goal here is to make web-nfc indirectly depend on user activation, through Permissions, no? Since permission prompt is gated by user activation, web-nfc spec algorithms can perhaps omit direct links to user activation state?

In any case, let us first have a "canary PR" landed first as @marcoscaceres suggested here. It seems the HTML spec needs to expose a few hidden definitions.

@zolkis
Copy link
Contributor

zolkis commented Dec 6, 2019

Thanks @mustaqahmed , that makes sense.
This can be merged at the nearest convenience.

@marcoscaceres
Copy link
Member

We are getting closer on the payment request PR... please don't merge this one tho, it definitely needs to be changed. Thankfully, I think the changes are straight forward... but deleting those steps is not right. They need to be adapted to the new model.

You also need to decide (as a working group) if the user activation gets consumed by the API call.
See also: w3c/payment-request#886

@beaufortfrancois
Copy link
Collaborator Author

@marcoscaceres Thanks for the ping.
I believe we want the user activation to be consumed by the API call.

@beaufortfrancois beaufortfrancois added the Origin Trial Issues that would be good to solve before OT label Dec 11, 2019
@beaufortfrancois beaufortfrancois self-assigned this Dec 11, 2019
@beaufortfrancois
Copy link
Collaborator Author

@mustaqahmed If permission prompt is gated by user activation, I guess we can omit user activation state. In other words, this PR is good as it is.

Does https://w3c.github.io/permissions/#request-permission-to-use needs to be updated as well?

@marcoscaceres
Copy link
Member

marcoscaceres commented Dec 11, 2019

@beaufortfrancois, let's say permission was granted to use NFC... what's the expected behavior when this call happens:

nfc.push(thing);

(substitute the above for any of the methods of WebNFC that you are modifying in the PR... sorry, I don't know the API, so you need to go through each on a case by case basis)

Should the above succeed without first going through some kind of user activation such as a click or whatever? If yes, then the PR is correct.

However, you are counting on the permission API on consuming the activation... I don't know yet if we want that.... maybe we do, but I don't know.

@beaufortfrancois
Copy link
Collaborator Author

@marcoscaceres Once user has granted access to WebNFC, I think* we should not mandate user activation anymore. This user activation requirement is only to prevent websites to show a prompt when user doesn't expect it because of no interaction.
What do you think @zolkis @kenchris?

@kenchris
Copy link
Contributor

kenchris commented Dec 11, 2019

I agree

@zolkis
Copy link
Contributor

zolkis commented Dec 11, 2019

I wonder if there should be a time bound for not requiring user activation again (imagine once you click ok, maybe after 2 weeks you forget). I guess the given browser policy should apply.

@marcoscaceres
Copy link
Member

Wait, how does one ask for permission? The Permission API doesn’t have a .request() method last I checked?

(@zolkis, you will the happy to hear to “transient activation” is time based... have a quick look at the definition in the HTML spec).

@beaufortfrancois
Copy link
Collaborator Author

@marcoscaceres scan and push methods are tightly integrated with the Permissions API. See JS example below.

const reader = new NDEFReader();
 
async function startScanning() {
  await reader.scan();
  reader.onreading = event => {
    /* handle NDEF messages */
  };
}
 
const nfcPermissionStatus = await navigator.permissions.query({ name: "nfc" });
if (permissionStatus.state === "granted") {
  // NFC access was previously granted, so we can start NFC scanning now.
  startScanning();
} else {
  // Show a "scan" button.
  document.querySelector("button").style.display = "block";
  document.querySelector("button").onclick = event => {
    // Prompt user to allow UA to send and receive info when they tap NFC devices.
    startScanning();
  };
}

@marcoscaceres
Copy link
Member

Ok, I see now where you want to go with this... thanks for the example! That was really helpful.

We will need to discuss further about consumption of the activation over in the permissions spec bug. Or if it will be sticky... I’m not sure yet.

@marcoscaceres
Copy link
Member

Btw, I don’t think “NotReadableError” is a thing, right?

https://heycam.github.io/webidl/#dfn-error-names-table

You probably want “NotAllowedError” or “SecurityError”.

Something still doesn’t feel right here. The spec doesn’t hook into the “request permission” algorithm of the permission API. I was at a minimum expecting that.

@beaufortfrancois
Copy link
Collaborator Author

beaufortfrancois commented Dec 11, 2019

@marcoscaceres I guess you're referring to this part:

If the UA is not allowed to access the underlying NFC Adapter (e.g. a user preference), then reject p with a "NotReadableError" DOMException and return p.

We used NotReadableError because we already had NotAllowedError and SecurityError. The goal was to help web developers to inform their users that NFC setting was disabled, and that they should turn on NFC manually in platform settings (android for info).
If you can think of another one, that is not used, we're all ears.

Something still doesn’t feel right here. The spec doesn’t hook into the “request permission” algorithm of the permission API. I was at a minimum expecting that.

We are. See https://w3c.github.io/web-nfc/#obtaining-permission

image

@marcoscaceres
Copy link
Member

Minting new DOM Exception names is not allowed, unfortunately. To mint a new one we need to request it from the WebIDL folks.

Thanks for pointing me to the permissions text. I was reading the .scan() method definition in the diff and missed that. I’ll take a proper look tomorrow.

@beaufortfrancois
Copy link
Collaborator Author

I'm confused. https://heycam.github.io/webidl/#notreadableerror exists right. What you're saying is that it's not meant for this?

@mustaqahmed
Copy link
Member

@beaufortfrancois: Yes, the PR looks good to me from user activation perspective. (And thanks for pinging the issue in Permission API.)

@marcoscaceres
Copy link
Member

My bad about NotReadableError... serves me right for checking on my phone.

@beaufortfrancois
Copy link
Collaborator Author

@mustaqahmed Thanks!
@marcoscaceres Let me know if this looks good to you as well. We may refer to this issue in the "obtain permission" section regarding the user activation bit missing in the Permissions Spec.

@marcoscaceres
Copy link
Member

Yes, lgtm! Sorry again about yesterday 🥺

@beaufortfrancois beaufortfrancois merged commit 6fe7312 into w3c:gh-pages Dec 12, 2019
@beaufortfrancois beaufortfrancois deleted the remove-user-activation branch December 12, 2019 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Origin Trial Issues that would be good to solve before OT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants