Bug 221399 – Device motion / orientation events not working in third-party iframes despite Feature-Policy allowing it

• Component: Frames
• Status: RESOLVED FIXED

[Will Morgan]

Created attachment 419269 [details]
Safari gyroscope iframe permission problem

Using iOS 14, DeviceMotionEvent.requestPermission does not exist in documents that are hosted within a cross-origin iframe, even if feature policy is set.

This seems to be because the gyroscope capability of Safari isn't yet supported, as documented on MDN: https://developer.mozilla.org/en-US/docs/Web/HTTP/Feature_Policy

To replicate, please:

1. See the attached reproduction or https://storage.cloud.google.com/public-demo/safarigyro.html
2. Navigate to the page using Safari, preferably on a device with a gyroscope, like an iPhone or iPad.
3. Observe that DeviceMotionEvent.requestPermission works correctly on the hosting page.
4. Observe that even though the gyroscope property is set per the Feature-Policy (Permissions-Policy?), the clicking the iframe'd button triggers an error.

This currently affects anyone wanting to integrate apps that use device motion in an iframe, which is a proven, reasonable and ubiquitous integration method for web apps wanting to compose their own user experiences with tools from other vendors.

There is no satisfactory workaround for the above issue. Any consenting web application cannot prompt their users to request gyroscope permissions, therefore device motion is broken in iframes.

[Radar WebKit Bug Importer]

<rdar://problem/74229227>

[Will Morgan]

Further to this, detecting this behaviour is extremely difficult if not impossible, which means it is impossible to provide a progressive enhancement workaround.

Using Safari 14, inside an iframe, DeviceMotionEvent and DeviceMotionEvent.requestPermission both exist and the latter is callable.

This means that the only way developers can work around this is to block ALL PREVIOUS, CURRENT AND FUTURE mobile WebKit browsers inside iframes. There is no way we can supply progressive enhancement scaffolding that provides a workaround that will disappear when or indeed if this bug is fixed.

[Chris Dumez]

WebKit does not allow the use of the device motion API at all in third-party iframes so exposing the permission API there did not make sense.

[Will Morgan]

(In reply to Chris Dumez from comment #3)
> WebKit does not allow the use of the device motion API at all in third-party
> iframes so exposing the permission API there did not make sense.

If that is the position that has been taken, why does DeviceMotionEvent.requestPermission exist but simply do nothing?

Would you agree that this undefined behaviour would be better replaced by throwing an error, or simply not exposing the DeviceMotionEvent or requestPermission APIs?

What about consenting app developers who have made the informed choice and want to make use of the Feature Policy functionality to whitelist device motion events, in the same way that cameras can be allowed?

To allow cameras but not device motion, to me, is an unusual weighting of priorities.

[Chris Dumez]

(In reply to Will Morgan from comment #4)
> (In reply to Chris Dumez from comment #3)
> > WebKit does not allow the use of the device motion API at all in third-party
> > iframes so exposing the permission API there did not make sense.
> 
> If that is the position that has been taken, why does
> DeviceMotionEvent.requestPermission exist but simply do nothing?

Do nothing? As far as I can tell, we are resolving the promise with state="denied".

[Will Morgan]

(In reply to Chris Dumez from comment #5)
> As far as I can tell, we are resolving the promise with state="denied".

You are correct - apologies. However IMHO, my point still stands after a slight reformulation:

requestPermission exists and is callable, but it always returns a predetermined denied result. There's been no user interaction; it is impossible to discern between an informed user action and a blanket block.

Site owners can opt-in to camera access on a per-iframe basis, but they cannot do the same for gyroscope access. iOS prompts for device motion and orientation in the same way as camera access is requested, with a modal confirm dialog.

It would be great to be able to tell the user "sorry, your browser isn't supported" versus "we need device motion data, could you please allow it?"

But it would be even better if WebKit could respect the gyroscope and accelerometer feature policies, just like it allows camera and fullscreen access.

[Mark B]

I would like to second the concerns expressed above.

Providing support for camera, microphone, fullscreen, etc in third-party iframes, but none for device motion detection, seems rather arbitrary and has severe implications, not just for us but almost certainly for many other web apps out there.

This functionality is available in all other major browsers.

Is there any room for optimism that this will be addressed in the near future? If not, can we please get a more comprehensive statement as to why?

[Will Morgan]

Another inconsistency in policy application (IMHO) is that very recently, WebKit allows third party iframes to request local storage access: https://webkit.org/blog/11545/updates-to-the-storage-access-api/

But still not device motion.

[Chris Dumez]

I agree it seems like a good idea to leverage Feature-Policy make device motion/orientation events functional in third-party iframes.

I see there are 2 feature policies that could be at play here: accelerometer & gyroscope. We also have 2 types of events: DeviceMotionEvent & DeviceOrientationEvent.

I need to look into this because I am not quite should what the mapping should be yet. Let me know if you have an opinion.

[Chris Dumez]

Created attachment 421322 [details]
WIP Patch

[Will Morgan]

Hi Chris, appreciate that massively. Look forward to testing as and when it hits a technology preview release. Let me know if we can help in any way.

[Chris Dumez]

(In reply to Will Morgan from comment #11)
> Hi Chris, appreciate that massively. Look forward to testing as and when it
> hits a technology preview release. Let me know if we can help in any way.

Sadly, we don't have Safari Technology Preview on iOS, only macOS. Device Motion/Orientation is an iOS-only feature.

You can help my letting me know which feature-policy (gyroscope, accelerometer) you think should be required for which events (Motion, Orientation).

[Chris Dumez]

(In reply to Chris Dumez from comment #12)
> (In reply to Will Morgan from comment #11)
> > Hi Chris, appreciate that massively. Look forward to testing as and when it
> > hits a technology preview release. Let me know if we can help in any way.
> 
> Sadly, we don't have Safari Technology Preview on iOS, only macOS. Device
> Motion/Orientation is an iOS-only feature.
> 
> You can help my letting me know which feature-policy (gyroscope,
> accelerometer) you think should be required for which events (Motion,
> Orientation).

Looking at the blink code it seems:
1. DeviceMotionEvent: Requires gyroscope & accelerometer
2. DeviceOrientationEvent: Requires gyroscope & accelerometer & magnetometer

Sounds about right?

[Will Morgan]

Ok, noted!

In terms of which feature policies enable which features, here's my understanding:

DeviceMotionEvent exposes data that you need both accelerometer and gyroscope sensors to access.

DeviceOrientationEvent exposes compass related data that you need the gyroscope for, and in the case of the orientationabsolute event, the magnetometer is needed: https://w3c.github.io/deviceorientation/#deviceorientationabsolute

Looking at your analysis I think we're agreed.

[Chris Dumez]

Created attachment 421339 [details]
WIP Patch (Needs tests)

[Chris Dumez]

Created attachment 421354 [details]
WIP Patch

[Chris Dumez]

Created attachment 421362 [details]
WIP Patch

[Chris Dumez]

Created attachment 421371 [details]
Patch

[Chris Dumez]

Created attachment 421376 [details]
Patch

[youenn fablet]

Comment on attachment 421376 [details]
Patch

LGTM overall, but I'd like to understand why we need m_accessStatePerOrigin.
 
View in context: https://bugs.webkit.org/attachment.cgi?id=421376&action=review

> Source/WebCore/dom/DeviceMotionEvent.cpp:144
> +    page->deviceOrientationAndMotionAccessController().shouldAllowAccess(document, [promise = WTFMove(promise)](PermissionState permissionState) mutable {

could use auto.

> Source/WebCore/dom/DeviceOrientationAndMotionAccessController.cpp:49
> +    auto it = m_accessStatePerOrigin.find(document.securityOrigin().data());

iterator?

> Source/WebCore/dom/DeviceOrientationAndMotionAccessController.cpp:72
> +    page->chrome().client().shouldAllowDeviceOrientationAndMotionAccess(*document.frame(), mayPrompt, [this, weakThis = makeWeakPtr(this), securityOrigin = makeRef(document.securityOrigin()), callback = WTFMove(callback)](DeviceOrientationOrMotionPermissionState permissionState) mutable {

could use auto.

> Source/WebCore/dom/DeviceOrientationAndMotionAccessController.cpp:76
> +        m_accessStatePerOrigin.set(securityOrigin->data(), permissionState);

Why do we need this map?
Also, it seems the map could span multiple top documents? What happens if user changes per-site settings and tries to apply them by reloading a page.
Can we simplify the patch by removing m_accessStatePerOrigin?

> Source/WebCore/page/DOMWindow.cpp:2030
> +        || !isFeaturePolicyAllowedByDocumentAndAllOwners(FeaturePolicy::Type::Accelerometer, *document(), LogFeaturePolicyFailure::No)) {

Is there a case where gyroscope can be used, but not accelerometer? Maybe we should send feedback to the spec authors if that is not the case.

> LayoutTests/ChangeLog:9
> +        Add layout test coverage.

Can we import web-platform-tests/orientation-sensor?
It seems some of these tests might cover the same area.

[Chris Dumez]

(In reply to youenn fablet from comment #20)
> Comment on attachment 421376 [details]
> Patch
> 
> LGTM overall, but I'd like to understand why we need m_accessStatePerOrigin.
>  
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421376&action=review
> 
> > Source/WebCore/dom/DeviceMotionEvent.cpp:144
> > +    page->deviceOrientationAndMotionAccessController().shouldAllowAccess(document, [promise = WTFMove(promise)](PermissionState permissionState) mutable {
> 
> could use auto.
> 
> > Source/WebCore/dom/DeviceOrientationAndMotionAccessController.cpp:49
> > +    auto it = m_accessStatePerOrigin.find(document.securityOrigin().data());
> 
> iterator?
> 
> > Source/WebCore/dom/DeviceOrientationAndMotionAccessController.cpp:72
> > +    page->chrome().client().shouldAllowDeviceOrientationAndMotionAccess(*document.frame(), mayPrompt, [this, weakThis = makeWeakPtr(this), securityOrigin = makeRef(document.securityOrigin()), callback = WTFMove(callback)](DeviceOrientationOrMotionPermissionState permissionState) mutable {
> 
> could use auto.
> 
> > Source/WebCore/dom/DeviceOrientationAndMotionAccessController.cpp:76
> > +        m_accessStatePerOrigin.set(securityOrigin->data(), permissionState);
> 
> Why do we need this map?
> Also, it seems the map could span multiple top documents? What happens if
> user changes per-site settings and tries to apply them by reloading a page.
> Can we simplify the patch by removing m_accessStatePerOrigin?

Since it is stored on the Page, then yes, it can span multiple top documents in case of navigation. The reason for the caching in the map is that we want to avoid IPC to the UIProcess every time an orientation/motion event listener gets added. I think you have a point with the per-site setting though. I think that ideally we want some kind of caching to avoid frequent IPC to the UIProcess (like we had before this patch) but also, we don't want this caching to persist on navigation (life before this patch). I will look into this more.
 
> > Source/WebCore/page/DOMWindow.cpp:2030
> > +        || !isFeaturePolicyAllowedByDocumentAndAllOwners(FeaturePolicy::Type::Accelerometer, *document(), LogFeaturePolicyFailure::No)) {
> 
> Is there a case where gyroscope can be used, but not accelerometer? Maybe we
> should send feedback to the spec authors if that is not the case.
> 
> > LayoutTests/ChangeLog:9
> > +        Add layout test coverage.
> 
> Can we import web-platform-tests/orientation-sensor?
> It seems some of these tests might cover the same area.

These tests seem to be for https://w3c.github.io/orientation-sensor/ which we do not support.

[youenn fablet]

Since we have feature policy integration, I would cache the value retrieved through IPC somewhere at the main document level.

[Chris Dumez]

(In reply to youenn fablet from comment #22)
> Since we have feature policy integration, I would cache the value retrieved
> through IPC somewhere at the main document level.

Oh, because different iframes from the same origin could have different feature policy and thus have different access to orientation events?

This works fine with my current approach because DOMWindow checks both whether the frame has API access and if the frame's origin has received the permission:
```
    if (!isAllowedToUseDeviceOrientation(innerMessage) || !hasPermissionToReceiveDeviceMotionOrOrientationEvents(innerMessage)) {
        if (auto* document = this->document())
            document->addConsoleMessage(MessageSource::JS, MessageLevel::Warning, makeString("No device orientation events will be fired, reason: ", innerMessage, "."));
        return;
    }
```

Consider the case where you have 2 third-party iframes from the same origin and both need access to orientation events. I do not want to prompt twice in such cases.

[Chris Dumez]

Created attachment 421412 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #23)
> (In reply to youenn fablet from comment #22)
> > Since we have feature policy integration, I would cache the value retrieved
> > through IPC somewhere at the main document level.
> 
> Oh, because different iframes from the same origin could have different
> feature policy and thus have different access to orientation events?
> 
> This works fine with my current approach because DOMWindow checks both
> whether the frame has API access and if the frame's origin has received the
> permission:
> ```
>     if (!isAllowedToUseDeviceOrientation(innerMessage) ||
> !hasPermissionToReceiveDeviceMotionOrOrientationEvents(innerMessage)) {
>         if (auto* document = this->document())
>             document->addConsoleMessage(MessageSource::JS,
> MessageLevel::Warning, makeString("No device orientation events will be
> fired, reason: ", innerMessage, "."));
>         return;
>     }
> ```
> 
> Consider the case where you have 2 third-party iframes from the same origin
> and both need access to orientation events. I do not want to prompt twice in
> such cases.

(and both third-party iframes are allowed to use the API via Feature-Policy, obviously)

[Chris Dumez]

I made the following changes:
1. Made suggested nit fixes
2. Moved the DeviceOrientationAndMotionAccessController from the Page to the top Document so that permissions no longer survive on navigation.
3. Fixed per-site setting checking in DeviceOrientationAndMotionAccessController to check the topFrame's documentLoader instead of the current frame's when in a first-party iframe.

[Chris Dumez]

(In reply to Chris Dumez from comment #26)
> I made the following changes:
> 1. Made suggested nit fixes
> 2. Moved the DeviceOrientationAndMotionAccessController from the Page to the
> top Document so that permissions no longer survive on navigation.
> 3. Fixed per-site setting checking in
> DeviceOrientationAndMotionAccessController to check the topFrame's
> documentLoader instead of the current frame's when in a first-party iframe.

And again, I kept the map of SecurityOrigin -> permission state because I do not want several third-party iframes (from the same origin and all allowed via Feature-Policy) to have to request permission independently, each time with a user-gesture. The permission is per origin so the user giving permission for one of these iframes should be enough.

[youenn fablet]

(In reply to Chris Dumez from comment #27)
> (In reply to Chris Dumez from comment #26)
> > I made the following changes:
> > 1. Made suggested nit fixes
> > 2. Moved the DeviceOrientationAndMotionAccessController from the Page to the
> > top Document so that permissions no longer survive on navigation.
> > 3. Fixed per-site setting checking in
> > DeviceOrientationAndMotionAccessController to check the topFrame's
> > documentLoader instead of the current frame's when in a first-party iframe.
> 
> And again, I kept the map of SecurityOrigin -> permission state because I do
> not want several third-party iframes (from the same origin and all allowed
> via Feature-Policy) to have to request permission independently, each time
> with a user-gesture. The permission is per origin so the user giving
> permission for one of these iframes should be enough.

From a user perspective, a prompt will likely be annoying no matter whether the two iframes are same origin or not.
User is not aware of the difference between the two.
The decision to prompt or not can either be done UIProcess or WebProcess.

Given feature policy trend, we could remove the possibility for a WebKit application to grant certain iframe origins and not others.
User either grants or denies access to all documents of the page (not persisting on top level document navigation though).

That would fix the case of a user having to deny access to topOrigin/iframeOrigin1, topOrigin/iframeOrigin2, topOrigin/iframeOrigin3.
That said, given there is a user activation requirement for this API and since we check user activation is on the document that calls the device orientation API, we could consider persisting granting but not denying.

That removes the need for the map.

[Chris Dumez]

(In reply to youenn fablet from comment #28)
> (In reply to Chris Dumez from comment #27)
> > (In reply to Chris Dumez from comment #26)
> > > I made the following changes:
> > > 1. Made suggested nit fixes
> > > 2. Moved the DeviceOrientationAndMotionAccessController from the Page to the
> > > top Document so that permissions no longer survive on navigation.
> > > 3. Fixed per-site setting checking in
> > > DeviceOrientationAndMotionAccessController to check the topFrame's
> > > documentLoader instead of the current frame's when in a first-party iframe.
> > 
> > And again, I kept the map of SecurityOrigin -> permission state because I do
> > not want several third-party iframes (from the same origin and all allowed
> > via Feature-Policy) to have to request permission independently, each time
> > with a user-gesture. The permission is per origin so the user giving
> > permission for one of these iframes should be enough.
> 
> From a user perspective, a prompt will likely be annoying no matter whether
> the two iframes are same origin or not.
> User is not aware of the difference between the two.
> The decision to prompt or not can either be done UIProcess or WebProcess.
> 
> Given feature policy trend, we could remove the possibility for a WebKit
> application to grant certain iframe origins and not others.
> User either grants or denies access to all documents of the page (not
> persisting on top level document navigation though).
> 
> That would fix the case of a user having to deny access to
> topOrigin/iframeOrigin1, topOrigin/iframeOrigin2, topOrigin/iframeOrigin3.
> That said, given there is a user activation requirement for this API and
> since we check user activation is on the document that calls the device
> orientation API, we could consider persisting granting but not denying.
> 
> That removes the need for the map.

I am not sure what you're arguing for. We standardized this permission API and it is per origin, not per top level-document. Any standards change is out of scope of this patch.

[Chris Dumez]

(In reply to Chris Dumez from comment #29)
> (In reply to youenn fablet from comment #28)
> > (In reply to Chris Dumez from comment #27)
> > > (In reply to Chris Dumez from comment #26)
> > > > I made the following changes:
> > > > 1. Made suggested nit fixes
> > > > 2. Moved the DeviceOrientationAndMotionAccessController from the Page to the
> > > > top Document so that permissions no longer survive on navigation.
> > > > 3. Fixed per-site setting checking in
> > > > DeviceOrientationAndMotionAccessController to check the topFrame's
> > > > documentLoader instead of the current frame's when in a first-party iframe.
> > > 
> > > And again, I kept the map of SecurityOrigin -> permission state because I do
> > > not want several third-party iframes (from the same origin and all allowed
> > > via Feature-Policy) to have to request permission independently, each time
> > > with a user-gesture. The permission is per origin so the user giving
> > > permission for one of these iframes should be enough.
> > 
> > From a user perspective, a prompt will likely be annoying no matter whether
> > the two iframes are same origin or not.
> > User is not aware of the difference between the two.
> > The decision to prompt or not can either be done UIProcess or WebProcess.
> > 
> > Given feature policy trend, we could remove the possibility for a WebKit
> > application to grant certain iframe origins and not others.
> > User either grants or denies access to all documents of the page (not
> > persisting on top level document navigation though).
> > 
> > That would fix the case of a user having to deny access to
> > topOrigin/iframeOrigin1, topOrigin/iframeOrigin2, topOrigin/iframeOrigin3.
> > That said, given there is a user activation requirement for this API and
> > since we check user activation is on the document that calls the device
> > orientation API, we could consider persisting granting but not denying.
> > 
> > That removes the need for the map.
> 
> I am not sure what you're arguing for. We standardized this permission API
> and it is per origin, not per top level-document. Any standards change is
> out of scope of this patch.

And I am opposed to requiring the user to interact with iframes individually in order to give them permission, if those iframes are from the same origin (and they all have the appropriate feature policy.

[Chris Dumez]

(In reply to youenn fablet from comment #28)
> (In reply to Chris Dumez from comment #27)
> > (In reply to Chris Dumez from comment #26)
> > > I made the following changes:
> > > 1. Made suggested nit fixes
> > > 2. Moved the DeviceOrientationAndMotionAccessController from the Page to the
> > > top Document so that permissions no longer survive on navigation.
> > > 3. Fixed per-site setting checking in
> > > DeviceOrientationAndMotionAccessController to check the topFrame's
> > > documentLoader instead of the current frame's when in a first-party iframe.
> > 
> > And again, I kept the map of SecurityOrigin -> permission state because I do
> > not want several third-party iframes (from the same origin and all allowed
> > via Feature-Policy) to have to request permission independently, each time
> > with a user-gesture. The permission is per origin so the user giving
> > permission for one of these iframes should be enough.
> 
> From a user perspective, a prompt will likely be annoying no matter whether
> the two iframes are same origin or not.
> User is not aware of the difference between the two.
> The decision to prompt or not can either be done UIProcess or WebProcess.

The UIProcess would already make sure we do not prompt twice for the same origin. However, the JS would still need to request the permission again in each iframe from same origin (or even time the third-party gets navigated) and requesting permission requires a user gesture. The issue is not with double prompting but rather with forcing the user to interact with third-party iframes individually for no good reason, or forcing them to interact with a third-party iframe again simply because it got navigated.

[youenn fablet]

> > I am not sure what you're arguing for. We standardized this permission API
> > and it is per origin, not per top level-document. Any standards change is
> > out of scope of this patch.

The permission is really per top level-document origin.

> And I am opposed to requiring the user to interact with iframes individually
> in order to give them permission, if those iframes are from the same origin
> (and they all have the appropriate feature policy.

There seems to be a misunderstanding, I am not pushing for more prompts but less prompts.

Let's say we have a top level document DOC (origin A) with iframe I1 (origin B), iframe I2 (origin B) and iframe I3 (origin C).

User interacts with I1 and allows device orientation after being prompted with origin A in the prompt. From user point of view, decision is made to grant DOC, I1, I2 and I3 are unknown implementation details.

At that point, what you are saying is that you do not want I2 to trigger a prompt. That is fair.
What I am saying is that we might not want to prompt if DOC wants access too.
Ditto for I3 (that might be the most controversial one).

[Chris Dumez]

(In reply to youenn fablet from comment #32)
> > > I am not sure what you're arguing for. We standardized this permission API
> > > and it is per origin, not per top level-document. Any standards change is
> > > out of scope of this patch.
> 
> The permission is really per top level-document origin.

It is not. I standardized it and it is per origin:
https://w3c.github.io/deviceorientation/#id=permission-model

[Chris Dumez]

(In reply to Chris Dumez from comment #33)
> (In reply to youenn fablet from comment #32)
> > > > I am not sure what you're arguing for. We standardized this permission API
> > > > and it is per origin, not per top level-document. Any standards change is
> > > > out of scope of this patch.
> > 
> > The permission is really per top level-document origin.
> 
> It is not. I standardized it and it is per origin:
> https://w3c.github.io/deviceorientation/#id=permission-model

And I want it to stay per origin and the origin is displayed on the prompt. Giving access to such sensitive data to the top level origin does not mean I would be OK as a user sharing this information with ads on the page. Similarly, giving the permission to a trusted iframe does not mean I would trust other third-party iframes from different origins with that information.

[youenn fablet]

(In reply to Chris Dumez from comment #33)
> (In reply to youenn fablet from comment #32)
> > > > I am not sure what you're arguing for. We standardized this permission API
> > > > and it is per origin, not per top level-document. Any standards change is
> > > > out of scope of this patch.
> > 
> > The permission is really per top level-document origin.
> 
> It is not. I standardized it and it is per origin:
> https://w3c.github.io/deviceorientation/#id=permission-model

Oh I see. So in the prompt, this is the origin of the iframe and not of the top level-document that is shown?
This is a very different model from getUserMedia, geolocation (as well?).
What is the rationale to differ there?

[youenn fablet]

> > It is not. I standardized it and it is per origin:
> > https://w3c.github.io/deviceorientation/#id=permission-model

It is not clear to me what the origin in this link refers to.

[Chris Dumez]

(In reply to youenn fablet from comment #35)
> (In reply to Chris Dumez from comment #33)
> > (In reply to youenn fablet from comment #32)
> > > > > I am not sure what you're arguing for. We standardized this permission API
> > > > > and it is per origin, not per top level-document. Any standards change is
> > > > > out of scope of this patch.
> > > 
> > > The permission is really per top level-document origin.
> > 
> > It is not. I standardized it and it is per origin:
> > https://w3c.github.io/deviceorientation/#id=permission-model
> 
> Oh I see. So in the prompt, this is the origin of the iframe and not of the
> top level-document that is shown?

That is correct.

> This is a very different model from getUserMedia, geolocation (as well?).
> What is the rationale to differ there?

I am not familiar with getUserMedia (and geolocation is old). The reason we added this permission API is that those events were used by ads for tracking purposes. As a user, I can be a top-level site that legitimately needs my device orientation and as a user I can choose to share this private information with first-party content. However, this should NOT give my private information to ads on that page.

[Chris Dumez]

(In reply to youenn fablet from comment #36)
> > > It is not. I standardized it and it is per origin:
> > > https://w3c.github.io/deviceorientation/#id=permission-model
> 
> It is not clear to me what the origin in this link refers to.

Maybe I am no good at writing specs :) But here is where I think it is clear
```
The static requestPermission() operation, when invoked, must run these steps:

Let promise be a new promise.

Run these steps in parallel:

Let permission be permission for relevant settings object’s origin.

If permission is "default" and the method call was not triggered by user activation, then reject promise with a NotAllowedError DOMException and abort these steps.

If permission is "default", ask the user whether sharing device orientation for the relevant settings object’s origin is acceptable. If it is, set permission to "granted", and "denied" otherwise.

Queue a task to run these steps:

Set permission for the relevant settings object’s origin to permission.

Fulfill promise with permission.

Return promise.
```

-> Let permission be permission for relevant settings object’s origin.

[youenn fablet]

> -> Let permission be permission for relevant settings object’s origin.

Ah ok, this one is clear!
I'll look again at the patch, I do not think this is the right place to discuss whether it is good for device orientation prompt to defer from other prompts.

[youenn fablet]

Comment on attachment 421412 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421412&action=review

> Source/WebCore/dom/DeviceOrientationAndMotionAccessController.cpp:58
> +    }

It is a bit odd to have this dual code path here.
Maybe we should always go to UIProcess if we do not have the value in the map.

> Source/WebCore/dom/DeviceOrientationAndMotionAccessController.cpp:79
> +        m_accessStatePerOrigin.set(securityOrigin->data(), permissionState);

s/set/add

> LayoutTests/ChangeLog:9
> +        Add layout test coverage.

It is probably worth adding tests for the two iframe behavior you mentionned above.
This could get easily overlooked in refactorings.

[Chris Dumez]

Comment on attachment 421412 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421412&action=review

>> Source/WebCore/dom/DeviceOrientationAndMotionAccessController.cpp:58
>> +    }
> 
> It is a bit odd to have this dual code path here.
> Maybe we should always go to UIProcess if we do not have the value in the map.

Hmm. The UIProcess currently doesn't know anything about the per-site setting AFAIK. It is passed by the client app in one of the navigation policy delegates and stored on the DocumentLoader in WebCore. Like before my change, the default access state is defined by the per-site setting. Also note that accessState() is synchronous and called when adding event listeners, we don't want to be doing IPC to the UIProcess in this case.

>> Source/WebCore/dom/DeviceOrientationAndMotionAccessController.cpp:79
>> +        m_accessStatePerOrigin.set(securityOrigin->data(), permissionState);
> 
> s/set/add

Hmm, I usually use add() but in this case I figured whatever the latest decision should overwrite whatever previous decision we have in the map so I figured set() was more appropriate. I don't think it is a huge deal in practice but I felt set() was conceptually more accurate since we always want the latest up-to-date permission state in the map.

>> LayoutTests/ChangeLog:9
>> +        Add layout test coverage.
> 
> It is probably worth adding tests for the two iframe behavior you mentionned above.
> This could get easily overlooked in refactorings.

Ok. I will look into this.

[youenn fablet]

> >> Source/WebCore/dom/DeviceOrientationAndMotionAccessController.cpp:79
> >> +        m_accessStatePerOrigin.set(securityOrigin->data(), permissionState);
> > 
> > s/set/add
> 
> Hmm, I usually use add() but in this case I figured whatever the latest
> decision should overwrite whatever previous decision we have in the map so I
> figured set() was more appropriate. I don't think it is a huge deal in
> practice but I felt set() was conceptually more accurate since we always
> want the latest up-to-date permission state in the map.

Oh, I thought we were checking the map first before going through IPC.
So that we would not be able to overwrite a map entry.

[Chris Dumez]

(In reply to youenn fablet from comment #42)
> > >> Source/WebCore/dom/DeviceOrientationAndMotionAccessController.cpp:79
> > >> +        m_accessStatePerOrigin.set(securityOrigin->data(), permissionState);
> > > 
> > > s/set/add
> > 
> > Hmm, I usually use add() but in this case I figured whatever the latest
> > decision should overwrite whatever previous decision we have in the map so I
> > figured set() was more appropriate. I don't think it is a huge deal in
> > practice but I felt set() was conceptually more accurate since we always
> > want the latest up-to-date permission state in the map.
> 
> Oh, I thought we were checking the map first before going through IPC.
> So that we would not be able to overwrite a map entry.

We are checking the map before calling page->chrome().client().shouldAllowDeviceOrientationAndMotionAccess(). But page->chrome().client().shouldAllowDeviceOrientationAndMotionAccess() is async so in theory, the domain could be in the map by the time the lambda runs.

[Chris Dumez]

(In reply to Chris Dumez from comment #43)
> (In reply to youenn fablet from comment #42)
> > > >> Source/WebCore/dom/DeviceOrientationAndMotionAccessController.cpp:79
> > > >> +        m_accessStatePerOrigin.set(securityOrigin->data(), permissionState);
> > > > 
> > > > s/set/add
> > > 
> > > Hmm, I usually use add() but in this case I figured whatever the latest
> > > decision should overwrite whatever previous decision we have in the map so I
> > > figured set() was more appropriate. I don't think it is a huge deal in
> > > practice but I felt set() was conceptually more accurate since we always
> > > want the latest up-to-date permission state in the map.
> > 
> > Oh, I thought we were checking the map first before going through IPC.
> > So that we would not be able to overwrite a map entry.
> 
> We are checking the map before calling
> page->chrome().client().shouldAllowDeviceOrientationAndMotionAccess(). But
> page->chrome().client().shouldAllowDeviceOrientationAndMotionAccess() is
> async so in theory, the domain could be in the map by the time the lambda
> runs.

And I don't think it is entirely theoretical. I believe this can happen if the JS makes multiple permission requests in parallel for the same origin.

[youenn fablet]

Could this trigger double prompting?

[Chris Dumez]

(In reply to youenn fablet from comment #45)
> Could this trigger double prompting?

No, I don't believe so. It looks like WebDeviceOrientationAndMotionAccessController::shouldAllowAccess() in the UIProcess properly queues requests and processes only one at a time.

[Chris Dumez]

(In reply to Will Morgan from comment #14)
> Ok, noted!
> 
> In terms of which feature policies enable which features, here's my
> understanding:
> 
> DeviceMotionEvent exposes data that you need both accelerometer and
> gyroscope sensors to access.
> 
> DeviceOrientationEvent exposes compass related data that you need the
> gyroscope for, and in the case of the orientationabsolute event, the
> magnetometer is needed:
> https://w3c.github.io/deviceorientation/#deviceorientationabsolute
> 
> Looking at your analysis I think we're agreed.

@Will Morgan: Just wanted to point out that the demo page you provided only half works with my change because you are missing the "magnetometer" Feature-Policy on your iframe. You are therefore getting the DeviceMotion events but not the DeviceOrientation ones. I have verified manually that adding the  "magnetometer" Feature-Policy on the iframe makes the demo fully work.

[Chris Dumez]

Created attachment 421457 [details]
Patch

[EWS]

Committed r273444: <https://commits.webkit.org/r273444>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421457 [details].

[Will Morgan]

(In reply to Chris Dumez from comment #47)
> @Will Morgan: Just wanted to point out that the demo page you provided only
> half works with my change because you are missing the "magnetometer"
> Feature-Policy on your iframe. You are therefore getting the DeviceMotion
> events but not the DeviceOrientation ones. I have verified manually that
> adding the  "magnetometer" Feature-Policy on the iframe makes the demo fully
> work.

Thanks for the heads up, and more generally the patching. I've updated the example to add magnetometer permissions in there too for future travellers.

Do you have an estimate on which iOS version this patch might land in?

[Chris Dumez]

(In reply to Will Morgan from comment #50)
> (In reply to Chris Dumez from comment #47)
> > @Will Morgan: Just wanted to point out that the demo page you provided only
> > half works with my change because you are missing the "magnetometer"
> > Feature-Policy on your iframe. You are therefore getting the DeviceMotion
> > events but not the DeviceOrientation ones. I have verified manually that
> > adding the  "magnetometer" Feature-Policy on the iframe makes the demo fully
> > work.
> 
> Thanks for the heads up, and more generally the patching. I've updated the
> example to add magnetometer permissions in there too for future travellers.
> 
> Do you have an estimate on which iOS version this patch might land in?

Sadly I cannot comment on when a particular fix will ship to customers, per Apple policy. All I can say is that WebKit trunk usually gets picked up in iOS/macOS builds twice a year (~Feb & ~Sept).

[Sam Sneddon [:gsnedders]]

*** Bug 152299 has been marked as a duplicate of this bug. ***