Closed Bug 1458249 Opened 6 years ago Closed 6 years ago

Add autoplay item to about:preferences

Categories

(Firefox :: Settings UI, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: daleharvey, Assigned: daleharvey)

References

Details

Attachments

(1 file)

Assignee: nobody → dharvey
Component: General → Site Identity and Permission Panels
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=
Component: Site Identity and Permission Panels → Preferences
> 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
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
Flags: needinfo?(bmao)
(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! :)
This looks like active work on an active project, so marking P1 for now.
Priority: -- → P1
(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.
Flags: needinfo?(bmao)
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...
Attachment #8973997 - Flags: review?(jhofmann) → review-
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)
Flags: needinfo?(jhofmann)
ok figured this out, cheers
Flags: needinfo?(jhofmann)
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
Attachment #8973997 - Flags: review?(jhofmann) → review+
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)
> 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
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 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
Attachment #8973997 - Flags: review?(francesco.lodolo)
Comment on attachment 8973997 [details]
Bug 1458249 - Add autoplay item to about:preferences.

https://reviewboard.mozilla.org/r/242336/#review254328
Attachment #8973997 - Flags: review?(francesco.lodolo) → review+
Pushed by francesco.lodolo@mozillaitalia.org:
https://hg.mozilla.org/integration/autoland/rev/6fdcdac9cd74
Add autoplay item to about:preferences. r=flod,johannh
https://hg.mozilla.org/mozilla-central/rev/6fdcdac9cd74
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: