Closed Bug 1461656 Opened 5 years ago Closed 5 years ago

Ask permission when user wants to autoplay media

Categories

(Firefox :: Site Identity, enhancement, P1)

Product:
Firefox
Component:
Site Identity
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: daleharvey, Assigned: daleharvey)

References

Details

Attachments

(1 file)

Bug 1461656 - Ask permission when site wants to autoplay media.
59 bytes, text/x-review-board-request
johannh
: review+
flod
: review+
Details 
      No description provided.
Assignee: nobody → dharvey
Priority: -- → P1
Summary: Add doorhanger permission prompt when autoplay denied → Ask permission when user wants to autoplay media
In the middle of writing browser tests for this, it requires the platform code @ https://bugzilla.mozilla.org/show_bug.cgi?id=1463919 in order for the tests to land, not sure if we can land this plumbing first and then land the tests when the platform piece has landed or we should merge them all into one bug (1463919) ?
(In reply to Dale Harvey (:daleharvey) from comment #3)
> In the middle of writing browser tests for this, it requires the platform
> code @ https://bugzilla.mozilla.org/show_bug.cgi?id=1463919 in order for the
> tests to land, not sure if we can land this plumbing first and then land the
> tests when the platform piece has landed or we should merge them all into
> one bug (1463919) ?

For the front-end parts only you can simply add your prompt to this test https://searchfox.org/mozilla-central/source/browser/modules/test/browser/browser_PermissionUI_prompts.js. It will mock away the underlying platform caller. I would be okay with landing this patch with only that test and adding integration tests for the whole call chain once the platform pieces are ready.

Alternatively we could also wait with landing this until bug 1463919 is done, or request review for this patch in bug 1463919. All these options are fine by me :)
Comment on attachment 8980541 [details]
Bug 1461656 - Ask permission when site wants to autoplay media.

https://reviewboard.mozilla.org/r/246696/#review253952

Cancelling review for now, though this is generally the right way to do it :)

Feel free to either re-submit the fixed patch here or merge into bug 1463919.

::: browser/base/content/browser.xul:828
(Diff revision 1)
>                           tooltiptext="&urlbar.screenBlocked.tooltip;"/>
>                    <image data-permission-id="persistent-storage" class="blocked-permission-icon persistent-storage-icon" role="button"
>                           tooltiptext="&urlbar.persistentStorageBlocked.tooltip;"/>
>                    <image data-permission-id="popup" class="blocked-permission-icon popup-icon" role="button"
>                           tooltiptext="&urlbar.popupBlocked.tooltip;"/>
> +                  <image data-permission-id="autoplay-media" class="blocked-permission-icon autoplay-media-blocked-icon" role="button"

This should not have the autoplay-media-blocked-icon class, you can just use autoplay-media-icon and match for .autoplay-media-icon.blocked-permission-icon in your CSS.

::: browser/base/content/browser.xul:842
(Diff revision 1)
>                  <box id="notification-popup-box"
>                       hidden="true"
>                       onmouseover="document.getElementById('identity-box').classList.add('no-hover');"
>                       onmouseout="document.getElementById('identity-box').classList.remove('no-hover');"
>                       align="center">
>                    <image id="default-notification-icon" class="notification-anchor-icon" role="button"

You need to add the .autoplay-media-icon anchor in this container, too, like the other permission prompts do it.

::: browser/locales/en-US/chrome/browser/browser.properties:953
(Diff revision 1)
> +autoplay.Allow.label = Allow
> +autoplay.Allow.accesskey = A
> +autoplay.DontAllow.label = Don’t Allow
> +autoplay.DontAllow.accesskey = N
> +autoplay.remember=Remember this decision
> +autoplay.message = Will you allow %S to autoplay media with sound?

Just a thought that I'm not sure the average person understands the word "autoplay", but I might be wrong.

::: browser/locales/en-US/chrome/browser/sitePermissions.properties:31
(Diff revision 1)
>  state.multichoice.alwaysAsk = Always Ask
>  state.multichoice.allow = Allow
>  state.multichoice.allowForSession = Allow for Session
>  state.multichoice.block = Block
>  
> -permission.autoplay-media.label = Automatically Play Media with Sound
> +permission.autoplay-media.label = Allow media with sound

Are you sure that this change is correct? Isn't this still allowing media with sound in general, just not autoplaying?

In any case, if you really want to change this string you need to change it to e.g. permission.autoplay-media2.label

::: browser/modules/PermissionUI.jsm:787
(Diff revision 1)
> +  get anchorID() {
> +    return "autoplay-media-icon";
> +  },
> +
> +  get message() {
> +    return gBrowserBundle.formatStringFromName("autoplay.message", ["<>"], 1);

You might note that other permission prompts have separate copy for local file:// pages requesting this permission (since there's no hostname). I don't know whether that can be the case for this permission, but if it is then you should consider doing that, too.

::: browser/modules/SitePermissions.jsm:617
(Diff revision 1)
>          return SitePermissions.ALLOW;
>        }
> -      return SitePermissions.BLOCK;
> -    }
> +      return SitePermissions.UNKNOWN;
> +    },
> +    labelID: "autoplay-media",
> +    states: [ SitePermissions.UNKNOWN, SitePermissions.ALLOW, SitePermissions.BLOCK ],

I think you can remove this, this is the default set of states.

::: browser/themes/shared/notification-icons.inc.css:63
(Diff revision 1)
> +  list-style-image: url(chrome://browser/skin/notification-icons/autoplay-media.svg);
> +}
> +
> +.autoplay-media-blocked-icon {
> +  list-style-image: url(chrome://browser/skin/notification-icons/autoplay-media-blocked.svg);
> +}

Please avoid breaking up the related .geo-icon styles here (moves these two rules a little higher or lower).

::: browser/themes/shared/notification-icons/autoplay-media-blocked.svg:5
(Diff revision 1)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16" fill="context-fill" fill-opacity="context-fill-opacity">
> +    <g id="Glyph-/-Autoplay-Block" stroke="none" stroke-width="1" fill="none" fill-rule="evenodd">

nit: you can probably remove the ids in this file

::: browser/themes/shared/notification-icons/autoplay-media.svg:5
(Diff revision 1)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16" fill="context-fill" fill-opacity="context-fill-opacity">
> +  <g>

nit: do you need this <g>?
Attachment #8980541 - Flags: review?(jhofmann)
Yup Chris asked me to get the frontend parts landed + strings when I could so think its a good plans to get this in an intergration tests landing with the platform patch, cheers
Comment on attachment 8980541 [details]
Bug 1461656 - Ask permission when site wants to autoplay media.

https://reviewboard.mozilla.org/r/246696/#review254634

Looks great, thank you!

::: browser/modules/test/browser/browser_PermissionUI_prompts.js:35
(Diff revision 2)
>    await testPrompt(PermissionUI.MIDIPermissionPrompt);
>  });
>  
> +// Tests that AutoplayPermissionPrompt works as expected
> +add_task(async function test_autoplay_permission_prompt() {
> +  Services.prefs.setBoolPref("media.autoplay.enabled", false);

Are you sure you need this? I think we mock away literally all the permission parts and just test the prompt front-end.
Attachment #8980541 - Flags: review?(jhofmann) → review+
Yeh its definitely needed

    let TestPrompt = new Prompt(mockRequest);
    TestPrompt.prompt();

Calls to https://dxr.mozilla.org/mozilla-central/source/browser/modules/PermissionUI.jsm#246 which does a SitePermissions.get going back to autoplay-media getDefault() which needs the pref

Cheers, pushed to try (https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb3587e19431bdaa856f6785babcb5478b3cca55) and will get l10n review
Attachment #8980541 - Flags: review?(francesco.lodolo)
Comment on attachment 8980541 [details]
Bug 1461656 - Ask permission when site wants to autoplay media.

https://reviewboard.mozilla.org/r/246696/#review254666

::: browser/locales/en-US/chrome/browser/browser.dtd:214
(Diff revision 2)
>  
>  <!ENTITY urlbar.viewSiteInfo.label                      "View site information">
>  
>  <!ENTITY urlbar.defaultNotificationAnchor.tooltip         "Open message panel">
>  <!ENTITY urlbar.geolocationNotificationAnchor.tooltip     "Open location request panel">
> +<!ENTITY urlbar.autoplayNotificationAnchor.tooltip        "Manage this site automatically playing media with sound">

Does this come from UX approved copy? I skimmed through the bug, and couldn't find any information.

Asking since it's using "site" vs "website" (we use the latter), and all other strings seem to use "Manage" + noun (e.g. "Manage media with sound autoplay")

::: browser/locales/en-US/chrome/browser/browser.properties:954
(Diff revision 2)
>  midi.shareSysexWithSite.message = Will you allow %S to access your MIDI devices and send/receive SysEx messages?
>  
> +autoplay.Allow.label = Allow
> +autoplay.Allow.accesskey = A
> +autoplay.DontAllow.label = Don’t Allow
> +autoplay.DontAllow.accesskey = N

N->n

Accesskeys are case sensitive, even if they fall back to the different case if available.
> Does this come from UX approved copy? I skimmed through the bug, and couldn't find any information.

Nope, Bryant can you give us the text for this tooltip, it is the text that shows up when you hover over the autoplay icon in the url bar when a site is currently requesting to play media. Cheers
Flags: needinfo?(bmao)
Got new copy from Bryant and updated, Francesco I believe landing these strings is time sensitive as we would like to be running a shield study against them, so I know things are hectic today but if there is any chance of getting these done soon it would be great, thank you
Flags: needinfo?(bmao)
Comment on attachment 8980541 [details]
Bug 1461656 - Ask permission when site wants to autoplay media.

https://reviewboard.mozilla.org/r/246696/#review257534

::: browser/locales/en-US/chrome/browser/browser.properties:959
(Diff revision 3)
> +autoplay.Allow.label = Allow Autoplay
> +autoplay.Allow.accesskey = A
> +autoplay.DontAllow.label = Don’t Allow
> +autoplay.DontAllow.accesskey = n
> +autoplay.remember = Remember this decision
> +autoplay.message = Will you allow %S to autoplay media with sound?

Can you add a comment similar to midi.shareSysexWithSite.message, explaining what %S is?
Attachment #8980541 - Flags: review?(francesco.lodolo) → review+
Will do, cheers
Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f527be41b97d
Ask permission when site wants to autoplay media. r=flod,johannh
https://hg.mozilla.org/mozilla-central/rev/f527be41b97d
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Depends on: 1476561
Depends on: 1477231
Depends on: 1483513
Depends on: 1482646
You need to log in before you can comment on or make changes to this bug.