Closed
Bug 1243431
Opened 9 years ago
Closed 8 years ago
Guard vibration API with permission
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: snorp, Assigned: esawin, NeedInfo)
Details
(Whiteboard: dom-triaged)
Attachments
(8 files, 11 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
esawin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
esawin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
antlam
:
feedback+
|
Details |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gbrown
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
Apparently some websites are abusing the vibration API (shocking, I know) on Android. We should guard it with a permission to avoid this.
Comment 1•9 years ago
|
||
Would it be enough to turn it off for background tabs?
Comment 2•9 years ago
|
||
Assuming [1] is even the right spec, then we shouldn't vibrate for non-visible tabs:
"If the hidden attribute [PAGE-VISIBILITY] is set to true, then return false and terminate these steps."
(if it's not the right spec, it sounds like a better thing to do)
[1] <http://dev.w3.org/2009/dap/vibration/>
Comment 3•9 years ago
|
||
> Would it be enough to turn it off for background tabs?
It _is_ turned off for background tabs:
792 MayVibrate(nsIDocument* doc) {
...
800 return (doc && !doc->Hidden());
It should also be turned off when the browser app is not in foreground, I would think, for the same reason...
Comment 4•9 years ago
|
||
Is it possible Android is not setting visibility state correctly on background tabs?
Comment 5•9 years ago
|
||
I meant Fennec, not Android, of course. Simple testcase:
<pre id="log"></pre>
<script>
function log(str) {
document.getElementById("log").appendChild(document.createTextNode(str + "\n"));
}
document.addEventListener("visibilitychange", function() {
log(document.hidden);
});
log(document.hidden);
</script>
Load the page, switch to another tab, switch back to this one. Log should show "false", "true", "false". And does for me on Fennec, so that part seems to be fine.
Comment 6•9 years ago
|
||
And I just tested that if I load that page, switch to another app, then switch back to Firefox on Android, I also get "false", "true", "false". So we're correctly setting the document hidden state when Firefox is not the current app as well.
Which means the only way a site can use this API is if it's loaded in the foreground tab of a currently-active Firefox.
Oh, and if I load that page, leave Firefox up, then allow the phone to turn off its screen (so it goes into the lockscreen when I hit the little "talk to me" button), I also get "false", "true", "false". Dunno whether we go into the hidden state when the screen turns off or when the lockscreen comes up, though. Could probably tell by logging some timestamps too.
Reporter | ||
Comment 7•9 years ago
|
||
According to the reporter (from reddit), the pages are in the foreground. I guess adult and/or sketchy pages open popups that abuse this.
Comment 8•9 years ago
|
||
> According to the reporter (from reddit), the pages are in the foreground.
That's not what he's saying in https://www.reddit.com/r/firefox/comments/42yeki/is_something_being_done_to_take_care_of/czg7tdr afaict.
Reporter | ||
Comment 9•9 years ago
|
||
Comment 10•9 years ago
|
||
i've spent a long time today trying to get those URL but sorry havn't been able to get any.
Im not sure if they change their ad hosts/partners but if i do stumble on the issue ill be sure to post the link.
On another note:
Wouldn't a test site with the pop-up vibration thing replicate the same situation?
Comment 11•9 years ago
|
||
Sure, but at that point it only replicates in the foreground tab and if Firefox is the current app.
As in, you navigate to a site in the foreground tab and it vibrates.
Updated•9 years ago
|
Whiteboard: dom-noted
Updated•9 years ago
|
Whiteboard: dom-noted → dom-triaged
Comment 12•9 years ago
|
||
A user had a scary incident with this. Sadly didn't get the URL
https://www.reddit.com/r/firefox/comments/42yeki/is_something_being_done_to_take_care_of/czopweq
Comment 13•9 years ago
|
||
Thanks for the update bull500.
> However, it was not only able to make my phone vibrate, but it could also control the screen brightness, as in, it made the brightness go up and down as if it were a "malfunctioning light bulb".
I don't think it's possible to actually control screen brightness (we have an API to read ambient light, but that can't set screen brightness) -- but you could probably do some kind of stupid opacity animation to mimic it.
Comment 14•9 years ago
|
||
It's also possible that on some phone models vibration is accompanied by coordinated screen brightness changes or something. A "visual vibrate" of sorts...
Comment 15•9 years ago
|
||
Got a couple of links
http://mangodigitalc.com/?zoneid=469237
http://j61launchers.com/?l=McboESTmK3H8wTr&s=146467506331&z=469237&g=IN&ba=1&dm=1&ep=1&vi=1&vo=1&fp=1&tr=in&language=hi
http://go.padsdel.com/afu.php?id=469237&var=469237
I don't think you'll get the first two but that padsdel site seems to redirect to mainly vibration causing sites and app downloads as well(pop-up for installation via playstore or fdroid is default is not chosen)
Take caution!
Comment 16•9 years ago
|
||
someone found a solid link for this
https://www.reddit.com/r/Android/comments/4d9f9n/malicious_use_of_the_html5_vibrate_api/
https://shkspr.mobi/blog/2014/01/malicious-use-of-the-html5-vibrate-api/
the demo link - https://shkspr.mobi/vibratescam/
Firefox is neatly mentioned
Comment 17•9 years ago
|
||
My friend Tim tweeted about an ad doing this over the weekend:
view-source:http://shell.mobile-winner.net/?subid=1460237137mb92739501774&zoneid=7009&sxid=8ym3031172ky#b
<script type="text/javascript">
navigator.vibrate = navigator.vibrate || navigator.webkitVibrate || navigator.mozVibrate || navigator.msVibrate;
navigator.vibrate([1000, 500, 1000, 500, 1000, 500, 1000, 500, 1000]);
</script>
<script>
alert("Congratulations!\n\nYou have been selected to participate in our survey to receive a $250 Shell Gift Card!\n\nPress OK to continue.");
</script>
Updated•9 years ago
|
tracking-fennec: --- → ?
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → esawin
tracking-fennec: ? → +
Assignee | ||
Comment 18•9 years ago
|
||
The protection level for "android.permission.VIBRATE" is "normal", which means our behavior of granting the permission automatically at installation is correct.
Here are some of the options we have to prevent abuse of this API:
1. Use a proxy permission with protection level "dangerous"
* Prompt the user for permission on first use
* Permission can be revoked via Android's permission UI
2. Blacklist the vibration API via pref/Fennec UI
3. Detect and block abusive API usage (based on user interaction? don't really see a reliable way)
Option 1 requires the user to manually revoke granted permissions manually. I don't see a way to force the prompt once the permission has been granted, is that possible, sebastian?
Options 1 and 2 would require strings (l10n) for the prompt/UI.
Flags: needinfo?(s.kaspari)
Comment 19•9 years ago
|
||
(In reply to Eugen Sawin [:esawin] from comment #18)
> Here are some of the options we have to prevent abuse of this API:
> 1. Use a proxy permission with protection level "dangerous"
> * Prompt the user for permission on first use
> * Permission can be revoked via Android's permission UI
This sounds like we'd exploit the permission system. This is for granting a permission to the application. Effectively we'd ask the user to grant Firefox a permission it already has. Apart from that I'm not even sure if this works with custom permissions. Would be interesting to try.
Isn't this more about a permission the user grants a website and where we show those doorhangers (Video, Microphone, Notification, ..)? Or if this should be global a setting (but then it probably needs to be off by default to avoid this abuse?).
> Option 1 requires the user to manually revoke granted permissions manually
> I don't see a way to force the prompt once the permission has been granted,
> is that possible, sebastian?
Nope. Those permissions are permanent (until revoked in Android's settings) and you can only show the prompt to ask for it if you don't have it (not revoke).
Flags: needinfo?(s.kaspari)
Reporter | ||
Comment 20•9 years ago
|
||
Yeah, the terminology here is a little confusing. In the context used in the bug summary, I'm referring to a Firefox permission and not an Android one.
Assignee | ||
Comment 21•8 years ago
|
||
Some refactoring to reduce code duplication.
Attachment #8745369 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 22•8 years ago
|
||
Navigator will dispatch "Vibration:request" and listen to "Vibration:response:*" events to allow for user permission request handling.
Attachment #8745371 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 23•8 years ago
|
||
Show doorhanger UI on "Vibration:request" and dispatch results.
This patch adds strings.
Attachment #8745372 -
Flags: review?(margaret.leibovic)
Comment 24•8 years ago
|
||
Comment on attachment 8745369 [details] [diff] [review]
0001-Bug-1243431-1.1-Add-permission-handling-utility-func.patch
>+ if (!permMgr || !aWindow || !aType) {
Why would aType ever be null here? I think you should be able to just assert it's not null.
I think GetPermission or GetPermissionValue is a better name for this helper, since it's not testing anything; all the testing is in the callers.
>+ if (!permMgr || !aPrincipal || !aType) {
Again, no need to check aType. Probably no need to check aPrincipal either; it should never be null.
>@@ -2217,12 +2254,7 @@ Navigator::CheckPermission(nsPIDOMWindowInner* aWindow, const char* aType)
You don't need the aWindow null-check here; it'll be done by TestPermission anyway. Please take it out.
r=me
Attachment #8745369 -
Flags: review?(bzbarsky) → review+
Comment 25•8 years ago
|
||
Comment on attachment 8745372 [details] [diff] [review]
0003-Bug-1243431-3.1-Show-doorhanger-on-vibration-request.patch
Review of attachment 8745372 [details] [diff] [review]:
-----------------------------------------------------------------
You should ask antlam to review the strings here, but otherwise looks fine with comments addressed.
::: mobile/android/chrome/content/browser.js
@@ +1951,5 @@
> + let buttons = [
> + {
> + label: Strings.browser.GetStringFromName("vibrationRequest.denyButton"),
> + callback: function() {
> + Services.obs.notifyObservers(null, "Vibration:response:deny", null);
Nit: This message doesn't really follow our capitalization convention, but it wasn't added in this patch.
@@ +1964,5 @@
> + }
> + ];
> + let host = browser.currentURI.host;
> + let message = Strings.browser.formatStringFromName(
> + "vibrationRequest.message", [host], 1);
Nit: This can all just go on one line, we don't care about long lines in JS.
@@ +1967,5 @@
> + let message = Strings.browser.formatStringFromName(
> + "vibrationRequest.message", [host], 1);
> + let options = {};
> + NativeWindow.doorhanger.show(message, "vibration-request", buttons,
> + BrowserApp.selectedTab.id, options, "VIBRATION");
I don't think you need this last parameter, since we don't have a special "VIBRATION" type of doorhanger. This would require a change to DoorHangerPopup.java as well. However, you should probably make that change, so that we can use this value for telemetry.
Look at this changeset for an example of what we did for WebRTC:
http://hg.mozilla.org/mozilla-central/rev/1fde3cca31b4
@@ +1968,5 @@
> + "vibrationRequest.message", [host], 1);
> + let options = {};
> + NativeWindow.doorhanger.show(message, "vibration-request", buttons,
> + BrowserApp.selectedTab.id, options, "VIBRATION");
> + break;
You should put braces around the body of this case statement, since let statements have block scope.
Attachment #8745372 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 26•8 years ago
|
||
Comment on attachment 8745372 [details] [diff] [review]
0003-Bug-1243431-3.1-Show-doorhanger-on-vibration-request.patch
Anthony, can you please review the new strings.
Attachment #8745372 -
Flags: review+ → review?(alam)
Assignee | ||
Comment 27•8 years ago
|
||
Addressed comments.
Carrying over r+ from Margaret.
Anthony r? for strings.
Attachment #8745372 -
Attachment is obsolete: true
Attachment #8745372 -
Flags: review?(alam)
Attachment #8746051 -
Flags: review?(alam)
Comment 28•8 years ago
|
||
Comment on attachment 8746051 [details] [diff] [review]
0003-Bug-1243431-3.2-Show-doorhanger-on-vibration-request.patch
Quick question, is "%S" the site?
With out current doorhanger design, it should already display the site above this line. That would make this quite repetitive.
If you don't mind, could you share a screenshot? that would be a lot easier too :) thanks!
Flags: needinfo?(esawin)
Attachment #8746051 -
Flags: review?(alam) → review-
Assignee | ||
Comment 29•8 years ago
|
||
Flags: needinfo?(esawin)
Assignee | ||
Comment 30•8 years ago
|
||
Assignee | ||
Comment 31•8 years ago
|
||
I've attached both, the new vibration request and the existing camera request doorhangers. The domain name is redundant but consistent with what we currently have.
Comment 32•8 years ago
|
||
Comment on attachment 8745371 [details] [diff] [review]
0002-Bug-1243431-2.1-Guard-Vibration-API-with-user-permis.patch
>+ obs->AddObserver(this, "Vibration:response:allow", false);
This is not a great pattern. We're passing a zero-refcount object to someone who might end up refcounting it. That might be OK, or it might cause us to die before the call returns, depending on what the callee does.
If we really need this, we should be adding Navigator as an observer after the constructor returns. But do we really need to add as observer before the first time someone calls Vibrate()? Seems like it would be better to register in there, if mVibratorPromise.IsEmpty().
Also, why two separate notifications instead of one notification which sends allow/deny as the string data?
And also also, why notifications at all? That seems pretty weird to me, especially because we never clear mVibratorPromise. If two different navigator objects both try to vibrate one after another, won't both of them get the notification that's mean to be a response to one of them?
What I think we should be doing instead is that that our "Vibration:request" notification should send the Navigator as the subject. Observers should then just call methods on that Navigator directly to tell it whether it's OK to vibrate.
And even then, we should think a bit about what happens if another vibrate() call happens while we're waiting on a permission response to a preceding one on the same Navigator.
>@@ -952,21 +995,36 @@ Navigator::Vibrate(const nsTArray<uint32_t>& aPattern)
>+ if (!aPattern.Length() || (aPattern.Length() == 1 && aPattern[0] == 0) ||
>+ permission == nsIPermissionManager::ALLOW_ACTION) {
I'd prefer the TestPermission() call were just inline here, so we don't even make that call when canceling.
Also, aPattern.IsEmpty() is probably better than !aPattern.Length().
>+ GetVibrator(pattern)->Then(AbstractThread::MainThread(), __func__,
These closures will run async. That means the resolve handler probably needs to recheck sanity stuff like mWindow not being null, mWindow->GetExtantDoc() not being null, and MayVibrate(mWindow->GetExtantDoc()). Note that you'll want to reget doc too, because the document in mWindow might have changed while we were waiting on that promise.
>+ nsIURI* uri = doc->GetDocumentURI();
>+ AddPermission(uri, "vibration", nsIPermissionManager::ALLOW_ACTION,
>+ nsIPermissionManager::EXPIRE_SESSION, 0);
This looks wrong to me: it'll fail when someone tries to vibrate in a srcdoc document, or blob:, etc, etc.
You presumably want to tie this permission to the principal URI or something, not the document URI...
I assume the UX for EXPIRE_SESSION is in fact the one we want here.
Also, please make GetVibrator return already_AddRefed instead of RefPtr, and just put it in a temporary on the stack before calling methods on it. That will make the ownership a lot clearer.
Attachment #8745371 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 33•8 years ago
|
||
Addressed comments.
Attachment #8745369 -
Attachment is obsolete: true
Attachment #8746514 -
Flags: review+
Assignee | ||
Comment 34•8 years ago
|
||
Due to the new approach taken, most review comments for patch version 2.1 no longer apply. I'm addressing the rest below.
(In reply to Boris Zbarsky [:bz] from comment #32)
> What I think we should be doing instead is that that our "Vibration:request"
> notification should send the Navigator as the subject. Observers should
> then just call methods on that Navigator directly to tell it whether it's OK
> to vibrate.
I've implemented this approach with this patch (2.2). For this, we expose a chrome-only method (setVibrationPermission) which is then called with the appropriate argument value in the doorhanger callback.
> And even then, we should think a bit about what happens if another vibrate()
> call happens while we're waiting on a permission response to a preceding one
> on the same Navigator.
As per spec, successive calls to vibrate() override previous patterns. I'm keeping the same logic for pre-authorized calls, i.e., only the lastly requested pattern before a successful permission response will be executed.
> >+ nsIURI* uri = doc->GetDocumentURI();
> >+ AddPermission(uri, "vibration", nsIPermissionManager::ALLOW_ACTION,
> >+ nsIPermissionManager::EXPIRE_SESSION, 0);
>
> This looks wrong to me: it'll fail when someone tries to vibrate in a srcdoc
> document, or blob:, etc, etc.
>
> You presumably want to tie this permission to the principal URI or
> something, not the document URI...
I'm now tying the permission to the document's node principal, is this OK?
Attachment #8745371 -
Attachment is obsolete: true
Attachment #8746520 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 35•8 years ago
|
||
I've removed the domain name from the message.
I'm not sure if it sounds right now, maybe we should add "here" or "on this website" at the end?
Attachment #8746051 -
Attachment is obsolete: true
Attachment #8746527 -
Flags: review?(alam)
Assignee | ||
Comment 36•8 years ago
|
||
This patch uses the new Navigator API call to permit vibration.
I will flag this for review, once patch 2.2 has been reviewed.
I've also encountered bug 1268471 with this.
Comment 37•8 years ago
|
||
Comment on attachment 8746520 [details] [diff] [review]
0002-Bug-1243431-2.2-Guard-Vibration-API-with-user-permis.patch
We should probably clear mRequestedVibrationPattern on the early returns from SetVibrationPermission. Maybe SwapElements it into an on-stack array up front in that method to make that easy?
I assume using a response to a previous request for a new call is OK because the response won't depend on things that might have changed in the interim, right? I I guess we're not providing that much information in our request, so I can't see how it can, ok. I guess that lack of info is OK for the request recipient because they can assume the currently selected tab is what's making the request due to the MayVibrate() check we did? Might be worth documenting.
>+++ b/dom/webidl/Navigator.webidl
The addition here needs documentation. And it should go in a separate partial interface or in the nsIDOMNavigator one or something, not in the spec IDL snippet, since this is not part of the spec.
r=me with those nits.
Attachment #8746520 -
Flags: review?(bzbarsky) → review+
Comment 38•8 years ago
|
||
Oh, one more thing. This bit:
static_cast<nsIDOMNavigator*>(this)
can probably be replaced with ToSupports(this). That's going to invoke http://hg.mozilla.org/mozilla-central/file/4292da9df16b/xpcom/glue/nsCycleCollectionNoteChild.h#l48 which is going to work for Navigator, because it's cycle-collected.
Assignee | ||
Comment 39•8 years ago
|
||
Comment on attachment 8746532 [details] [diff] [review]
0004-Bug-1243431-4.1-Show-permission-request-doorhanger-o.patch
Thanks Boris, I'll address the comments before landing.
I have to run the chrome patch through your review again, Margaret, since the Navigator API has changed.
Attachment #8746532 -
Flags: review?(margaret.leibovic)
Comment 40•8 years ago
|
||
(In reply to Eugen Sawin [:esawin] from comment #29)
> Created attachment 8746110 [details]
> Vibration request doorhanger
The language in our permissions really needs to be cleaned up... but that's a bug for another day.
I'm thinking we can say something like this for now:
Would you like to allow this site to vibrate your device?
Don't allow | Allow
Flags: needinfo?(esawin)
Assignee | ||
Comment 41•8 years ago
|
||
Addressed comments.
Attachment #8746520 -
Attachment is obsolete: true
Attachment #8747080 -
Flags: review+
Assignee | ||
Comment 42•8 years ago
|
||
Attachment #8746527 -
Attachment is obsolete: true
Attachment #8746527 -
Flags: review?(alam)
Flags: needinfo?(esawin)
Attachment #8747081 -
Flags: review?(alam)
Assignee | ||
Comment 43•8 years ago
|
||
One last patch to enable vibration in the mochitest to test the full code path.
Attachment #8747141 -
Flags: review?(bzbarsky)
Comment 44•8 years ago
|
||
Can you post a screenshot please? :) it'll help me a lot more to see what this looks like "on device".
Also I noticed this in the diff
"vibrationRequest.denyButton = Dont' allow" ... I think you misplaced the apostrophe ;)
Flags: needinfo?(esawin)
Updated•8 years ago
|
Attachment #8747081 -
Flags: review?(alam) → review-
Assignee | ||
Comment 45•8 years ago
|
||
Oh, sorry about that.
I'll post a screenshot once the build has finished.
Attachment #8747081 -
Attachment is obsolete: true
Attachment #8747154 -
Flags: review?(alam)
Assignee | ||
Comment 46•8 years ago
|
||
Attachment #8746110 -
Attachment is obsolete: true
Flags: needinfo?(esawin)
Assignee | ||
Comment 47•8 years ago
|
||
Comment 48•8 years ago
|
||
Comment on attachment 8747141 [details] [diff] [review]
0005-Bug-1243431-5.1-Enable-vibration-permission-for-test.patch
Won't this just throw, since the API is chromeonly?
Attachment #8747141 -
Flags: review?(bzbarsky) → review-
Updated•8 years ago
|
Attachment #8747154 -
Flags: review?(alam)
Comment 49•8 years ago
|
||
(In reply to Eugen Sawin [:esawin] from comment #46)
> Created attachment 8747190 [details]
> Vibration request doorhanger
Actually reading it in the doorhanger - it sounds like "Would you like to" seems redundant.
"Allow this site to vibrate your device?" is enough.
Can we update that? Thanks!
Flags: needinfo?(esawin)
Assignee | ||
Comment 50•8 years ago
|
||
Attachment #8747190 -
Attachment is obsolete: true
Flags: needinfo?(esawin)
Attachment #8747752 -
Flags: feedback?(alam)
Comment 51•8 years ago
|
||
Comment on attachment 8747752 [details]
doorhanger.png
wfm!
Attachment #8747752 -
Flags: feedback?(alam) → feedback+
Assignee | ||
Comment 52•8 years ago
|
||
The Try run was green with the broken test because we exclude the test on Android.
With this patch, we add the vibration permission for the test.
Attachment #8747141 -
Attachment is obsolete: true
Attachment #8748128 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 53•8 years ago
|
||
The vibration test has been disabled for years because of "CRASH_SUTAGENT".
The test run with it enabled looks good now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d75987225f0e
Is it safe to enable this test on Android?
Attachment #8748129 -
Flags: review?(gbrown)
Assignee | ||
Comment 54•8 years ago
|
||
Attachment #8747154 -
Attachment is obsolete: true
Attachment #8748130 -
Flags: review?(alam)
Comment 55•8 years ago
|
||
Comment on attachment 8748129 [details] [diff] [review]
0006-Bug-1243431-6.1-Enable-test_vibrator-on-Android.-r-g.patch
Review of attachment 8748129 [details] [diff] [review]:
-----------------------------------------------------------------
lgtm
Attachment #8748129 -
Flags: review?(gbrown) → review+
Comment 56•8 years ago
|
||
Comment on attachment 8748130 [details] [diff] [review]
0003-Bug-1243431-3.6-Add-strings-for-Vibration-API-permis.patch
Sorry, I shouldn't review patches :)
Attachment #8748130 -
Flags: review?(alam)
Assignee | ||
Comment 57•8 years ago
|
||
Comment on attachment 8748130 [details] [diff] [review]
0003-Bug-1243431-3.6-Add-strings-for-Vibration-API-permis.patch
Anthony has accepted the strings, redirecting to Margaret for review.
Attachment #8748130 -
Flags: review?(margaret.leibovic)
Comment 58•8 years ago
|
||
Comment on attachment 8748128 [details] [diff] [review]
0005-Bug-1243431-5.2-Enable-vibration-permission-for-test.patch
r=me
Attachment #8748128 -
Flags: review?(bzbarsky) → review+
Updated•8 years ago
|
Attachment #8748130 -
Flags: review?(margaret.leibovic) → review+
Comment 59•8 years ago
|
||
Comment on attachment 8746532 [details] [diff] [review]
0004-Bug-1243431-4.1-Show-permission-request-doorhanger-o.patch
Review of attachment 8746532 [details] [diff] [review]:
-----------------------------------------------------------------
Oops, sorry, I saw the other review request but not this one.
Attachment #8746532 -
Flags: review?(margaret.leibovic) → review+
Comment 60•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/afce415c3efd
https://hg.mozilla.org/integration/mozilla-inbound/rev/64aef96581e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/0370882039d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/379cf1150c67
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a9f596c062d
https://hg.mozilla.org/integration/mozilla-inbound/rev/1efb537d42a6
Comment 61•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/afce415c3efd
https://hg.mozilla.org/mozilla-central/rev/64aef96581e4
https://hg.mozilla.org/mozilla-central/rev/0370882039d7
https://hg.mozilla.org/mozilla-central/rev/379cf1150c67
https://hg.mozilla.org/mozilla-central/rev/8a9f596c062d
https://hg.mozilla.org/mozilla-central/rev/1efb537d42a6
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 62•8 years ago
|
||
We're debating the right design (and eventually spec text) for this over here: https://github.com/WICG/interventions/issues/25
In particular I'm arguing that this is really conceptually just like the autoplay audio debate and vibration should be enabled after the user has tapped the frame. We'd love any input any of you have on that over in WICG.
Comment 63•8 years ago
|
||
Barbara, who is the right Mozilla person in product or engineering to discuss navigator.vibrate() API permissions and UX in Firefox Mobile browsers?
Flags: needinfo?(bbermes)
Comment 64•8 years ago
|
||
NIing Joe who will make sure this gets addressed. Most likely UX and Eng from Taipei will take this on.
Flags: needinfo?(jcheng)
Flags: needinfo?(hhsu)
Flags: needinfo?(bbermes)
Comment 65•8 years ago
|
||
As Rick mentioned above, we're discussing the design (and eventually spec text) for this over this new thread: https://github.com/WICG/interventions/issues/47, a follow-up effort of https://github.com/WICG/interventions/issues/25.
The idea is that vibration should be enabled after the user has tapped the frame. We'd love any input any of you have on that over in WICG. Thanks.
Updated•7 years ago
|
Flags: needinfo?(hhsu)
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•