1458249 - Add autoplay item to about:preferences

Status: RESOLVED FIXED

(Firefox :: Settings UI, enhancement, P1)

Description

[Dale Harvey (:daleharvey)]

As seen in https://mozilla.invisionapp.com/share/N8F2K0I93#/screens/287176983

Comment 1

[Dale Harvey (:daleharvey)]

Attachment: Bug 1458249 - Add autoplay item to about:preferences.

Review commit: https://reviewboard.mozilla.org/r/242336/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242336/

Comment 2

[Dale Harvey (:daleharvey)]

Comment on attachment 8973997 [details]
Bug 1458249 - Add autoplay item to about:preferences.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242336/diff/1-2/

Comment 3

[Dale Harvey (:daleharvey)]

Comment on attachment 8973997 [details]
Bug 1458249 - Add autoplay item to about:preferences.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242336/diff/2-3/

Comment 4

[Johann Hofmann [:johannh]]

Comment on attachment 8973997 [details]
Bug 1458249 - Add autoplay item to about:preferences.

https://reviewboard.mozilla.org/r/242336/#review248436

::: browser/components/preferences/in-content/privacy.xul
(Diff revision 3)
>      <checkbox id="popupPolicy" preference="dom.disable_open_during_load"
>                data-l10n-id="permissions-block-popups"
>                onsyncfrompreference="return gPrivacyPane.updateButtons('popupPolicyButton',
>                                           'dom.disable_open_during_load');"
>                flex="1" />
> -    <!-- Please don't remove the wrapping hbox/vbox/box for these elements. It's used to properly compute the search tooltip position. -->

Drive-by: please keep this comment

::: browser/locales/en-US/chrome/browser/preferences/preferences.properties:24
(Diff revision 3)
>  acceptVeryLargeMinimumFont=Keep my changes anyway
>  
> +#### Permissions Manager
> +
> +audiableautoplaytitle2=Allowed Websites - Autoplay
> +audiableautoplaytext=You can specify which websites are allowed to open pop-up windows. Type the exact address of the site you want to allow and then click Allow.

Drive-by:

Please use fluent for this, an example how to do it is https://searchfox.org/mozilla-central/search?q=permissions-exceptions-tracking-protection-window&case=false&regexp=false&path=

Comment 5

[Dale Harvey (:daleharvey)]

Comment on attachment 8973997 [details]
Bug 1458249 - Add autoplay item to about:preferences.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242336/diff/3-4/

Comment 6

[Dale Harvey (:daleharvey)]

> Drive-by: please keep this comment

I moved the comment to the row I newly added to the top, it seemed like the comment applied to all rows (I added a wrapping hbox as well) so figured it should be at the top row, you want me to put it back in so its in the top 2 rows or fine as is?

> Please use fluent for this

Yeh was following the rest of the prefs implementation and they just all changed on inbound, rebased and switched to ftl

Comment 7

[Dale Harvey (:daleharvey)]

So this uses |media.autoplay.enabled.user-gestures-needed| to control whether we show the new controls in about:preferences, the switch controls |media.autoplay.enabled|

Bryant I am not certain about the wording here, specifically "Block website to play media with sound" seems like "Block website from playing media with sound" is more natural, and "You can specify which websites are allowed to autoplay media element" the media element seems wrong here

Cheers for the drive bys Johann, I think with the text fixed mostly need to add tests for this then will flag you up

Comment 8

[Johann Hofmann [:johannh]]

(In reply to Dale Harvey (:daleharvey) from comment #6)
> > Drive-by: please keep this comment
> 
> I moved the comment to the row I newly added to the top, it seemed like the
> comment applied to all rows (I added a wrapping hbox as well) so figured it
> should be at the top row, you want me to put it back in so its in the top 2
> rows or fine as is?

Yeah, I think we generally try to have this sort of comment in front of every <hbox> that's necessary for search.

https://searchfox.org/mozilla-central/search?q=%3C!--%20Please%20don%27t%20remove%20the%20wrapping%20hbox%2Fvbox%2Fbox%20for%20these%20elements.%20It%27s%20used%20to%20properly%20compute%20the%20search%20tooltip%20position.%20--%3E&case=false&regexp=false&path=

Not very pretty, someone should try to come up with a better solution to this :|

Thanks! :)

Comment 9

[Dale Harvey (:daleharvey)]

Comment on attachment 8973997 [details]
Bug 1458249 - Add autoplay item to about:preferences.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242336/diff/4-5/

Comment 10

[Mike Conley (:mconley) (:⚙️)]

This looks like active work on an active project, so marking P1 for now.

Comment 11

[Dale Harvey (:daleharvey)]

Comment on attachment 8973997 [details]
Bug 1458249 - Add autoplay item to about:preferences.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242336/diff/5-6/

Comment 12

[Bryant Mao [:bryantmao]]

(In reply to Dale Harvey (:daleharvey) from comment #7)
> So this uses |media.autoplay.enabled.user-gestures-needed| to control
> whether we show the new controls in about:preferences, the switch controls
> |media.autoplay.enabled|
> 
> Bryant I am not certain about the wording here, specifically "Block website
> to play media with sound" seems like "Block website from playing media with
> sound" is more natural, and "You can specify which websites are allowed to
> autoplay media element" the media element seems wrong here

Good points, the spec is now under review by the copywriter, so some wording might change afterward. The word 'from' seems fit better here, and maybe we should highlight the 'auto' by restating it as 'Block autoplay media with sound' since we still allow the media to play if user gesture activates the document or just click the play button. 

For exception panel, we can say "You can specify which websites are allowed to autoplay media with sound", which are much aligned with the wording we used in the checkbox. But I also have some concern by using the word 'autoplay' as it might not be a common term for the general audience.

I'll communicate those ideas with the copywriter. Once we nail it down, I'll reflect the changes in the spec and the bug here.

Comment 13

[Dale Harvey (:daleharvey)]

Comment on attachment 8973997 [details]
Bug 1458249 - Add autoplay item to about:preferences.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242336/diff/6-7/

Comment 14

[Johann Hofmann [:johannh]]

Comment on attachment 8973997 [details]
Bug 1458249 - Add autoplay item to about:preferences.

https://reviewboard.mozilla.org/r/242336/#review252224

This seems pretty good already, not an r+ yet because I'd like to take another look at the next patch :)

I also assume you'll figure out the right copy in time for this landing.

::: browser/components/preferences/in-content/privacy.js:283
(Diff revision 7)
>        gPrivacyPane.trackingProtectionReadPrefs.bind(gPrivacyPane));
>      Preferences.get("privacy.trackingprotection.pbmode.enabled").on("change",
>        gPrivacyPane.trackingProtectionReadPrefs.bind(gPrivacyPane));
> +    Preferences.get("media.autoplay.enabled").on("change",
> +     gPrivacyPane.updateAutoplayMediaControls.bind(gPrivacyPane));
> +    Preferences.get(AUTOPLAYMEDIA_KEY).on("change",

I'm not really sure why you have a constant for "media.autoplay.enabled.user-gestures-needed" and no constant for "media.autoplay.enabled". I think I don't mind either way but it looks a little inconsistent like this...

::: browser/components/preferences/in-content/privacy.xul:510
(Diff revision 7)
> +      <button id="autoplayMediaPolicyButton"
> +              class="accessory-button"
> +              data-l10n-id="permissions-block-autoplay-media-exceptions"
> +              search-l10n-ids="permissions-address,
> +                               permissions-button-cancel.label,
> +                               permissions-button-ok.label

You should add permissions-exceptions-autoplay-media-window.title and permissions-exceptions-autoplay-media-desc as search ids.

::: browser/components/preferences/in-content/privacy.xul
(Diff revision 7)
>      <checkbox id="popupPolicy" preference="dom.disable_open_during_load"
>                data-l10n-id="permissions-block-popups"
>                onsyncfrompreference="return gPrivacyPane.updateButtons('popupPolicyButton',
>                                           'dom.disable_open_during_load');"
>                flex="1" />
> -    <!-- Please don't remove the wrapping hbox/vbox/box for these elements. It's used to properly compute the search tooltip position. -->

I thought we wanted to keep this comment? :)

::: browser/components/preferences/in-content/tests/browser_site_autoplay_media_exceptions.js:11
(Diff revision 7)
> +
> +var exceptionsDialog;
> +
> +// Store current pref values so we can restore them
> +var autoplayEnabled = Services.prefs.getBoolPref(AUTOPLAY_ENABLED_KEY);
> +var gesturesNeeded = Services.prefs.getBoolPref(GESTURES_NEEDED_KEY);

you can restore these values with Services.prefs.clearUserPref

::: browser/components/preferences/in-content/tests/browser_site_autoplay_media_exceptions.js:14
(Diff revision 7)
> +// Store current pref values so we can restore them
> +var autoplayEnabled = Services.prefs.getBoolPref(AUTOPLAY_ENABLED_KEY);
> +var gesturesNeeded = Services.prefs.getBoolPref(GESTURES_NEEDED_KEY);
> +
> +Services.prefs.setBoolPref(AUTOPLAY_ENABLED_KEY, true);
> +Services.prefs.setBoolPref(GESTURES_NEEDED_KEY, true);

Can you please add a short test that verifies that the section is hidden without this pref?

::: browser/components/preferences/in-content/tests/browser_site_autoplay_media_exceptions.js:23
(Diff revision 7)
> +  registerCleanupFunction(async function() {
> +    await ContentTask.spawn(gBrowser.selectedBrowser, null, function() {
> +      let doc = content.document;
> +      let autoplayCheckBox = doc.getElementById("autoplayMediaPolicy");
> +      if (autoplayCheckBox.checked) {
> +        autoplayCheckBox.click();

This shouldn't be necessary if you reset the prefs.

::: browser/components/preferences/in-content/tests/browser_site_autoplay_media_exceptions.js:40
(Diff revision 7)
> +  let dialogOpened = promiseLoadSubDialog(PERMISSIONS_URL);
> +
> +  await ContentTask.spawn(gBrowser.selectedBrowser, null, function() {
> +    let doc = content.document;
> +    let autoplayCheckBox = doc.getElementById("autoplayMediaPolicy");
> +    autoplayCheckBox.click();

Can you please assert that after this the autoplay pref is set to false?

::: browser/components/preferences/in-content/tests/browser_site_autoplay_media_exceptions.js:66
(Diff revision 7)
> +  btnBlock.click();
> +
> +  await TestUtils.waitForCondition(() => tree.view.rowCount == 1);
> +
> +  Assert.equal(tree.view.getCellText(0, tree.treeBoxObject.columns.getColumnAt(0)),
> +               "http://www.example.com");

Again, here you're only asserting that the UI works. It would be great if you could check that the correct permission was set in the permission manager.

::: browser/components/preferences/in-content/tests/browser_site_autoplay_media_exceptions.js:83
(Diff revision 7)
> +    EventUtils.synthesizeKey("KEY_Backspace");
> +  } else {
> +    EventUtils.synthesizeKey("KEY_Delete");
> +  }
> +
> +  await TestUtils.waitForCondition(() => tree.view.rowCount == 0);

Please assert that the permission was removed.

::: browser/locales/en-US/browser/preferences/permissions.ftl:104
(Diff revision 7)
> +## Exceptions - Autoplay Media
> +
> +permissions-exceptions-autoplay-media-window =
> +    .title = Allowed Websites - Autoplay
> +    .style = { permissions-window.style }
> +permissions-exceptions-autoplay-media-desc = You can specify which websites are allowed to autoplay media element. Type the exact address of the site you want to allow and then click Allow.

My suggestion would be

"You can specify which websites are allowed to automatically play media elements."

::: browser/locales/en-US/browser/preferences/preferences.ftl:849
(Diff revision 7)
>  permissions-notification-pause =
>      .label = Pause notifications until { -brand-short-name } restarts
>      .accesskey = n
>  
> +permissions-block-autoplay-media =
> +    .label = Block website to play media with sound

I'd suggest "Block websites from automatically playing media with sound" although that might get a little long...

Comment 15

[Dale Harvey (:daleharvey)]

Comment on attachment 8973997 [details]
Bug 1458249 - Add autoplay item to about:preferences.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242336/diff/7-8/

Comment 16

[Dale Harvey (:daleharvey)]

Hey Johann, got most of the changes done but running into an issue in the test, you mentioned I should assert that the permission gets added and in the diff I added:

  let btnApplyChanges = doc.getElementById("btnApplyChanges");
  btnApplyChanges.click();
  await waitForPermChanged;

  is(SitePermissions.get(URL, "autoplay-media").state, SitePermissions.ALLOW,
     "Correctly added the exception");

only it keeps failing with SitePermissions.BLOCK, I have tried setTimeout etc but the permission never seems to get set, the perm-changed event gets fired and if I pause the test from running after the .click I can see the permission being shown in the UI + see it having an effect if I go to an autoplaying url, is there something I am missing here? Thanks (full diff @ https://reviewboard.mozilla.org/r/242334/diff/8)

Comment 17

[Dale Harvey (:daleharvey)]

ok figured this out, cheers

Comment 18

[Dale Harvey (:daleharvey)]

Comment on attachment 8973997 [details]
Bug 1458249 - Add autoplay item to about:preferences.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242336/diff/8-9/

Comment 19

[Johann Hofmann [:johannh]]

Comment on attachment 8973997 [details]
Bug 1458249 - Add autoplay item to about:preferences.

https://reviewboard.mozilla.org/r/242336/#review253984

This looks good, thank you!

::: browser/components/preferences/in-content/tests/browser_site_autoplay_media_exceptions.js:27
(Diff revision 9)
> +    exceptionsButton.click();
> +  });
> +  exceptionsDialog = await dialogOpened;
> +}
> +
> +async function waitForPermChanged() {

You don't need this function, there's a utility function you can use instead:

await TestUtils.topicObserved("perm-changed");

::: browser/components/preferences/in-content/tests/browser_site_autoplay_media_exceptions.js:69
(Diff revision 9)
> +
> +  Assert.equal(Services.prefs.getBoolPref(AUTOPLAY_ENABLED_KEY), false,
> +               "Ensure we have set autoplay to false");
> +});
> +
> +const timeout = ms => new Promise(res => setTimeout(res, ms))

you can probably remove this

::: browser/components/preferences/in-content/tests/browser_site_autoplay_media_exceptions.js:124
(Diff revision 9)
> +  let btnApplyChanges = doc.getElementById("btnApplyChanges");
> +  btnApplyChanges.click();
> +  await waitForPermChanged;
> +
> +  is(Services.perms.testPermissionFromPrincipal(PRINCIPAL, "autoplay-media"),
> +     Ci.nsIPermissionManager.UNKNOWN_ACTION, "Correctly added the exception");

nit: correctly removed the exception

Comment 20

[Johann Hofmann [:johannh]]

Comment on attachment 8973997 [details]
Bug 1458249 - Add autoplay item to about:preferences.

https://reviewboard.mozilla.org/r/242336/#review253986

::: browser/locales/en-US/browser/preferences/preferences.ftl:850
(Diff revision 9)
>      .label = Pause notifications until { -brand-short-name } restarts
>      .accesskey = n
>  
> +permissions-block-autoplay-media =
> +    .label = Block websites from automatically playing media with sound
> +    .accesskey = A

another note: this may not be the most appropriate accesskey anymore (doesn't occur in the label)

Comment 21

[Dale Harvey (:daleharvey)]

Comment on attachment 8973997 [details]
Bug 1458249 - Add autoplay item to about:preferences.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242336/diff/9-10/

Comment 22

[Dale Harvey (:daleharvey)]

> another note: this may not be the most appropriate accesskey anymore (doesn't occur in the label)

All the following accesskeys use the first letter of the label so followed that pattern

Cheers for the review, pushed to try @ https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5dedbd6ae629404945d4f6c8ae44bc34e93ae91 before landing

Comment 23

[Mozilla Autoland]

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 7 changes to 7 files
remote: (ftl_check check enabled per config override)
remote: 
remote: ************************ ERROR *************************
remote: You are trying to commit a change to an FTL file.
remote: At the moment modifying FTL files requires a review from
remote: one of the L10n Drivers.
remote: Please, request review from either:
remote:     - Francesco Lodolo (:flod)
remote:     - Zibi Braniecki (:gandalf)
remote:     - Axel Hecht (:pike)
remote:     - Stas Malolepszy (:stas)
remote: ********************************************************
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.mozhooks hook failed
abort: push failed on remote

Comment 24

[Dale Harvey (:daleharvey)]

Comment on attachment 8973997 [details]
Bug 1458249 - Add autoplay item to about:preferences.

Hey, looks like hg flagged up I need an l10n review for this, cheers

Comment 25

[Francesco Lodolo [:flod]]

Comment on attachment 8973997 [details]
Bug 1458249 - Add autoplay item to about:preferences.

https://reviewboard.mozilla.org/r/242336/#review254328

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6fdcdac9cd74
Add autoplay item to about:preferences. r=flod,johannh

Comment 27

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/6fdcdac9cd74