No notification to the user if autoplay is configured to permanently block

VERIFIED FIXED in Firefox 63

Status

()

enhancement
P1
normal
VERIFIED FIXED
10 months ago
7 months ago

People

(Reporter: daleharvey, Assigned: daleharvey)

Tracking

(Blocks 2 bugs)

unspecified
Firefox 63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 verified)

Details

Attachments

(2 attachments)

Assignee

Description

10 months ago
Configure autoplay to Block Always then visit https://html5zombo.com/, there is no indication to the user that we have blocked it from autoplaying
Assignee

Comment 1

10 months ago
Theres a similiar bug open @ https://bugzilla.mozilla.org/show_bug.cgi?id=1471723 for all icons, Ill give a shot at fixing it for all blocked permissions but leave this one in case we only fix it for autoplay
Assignee

Updated

10 months ago
Assignee: nobody → dharvey
Assignee

Comment 2

10 months ago
So one issue here is that as Chris predicted, when autoplay is configured to block we do that check in c++ and never defer to SitePermissions so the frontend has no way to even know of a request was blocked. 

It seems like the choices would be to remove the BLOCKED checked in c++ and have it call PROMPT everytime, having SitePermissions check whether its blocked or not and give that back, or for c++ to send an event down when something it blocked.

Johann I am not certain on how the rest of the permissions (geolocation etc) work when globally blocked and would be good to have this work for everything, got any pointers on how you think it should be done?
Flags: needinfo?(jhofmann)
(In reply to Dale Harvey (:daleharvey) from comment #2)
> So one issue here is that as Chris predicted, when autoplay is configured to
> block we do that check in c++ and never defer to SitePermissions so the
> frontend has no way to even know of a request was blocked. 
> 
> It seems like the choices would be to remove the BLOCKED checked in c++ and
> have it call PROMPT everytime, having SitePermissions check whether its
> blocked or not and give that back, or for c++ to send an event down when
> something it blocked.

Yes, that's basically what I thought, and I think it works best when the C++ just doesn't check the permission itself.

> Johann I am not certain on how the rest of the permissions (geolocation etc)
> work when globally blocked and would be good to have this work for
> everything, got any pointers on how you think it should be done?

I think you'd already do this endeavor a great service if you implemented this for one specific use case (autoplay) and from your patch we can figure out how to generalize it to the other permissions (they all have some sort of code that checks for blocked in C++ so I think it mostly requires consolidation in the front-end anyway).

Thanks!
Flags: needinfo?(jhofmann)
Status: NEW → ASSIGNED
Priority: -- → P1
Blocks: 1471723
It sounds like the fix here is to make the C++ function AutoplayPolicy::IsAllowedToPlay() return a bool. On true, assume we can play, otherwise, assume we need to ask.

There's work to spec a synchronous Web API HTMLMediaElement.allowedToPlay [1] for which Gecko would need to be able to check synchronously whether we've been granted permission to autoplay. The current form of that API should behave as if requests for permission would be denied, so Gecko doesn't need to distinguish between the ask and block cases, but may still need to poll for an ALLOW permission.

Having C++ always ask permission to autoplay means that the front end code would need to handle honouring the media.autoplay.default pref. That is, Gecko would dispatch a "can origin X play?" request to the front end, the front end would lookup the permission for origin X, if it's UNKNOWN, it would check media.autoplay.default, and if that's BLOCK show the control centre block icon and send back a block response to Gecko.

To me it seems that all would require us to add a hook to PermissionPromptForRequestPrototype which PermissionPromptPrototype.prompt() calls if it encounters and UNKNOWN permission to give the front end a chance to abort asking for permission with a default value?

[1] https://github.com/whatwg/html/issues/3617#issuecomment-401968833
Assignee

Comment 5

10 months ago
The frontend already checks the pref in SitePermissions, however I used the wrong constants, fixing it with 

-      if (state == Ci.nsIAutoplay.ALLOW) {
+      if (state == Ci.nsIAutoplay.ALLOWED) {
         return SitePermissions.ALLOW;
-      } if (state == Ci.nsIAutoplay.BLOCK) {
-        return SitePermissions.DENY;
+      } if (state == Ci.nsIAutoplay.BLOCKED) {
+        return SitePermissions.BLOCK;

and having c++ ignore the blocked case means we ask the frontend correctly and it behaves appropriately, now I think its just a matter of adding the persistent permission so it can be picked up by SitePermissions.getAll and then it should all work I believe
Comment hidden (mozreview-request)
Assignee

Comment 8

10 months ago
So this is a 'working' patch, will show the user a notification when autoplay is globally blocked. Just trying to think of the cleanest way to store that in the frontend as it doesnt fit into the current code very cleanly

I think its likely best to create a new object similiar to |TemporaryBlockedPermissions| that informs the UI about any global blocks when they have been accessed, but doesnt effect the actual fetching of permissions
Comment hidden (mozreview-request)
Assignee

Comment 10

10 months ago
Comment on attachment 8994458 [details]
Bug 1476555 - Show notification when autoplay blocked globally.

Hey Florian

I just wanted to check that I am going in the right direction (on the JS side, Chris can check the c++) as I am writing the tests for this, patch looks to be working well for me although I would like to make sure there arent cases where we could be leaving listeners that we do not want, I think we will probably want to limit it to the autoplay-media permission at first

Cheers
Attachment #8994458 - Flags: feedback?(florian)

Comment 11

10 months ago
mozreview-review
Comment on attachment 8994458 [details]
Bug 1476555 - Show notification when autoplay blocked globally.

https://reviewboard.mozilla.org/r/259010/#review266984


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/modules/SitePermissions.jsm:481
(Diff revision 3)
> -    if (state == this.UNKNOWN || state == this.getDefault(permissionID)) {
> +
> +    let defaultPermission = this.getDefault(permissionID);
> +
> +    if (scope == this.SCOPE_POLICY) {
> +      if (state == this.BLOCK) {
> +        dump('!!! SETTING GLOBALLY BLOCKED PERMISSION\n');

Error: Strings must use doublequote. [eslint: quotes]

::: browser/modules/SitePermissions.jsm:484
(Diff revision 3)
> +    if (scope == this.SCOPE_POLICY) {
> +      if (state == this.BLOCK) {
> +        dump('!!! SETTING GLOBALLY BLOCKED PERMISSION\n');
> +        GloballyBlockedPermissions.set(browser, permissionID);
> +      } else {
> +        dump('!!! REMOVING GLOBALLY BLOCKED PERMISSION\n');

Error: Strings must use doublequote. [eslint: quotes]
Comment hidden (mozreview-request)

Comment 13

10 months ago
(In reply to Dale Harvey (:daleharvey) from comment #5)
> The frontend already checks the pref in SitePermissions, however I used the
> wrong constants, fixing it with 
> 
> -      if (state == Ci.nsIAutoplay.ALLOW) {
> +      if (state == Ci.nsIAutoplay.ALLOWED) {
>          return SitePermissions.ALLOW;
> -      } if (state == Ci.nsIAutoplay.BLOCK) {
> -        return SitePermissions.DENY;
> +      } if (state == Ci.nsIAutoplay.BLOCKED) {
> +        return SitePermissions.BLOCK;
> 
> and having c++ ignore the blocked case means we ask the frontend correctly
> and it behaves appropriately, now I think its just a matter of adding the
> persistent permission so it can be picked up by SitePermissions.getAll and
> then it should all work I believe

Can you file a follow-up bug to write tests so that this code is actually covered by tests, so we notice if we write bogus stuff here? Thank you! :-)
Flags: needinfo?(dharvey)
(In reply to Dale Harvey (:daleharvey) from comment #10)

> I just wanted to check that I am going in the right direction (on the JS
> side, Chris can check the c++) as I am writing the tests for this, patch
> looks to be working well for me although I would like to make sure there
> arent cases where we could be leaving listeners that we do not want, I think
> we will probably want to limit it to the autoplay-media permission at first

I looked at the patch and didn't see any obvious problem, but I haven't worked on this code much in the last year, so I would be more comfortable if Johann checks the direction this is taking once he returns.
Blocks: 1479513
Assignee

Comment 15

10 months ago
> Can you file a follow-up bug to write tests so that this code 
> is actually covered by tests, so we notice if we write bogus 
> stuff here? Thank you! :-)

Yup definitely worth making sure to not make the same mistake. This patch 
will add tests to make sure that code is tested for autoplay and Marks bug 
looks good for making sure it doesnt happen again
Flags: needinfo?(dharvey)
Comment hidden (mozreview-request)
It looks like you're not actually testing that the block icon appears in the URL bar when we're set to block by default, only that it's there when you click on the identity box?

I think you can test that the icon appears in the URL bar with this patch. Note it's hacky in that I added a setTimeout() in order to account for the delay it takes for the icon to appear; that needs to be fixed. But this is the general approach you could take for testing whether the icon shows.

Comment 18

10 months ago
mozreview-review
Comment on attachment 8994458 [details]
Bug 1476555 - Show notification when autoplay blocked globally.

https://reviewboard.mozilla.org/r/259010/#review268154

Almost there on the C++ side, just some unnecessary code to remove. It would be good if you could add a test for the block icon appearing when we block too.

::: dom/media/AutoplayPolicy.cpp:140
(Diff revision 5)
>  
>    if (IsMediaElementAllowedToPlay(aElement)) {
> -    return nsIAutoplay::ALLOWED;
> +    return true;
>    }
>  
> -  return autoplayDefault;
> +  // Muted content

You'll need to rebase on top of Bug 1480281, which changes this code.

Note IsMediaElementAllowedToPlay() checks everything here except autoplayDefault. So you don't need to re-add the checks for volume/muted/IsWindowAllowedToPlay().
Attachment #8994458 - Flags: review?(cpearce) → review-
Comment hidden (mozreview-request)

Comment 20

10 months ago
mozreview-review
Comment on attachment 8994458 [details]
Bug 1476555 - Show notification when autoplay blocked globally.

https://reviewboard.mozilla.org/r/259010/#review268486

C++ looks fine.

::: dom/media/AutoplayPolicy.cpp:169
(Diff revision 6)
>      // If element is blessed, it would always be allowed to play().
>      return (autoplayDefault == nsIAutoplay::ALLOWED ||
>              aElement.IsBlessed() ||
> -            EventStateManager::IsHandlingUserInput())
> -              ? nsIAutoplay::ALLOWED : nsIAutoplay::BLOCKED;
> +            EventStateManager::IsHandlingUserInput());
> +  }
> +

Extra blank line here.
Attachment #8994458 - Flags: review?(cpearce) → review+

Comment 21

10 months ago
mozreview-review
Comment on attachment 8994458 [details]
Bug 1476555 - Show notification when autoplay blocked globally.

https://reviewboard.mozilla.org/r/259010/#review268704

So, my original idea for this was that, instead of displaying the blocked icon, we could show a minimized doorhanger (just the doorhanger icon) that the user could maximize and approve the permission whenever they liked. However, I don't mind if you prefer this variant (showing a crossed-out icon) and maybe it's a little more consistent.

Anyway, this patch is definitely a feedback+, just that I'd like to avoid conflating two concepts into SCOPE_POLICY.

Thanks!

::: browser/modules/PermissionUI.jsm:288
(Diff revision 6)
> +        // If the request is blocked by a global setting then we record
> +        // a flag that lasts for the duration of the current page load
> +        // to notify the user that the permission has been blocked.
> +        // Currently only applies to autoplay-media
> +        if (state == SitePermissions.getDefault(this.permissionKey) &&
> +            this.permissionKey === "autoplay-media") {

How about introducing a flag like this.showGloballyBlocked instead of doing this comparison here?

::: browser/modules/SitePermissions.jsm:136
(Diff revision 6)
>  
> +// This hold a flag per browser to indicate whether we should show the
> +// user a notification as a permission has been requested that has been
> +// blocked globally. We only want to notify the user in the case that
> +// they actually requested the permission within the current page load
> +// so will clear the flag on navigation

typo: page load so will clear the flag

(also missing a dot at the end)

::: browser/modules/SitePermissions.jsm:153
(Diff revision 6)
> +      entry[prePath] = {};
> +    }
> +
> +    entry[prePath][id] = true;
> +
> +    // Listen to any top level navigations, once we so one, clear the flag

typo: once we so one?

::: browser/modules/SitePermissions.jsm:477
(Diff revision 6)
>     *        The browser object to set temporary permissions on.
>     *        This needs to be provided if the scope is SCOPE_TEMPORARY!
>     */
>    set(uri, permissionID, state, scope = this.SCOPE_PERSISTENT, browser = null) {
> +
> +    if (scope == this.SCOPE_POLICY && state == this.BLOCK) {

I know using SCOPE_POLICY is convenient because it already handles the UI correctly for the identity popup, but I think it's a bad idea to conflate the two concepts. We should come up with a new scope instead, something like SCOPE_GLOBAL?

It's a little bit more work by updating the places where we use SCOPE_POLICY to also accept GLOBAL but I think it's worth it.

::: browser/modules/SitePermissions.jsm:480
(Diff revision 6)
>    set(uri, permissionID, state, scope = this.SCOPE_PERSISTENT, browser = null) {
> +
> +    if (scope == this.SCOPE_POLICY && state == this.BLOCK) {
> +      GloballyBlockedPermissions.set(browser, permissionID);
> +      browser.dispatchEvent(new browser.ownerGlobal
> +                            .CustomEvent("PermissionStateChange"));

nit: This indentation looks weird to me. Can you either give it an indentation of 2 spaces or make it align with the dot of .ownerGlobal?

I'm actually not even sure you need to put this on a new line :)
Attachment #8994458 - Flags: review?(jhofmann) → review-
Comment hidden (mozreview-request)
Assignee

Comment 23

10 months ago
Yeh I was considering a new flag but I wasnt certain that it wasnt what SCOPE_POLICY is actually made for, awesome thanks made those changes
Comment hidden (mozreview-request)

Comment 25

9 months ago
mozreview-review
Comment on attachment 8994458 [details]
Bug 1476555 - Show notification when autoplay blocked globally.

https://reviewboard.mozilla.org/r/259010/#review269410

r=me, see comments :)

Thanks for this and sorry for the delay!

::: browser/base/content/test/permissions/browser_autoplay_blocked.js:23
(Diff revision 8)
> +
> +function autoplayBlockedIcon() {
> +  return document.querySelector("#blocked-permissions-container " +
> +                                ".blocked-permission-icon.autoplay-media-icon");
> +}
> +add_task(async function testMainViewVisible() {

super tiny nit: can you add blank space above the add_task?

::: browser/modules/SitePermissions.jsm:155
(Diff revision 8)
> +
> +    entry[prePath][id] = true;
> +
> +    // Listen to any top level navigations, once we see one clear the flag
> +    // and remove the listener.
> +    browser.addProgressListener({

This is fine by me, but I just wanted to throw in what I would have done here:

Call a function like SitePermissions.clearGlobalPermissionForBrowser(browser) in https://searchfox.org/mozilla-central/rev/ca869724246f4230b272ed1c8b9944596e80d920/browser/base/content/browser.js#4677

Anyway, just a thought, not sure if it's worth a follow-up or whether we should just forget about it :)

::: browser/modules/SitePermissions.jsm:189
(Diff revision 8)
> +      let timeStamps = entry[prePath];
> +      for (let id of Object.keys(timeStamps)) {
> +        permissions.push({
> +          id,
> +          state: SitePermissions.BLOCK,
> +          scope: SitePermissions.SCOPE_POLICY

I think this is supposed to be SCOPE_GLOBAL now, right?
Attachment #8994458 - Flags: review?(jhofmann) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 28

9 months ago
Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1cee6e0d07e7
Show notification when autoplay blocked globally. r=cpearce,johannh

Comment 29

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1cee6e0d07e7
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63

Comment 30

9 months ago
Verified, that the issue is no longer reproducible on Nightly 63.0a1(20180817100105).
Status: RESOLVED → VERIFIED

Updated

9 months ago
See Also: → 1485955

Updated

7 months ago
Blocks: 1493766

Updated

7 months ago
Blocks: 1502046
You need to log in before you can comment on or make changes to this bug.