Closed Bug 1580567 Opened 5 years ago Closed 4 years ago

Implement XR device access permission UI

Categories

(Core :: WebVR, task)

Product:
Core
Component:
WebVR
Type:
task
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla73
Tracking Flags:
Tracking Status
firefox73 --- fixed

People

(Reporter: kip, Assigned: kip)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

Bug 1580567 - Implement XR device access permission UI
47 bytes, text/x-phabricator-request
Details | Review
No description provided.

Patch is WIP. Notably, need to avoid creating the VR presentation when the promise is not resolved.

Depends on: 1582602

Fixed WPT, changed WebVR interfaces to [SecureContext]

Updated try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6e2ee1a8ffc716e991e2782fda9df9bd47f74f9

Fixed dom/tests/mochitest/general/test_interfaces.html, to reflect that the WebVR interfaces are now [SecureContext]. (This will require DOM peer review).

Updated try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=375bfa4ea8d24150b0f8905c0a8f1fb94dd2caef

Prevented redundant popups and spamming of permission prompt.

Updated try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f68e0363bd1de53419c7c3351cf20d7a3ece2435

Added screenshots from this patch for reference to Bug 1579281.

Depends on: 1579276

Fixes for review feedback and merged in status icons from Bug 1579276.

Updated try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bc8b42959042d4e76db102ea72b81027ba15f6c

Fixed issue with removing Virtual Reality permissions from an iframe that had previously been set to remember.

Updated try push (T-shaped), for sanity check:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4fd46abcd9f1893eae042f16fa0b2a2bca5b293

IIUC, this fix should address the last of the review feedback.

Blocks: 1599927

With Daosheng's fix to follow in Bug 1599927, do you have any additional review feedback on the patch?

Flags: needinfo?(imanol)

I tried both patches together and it works well, r+

Flags: needinfo?(imanol)
Pushed by kgilbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/15f8fa34d2f4
Implement XR device access permission UI r=fluent-reviewers,bzbarsky,pbz,daoshengmu,imanol

I'll update the patch with fixes for the test failures and attempt to re-land.

The test-interfaces.html test possibly failed due to the r+ from a dom peer (bz) not being re-applied after the patch was rebased...

Flags: needinfo?(kgilbert)

Tests were failing due to changes related to permission requests from iframes since the patch was authored.

I have un-bitrotted the patch and tested locally.

Updated try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=71fa78845f15cb4f863acb57489b5a7af1e9a733

I notice from the last try run, the patch is also causing:

TEST-UNEXPECTED-ERROR | /feature-policy/reporting/vr-reporting.https.html | unable to find test window

I'll investigate and update the patch.

(In reply to :kip (Kearwood Gilbert) from comment #19)

I notice from the last try run, the patch is also causing:

TEST-UNEXPECTED-ERROR | /feature-policy/reporting/vr-reporting.https.html | unable to find test window

I'll investigate and update the patch.

Note that this is on the android platform only. We don't expect VR to be enabled on android, except in the geckoview-based Firefox Reality browser.

(In reply to :kip (Kearwood Gilbert) from comment #20)

(In reply to :kip (Kearwood Gilbert) from comment #19)

I notice from the last try run, the patch is also causing:

TEST-UNEXPECTED-ERROR | /feature-policy/reporting/vr-reporting.https.html | unable to find test window

I'll investigate and update the patch.

Note that this is on the android platform only. We don't expect VR to be enabled on android, except in the geckoview-based Firefox Reality browser.

I've repro'ed locally and confirm that the follow-up, bug 1599927, corrects this WPT failure.

If I can get the test-interfaces.html re-reviewed, and otherwise get a r+ from @bz, I can attempt to re-land this in a lando stack together with Bug 1599927.

(In reply to :kip (Kearwood Gilbert) from comment #20)

(In reply to :kip (Kearwood Gilbert) from comment #19)

I notice from the last try run, the patch is also causing:

TEST-UNEXPECTED-ERROR | /feature-policy/reporting/vr-reporting.https.html | unable to find test window

I'll investigate and update the patch.

Note that this is on the android platform only. We don't expect VR to be enabled on android, except in the geckoview-based Firefox Reality browser.

Is it resolved after applying the patch from Bug 1599927?

(In reply to Daosheng Mu[:daoshengmu] from comment #23)

(In reply to :kip (Kearwood Gilbert) from comment #20)

(In reply to :kip (Kearwood Gilbert) from comment #19)

I notice from the last try run, the patch is also causing:

TEST-UNEXPECTED-ERROR | /feature-policy/reporting/vr-reporting.https.html | unable to find test window

I'll investigate and update the patch.

Note that this is on the android platform only. We don't expect VR to be enabled on android, except in the geckoview-based Firefox Reality browser.

Is it resolved after applying the patch from Bug 1599927?

Indeed, it is! I'll land these two patches together.

Pushed by kgilbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/29809135f18e
Implement XR device access permission UI r=fluent-reviewers,bzbarsky,pbz,daoshengmu,imanol
Flags: needinfo?(kgilbert)
Backout by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fe86796921dd
Backed out 2 changesets (bug 1580567, bug 1599927) for causing permafailures in dom/tests/mochitest/general/test_interfaces.html CLOSED TREE

Looks like this needed to be re-reviewed by BZ (dom peer) to pass the test_interfaces.html after the rebasing / updating. I see there is a new r+ now, will re-attempt now in hopes it doesn’t get stomped again by another patch in auto land.

Flags: needinfo?(kgilbert)
Pushed by kgilbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/522c8d183bd5
Implement XR device access permission UI r=fluent-reviewers,bzbarsky,pbz,daoshengmu,imanol

Ugh.. I see now the last r+ from @bz was prior to the most recent rebase... sorry for causing another back out. Would you mind re-reviewing the patch, @bzbarsky ?

Flags: needinfo?(bzbarsky)
Pushed by kgilbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cce8edb56b19
Implement XR device access permission UI r=fluent-reviewers,bzbarsky,pbz,daoshengmu,imanol

https://hg.mozilla.org/mozilla-central/rev/cce8edb56b19

Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox73: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla73
Regressions: 1603396

Clearing NI related to earlier backout. This has now landed successfully.

Flags: needinfo?(kgilbert)

Hello,

On recent nightlies on linux (but I guess everywhere), each and ever web page that integrates a video from Vimeo provider does trigger the display of this notification. For a simple video, that feels weird. I understand that currently, anyway, the implementation is not ready on Linux, but I'm wondering how to check if it's legitimate that for a trivial video the vimeo integration triggers such VR permission.

Flags: needinfo?(kgilbert)

FTR, even on their website. One example: https://vimeo.com/304198961

(In reply to Alexandre LISSY :gerard-majax from comment #35)

Hello,

On recent nightlies on linux (but I guess everywhere), each and ever web page that integrates a video from Vimeo provider does trigger the display of this notification. For a simple video, that feels weird. I understand that currently, anyway, the implementation is not ready on Linux, but I'm wondering how to check if it's legitimate that for a trivial video the vimeo integration triggers such VR permission.

Hello,

Thanks for the feedback!

Many sites are using libraries that access the WebVR API during their initialization, even when the VR functions are not in use. The prompt is displayed before performing enumeration of VR hardware due to the enumeration itself having some side effects such as causing software to start up in the background or devices to power on.

This results in the prompt being displayed in cases where it might not be relevant.

I've filed a couple of follow-up bugs, which I'm working on now to help the situation:

Bug 1603825 - Suppress the VR permission UI when no VR runtimes are detected
Bug 1603631 - Consider hiding "Virtual Reality" permission options for Linux until WebXR is fully enabled on Linux

I hope this helps!

Flags: needinfo?(kgilbert)

(In reply to :kip (Kearwood Gilbert) from comment #37)

(In reply to Alexandre LISSY :gerard-majax from comment #35)

Hello,

On recent nightlies on linux (but I guess everywhere), each and ever web page that integrates a video from Vimeo provider does trigger the display of this notification. For a simple video, that feels weird. I understand that currently, anyway, the implementation is not ready on Linux, but I'm wondering how to check if it's legitimate that for a trivial video the vimeo integration triggers such VR permission.

Hello,

Thanks for the feedback!

Many sites are using libraries that access the WebVR API during their initialization, even when the VR functions are not in use. The prompt is displayed before performing enumeration of VR hardware due to the enumeration itself having some side effects such as causing software to start up in the background or devices to power on.

This results in the prompt being displayed in cases where it might not be relevant.

I've filed a couple of follow-up bugs, which I'm working on now to help the situation:

Bug 1603825 - Suppress the VR permission UI when no VR runtimes are detected
Bug 1603631 - Consider hiding "Virtual Reality" permission options for Linux until WebXR is fully enabled on Linux

I hope this helps!

Until these updates land, note that you can disable the permission prompts yourself. In the "Privacy and Security" section in the browser options screen, there is a new "Virtual Reality" permissions section. In there, you can activate the "Block new requests asking to access your virtual reality devices"

(In reply to :kip (Kearwood Gilbert) from comment #38)

[...]

Bug 1603825 - Suppress the VR permission UI when no VR runtimes are detected

So basically once this once is fixed, this would not bother people with no VR hardware. And those with VR hardware would, by just iterating, expose some informations, so the prompt makes sense there, even if it's just probing ?

Flags: needinfo?(bzbarsky)
Regressions: 1604853
Depends on: 1605989
Blocks: 1381645
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: