STR: 1. Open http://pearce.org.nz/video/h264.html 2. A door hanger asking for permission to autoplay appears 3. Click on the play button in the video The video starts playing, but note that the doorhanger still shows 4. Press ESC key. This dismiss the door hanger Now reload the page, notice that no door hanger appear. It turns out that autoplay is now marked as blocked for this page. ESC shouldn't validate the door hanger with "block", especially when the default is to allow it. I would have expected the prompt to appear again, not for autoplay to be blocked.

:cpearce, do you know someone who could take a look at this? Does this fall into the UX side of things?

I'm looking into issues regarding the permission prompts for autoplay at the moment. I'll look into this.

So with bug 1471485 and bug 1472580 we'll hide the doorhanger prompt when we start playing video. So our behaviour with the patches there is upon ESC press the front end code effectively presses the secondary (cancel) button: https://searchfox.org/mozilla-central/rev/b0275bc977ad7fda615ef34b822bba938f2b16fd/toolkit/modules/PopupNotifications.jsm#235 Since we have the "remember" checkbox checked by default, this will store persistent a "block" permission. So the question here is: should we store a persistent block permission on ESC key press, or should it be temporary? I don't think there's currently a way to distinguish between these different ways to reject requests. We could add one. We could also change to not have the "remember" checkbox checked by default. That's something that hopefully the shield study will help us figure out what is the best default.

That's how all our doorhangers work, ESC is the same thing as pressing deny. I'm really hesitant to make our doorhanger behavior more inconsistent here. I can understand that you associate ESC with temporary deny, but are you sure that everyone thinks that way? I find ESC == deny button an easier association to make than figuring out the internal logic you want to apply here. You as a Firefox developer are familiar with the prompt, so you press ESC without reading it, but anyone who will get this prompt less than a few times per day will read it and notice that the "Remember this decision" box is checked. How do you explain ignoring that checkbox without linking to the bug that explains the rationale? I can see the opposite bug report coming in about "Remember this decision" not working when pressing ESC, if we go this route. Even if most people expect this behavior, you're solving your own use case but you're making things worse for everyone else, because now users have different behavior for different prompts. Generally, this is why we show the crossed-out icon in the identity block, to make sure users can notice and revert their decision. To me what you describe actually sounds like you really want to uncheck the checkbox by default or do what the "Notifications" prompt does, offering buttons for permanent allow and temporary deny (with a dropdown option to permanently deny). You can try it on https://permission.site/. I don't like this option much either because it requires users to differentiate between two different types of prompts, but at least we're not lying to the user in that case.

(In reply to Jean-Yves Avenard [:jya] from comment #0) > STR: > 1. Open http://pearce.org.nz/video/h264.html > 2. A door hanger asking for permission to autoplay appears > 3. Click on the play button in the video > > The video starts playing, but note that the doorhanger still shows I can't reproduce this in my Nightly. The doorhanger hides for me. > 4. Press ESC key. This dismiss the door hanger > > Now reload the page, notice that no door hanger appear. It turns out that > autoplay is now marked as blocked for this page. > > ESC shouldn't validate the door hanger with "block", especially when the > default is to allow it. > > I would have expected the prompt to appear again, not for autoplay to be > blocked. I don't understand your issue. You solved your problem, right? With hitting the ESC key you made the prompt never show again, but you need to press "play" on the video every time. If we landed this patch what you would get is that you would have to both dismiss the prompt AND press play every time you load the page. That doesn't sound like an improvement to me. The solution for you is to actually read the prompt and make a decision on whether you want to allow or disallow autoplay on the site, because it sounds like you really wanted to press "allow". Maybe having the checkbox checked by default is actually the best option after all.

(In reply to Johann Hofmann [:johannh] from comment #6) > (In reply to Jean-Yves Avenard [:jya] from comment #0) > > STR: > > 1. Open http://pearce.org.nz/video/h264.html > > 2. A door hanger asking for permission to autoplay appears > > 3. Click on the play button in the video > > > > The video starts playing, but note that the doorhanger still shows Because that was another bug, fixed in bug 1474430 > > > I would have expected the prompt to appear again, not for autoplay to be > > blocked. > > I don't understand your issue. You solved your problem, right? With hitting > the ESC key you made the prompt never show again, but you need to press > "play" on the video every time. If we landed this patch what you would get > is that you would have to both dismiss the prompt AND press play every time > you load the page. That doesn't sound like an improvement to me. Here the issue isn't that the doorhanger will disappear, but that no video will ever autoplay again on that site, it's the permanent bit that's a problem. I can't think of any user ever thinking that ESC doesn't mean cancel/ignore the current action and that ESC should behave here the same as return. If the user did read the doc, he/she would have validated the default doorhanger choice with ENTER, and that would have made it permanent. Or manually selected to always allow to play. > > The solution for you is to actually read the prompt and make a decision on > whether you want to allow or disallow autoplay on the site, because it > sounds like you really wanted to press "allow". No user ever reads the doc :) > > Maybe having the checkbox checked by default is actually the best option > after all. And short testing on users proved otherwise. In any case, our team decided that ESC wasn't to make the change permanent but temporary.

(In reply to Jean-Yves Avenard [:jya] from comment #7) > (In reply to Johann Hofmann [:johannh] from comment #6) > > (In reply to Jean-Yves Avenard [:jya] from comment #0) > > > STR: > > > 1. Open http://pearce.org.nz/video/h264.html > > > 2. A door hanger asking for permission to autoplay appears > > > 3. Click on the play button in the video > > > > > > The video starts playing, but note that the doorhanger still shows > > Because that was another bug, fixed in bug 1474430 Ok, that probably makes people much less likely to hit the case we're talking about here. > Here the issue isn't that the doorhanger will disappear, but that no video > will ever autoplay again on that site, it's the permanent bit that's a > problem. > I can't think of any user ever thinking that ESC doesn't mean cancel/ignore > the current action and that ESC should behave here the same as return. Well, that's how all of our prompts work. I'm happy to talk about this as a general concept, but what I'm trying to prevent is individual teams looking to solve only their individual concerns making the experience inconsistent for everyone else. > If the user did read the doc, he/she would have validated the default > doorhanger choice with ENTER, and that would have made it permanent. > Or manually selected to always allow to play. I don't understand this. Are you assuming that users never (or rarely) make the educated choice to always block autoplay? Then, as I mentioned, you should instead reconsider your default choices for this prompt. > > The solution for you is to actually read the prompt and make a decision on > > whether you want to allow or disallow autoplay on the site, because it > > sounds like you really wanted to press "allow". > > No user ever reads the doc :) It's not a doc it's two sentences in a prompt and if your assumption is that literally all your users will try to make this prompt disappear without reading it you probably shouldn't ship a prompt. > > Maybe having the checkbox checked by default is actually the best option > > after all. > > And short testing on users proved otherwise. Your user testing shows that the checkbox should not be checked by default? Then why are we having this discussion and don't just uncheck the checkbox? As I mentioned, I can totally understand that you and your team associate ESC with "make this go away", but I don't think you're approaching this problem from the right angle. It's very likely your users are predominantly not using the keyboard to answer prompts, and if you are afraid that users will suffer from the permanent blocking, it's a good idea to reconsider your defaults.

Thanks for your input here Johann. (In reply to Johann Hofmann [:johannh] from comment #8) > Well, that's how all of our prompts work. I'm happy to talk about this as a > general concept, but what I'm trying to prevent is individual teams looking > to solve only their individual concerns making the experience inconsistent > for everyone else. I agree that there's value in keeping the behaviour of doorhangers consistent. So I'm happy to talk about the general case. :) I personally think pressing ESC signifies a desire to "escape" without doing any permanent damage (to your permission settings), and so I still think it makes sense to not remember a permission on ESC key press. Based on some spelunking through searchfox, it seems persistent-storage has a checkbox checked by default, and the WebRTC permissions, geolocation, "click-to-play-plugins, have checkboxes unchecked by default. Form autofill uses PopupNotifications too and can have its checkbox checked depending users' settings. So I think if we're talking about the general case, there are three existing users of PopupNotifications who have a remember checkbox for whom pressing ESC could remember a permanent setting. > I don't understand this. Are you assuming that users never (or rarely) make > the educated choice to always block autoplay? Then, as I mentioned, you > should instead reconsider your default choices for this prompt. FWIW, I was told by our data science team that our existing doorhangers have an interaction rate of something like 5%. > It's not a doc it's two sentences in a prompt and if your assumption is that > literally all your users will try to make this prompt disappear without > reading it you probably shouldn't ship a prompt. > > > > Maybe having the checkbox checked by default is actually the best option > > > after all. > > > > And short testing on users proved otherwise. > > Your user testing shows that the checkbox should not be checked by default? The user testing jya mentioned was our UX guy asking several people in his office what they thought pressing ESC should do, plus the opinions of the people involved in the feature, so it's hardly a robust study. We're planning to do a shield study on the defaults we show on the doorhanger, which includes testing whether the "remember" checkbox is checked by default. > Then why are we having this discussion and don't just uncheck the checkbox? We feel that the block autoplay prompt is a bit different to other prompts in that we're not adding a prompt to activate features which doesn't yet exist in the web platform, we're adding a prompt to take said features away. We want the remember checkbox default checked because we feel that media playback will be far more common than other things that use the doorhanger prompts, so we want to reduce the annoyance of frequently being prompted (at least until the user has built up their own personal whitelist of sites trusted to autoplay). > > As I mentioned, I can totally understand that you and your team associate > ESC with "make this go away", but I don't think you're approaching this > problem from the right angle. It's very likely your users are predominantly > not using the keyboard to answer prompts, and if you are afraid that users > will suffer from the permanent blocking, it's a good idea to reconsider your > defaults. You're probably right about most users are not using the keyboard. We, being Gecko devs, don't normally come up into the realm of how things interact with real users, so I'm curious what approach you think we should take here? We're already planning a shield study on the doorhanger. What would have to believe in order to be happy to have pressing ESC always call the secondary button action with the checkbox reported as unchecked for all users of the PopupNotifications API?

Comment on attachment 8992775 [details] Bug 1473671 - Don't store persistent block permission if ESC pressed while showing permission doorhanger. https://reviewboard.mozilla.org/r/257626/#review264580 I'm going on PTO now and it's probably a good idea to have someone else review this. I might be a bit too pedantic here. In the end this is quite a small detail and it could be fine to just go with this patch. I won't mind either way :) My recommendation is still: - Wait for the results of the study before introducing this flag. - Try using the same doorhanger style as the "Notifications" prompt - Consider that the majority of your users will use the doorhanger with a mouse. Florian, what do you think?

Adding my two cents here using this feature for a few days now: I wanted to test something regarding the auto-play permission and had to reload the page a few times removing the permission each time. As I at some point didn't want to have to interact with the doorhanger, I just hit esc and expected to have no change of the base behaviour of the page I was currently visiting, which was videos playing automatically after the player finished loading. 'Esc' for me implies, whatever the current default of the page is, remains true.

Comment on attachment 8992775 [details] Bug 1473671 - Don't store persistent block permission if ESC pressed while showing permission doorhanger. https://reviewboard.mozilla.org/r/257626/#review266436 I agree that saving persistent deny permission when the user hits escape is unexpected from a user point of view. I dislike that the current patch adds a way to make the PopupNotifications API lie to the action callback about the state of the checkbox. Suggestions: - change the "checkboxChecked" field name to something like "shouldRemember". This only works if we are confident that the checkbox label will always say "Remember this decision". I don't think this is guaranteed by the API, as the checkbox label is supplied by the consummer. - alternatively, we could add another field in the object given to the action callback. This new field could indicate the source of the event that triggered the action, and the action callback could use that field + the state of the checkbox to make a decision about whether something should be saved persistently. These are just suggestions, I'm open to discussion.

Comment on attachment 8992775 [details] Bug 1473671 - Don't store persistent block permission if ESC pressed while showing permission doorhanger. https://reviewboard.mozilla.org/r/257626/#review266560 Code analysis found 1 defect in this patch: - 1 defect 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 ::: toolkit/modules/PopupNotifications.jsm:1549 (Diff revision 2) > > if (action) { > try { > action.callback.call(undefined, { > - checkboxChecked: notificationEl.checkbox.checked > + checkboxChecked: notificationEl.checkbox.checked, > + source: source, Error: Expected property shorthand. [eslint: object-shorthand]

Comment on attachment 8992775 [details] Bug 1473671 - Don't store persistent block permission if ESC pressed while showing permission doorhanger. https://reviewboard.mozilla.org/r/257626/#review266616 Thanks for updating the patch, I think this is going in a reasonable direction (would be f+ on bugzilla). ::: browser/base/content/test/popupNotifications/browser_popupNotification_keyboard.js:28 (Diff revision 3) > var tests = [ > // Test that for persistent notifications, > // the secondary action is triggered by pressing the escape key. > { id: "Test#1", > - run() { > + async run() { > + await waitForWindowReadyForPopupNotifications(window); Why do you need to add this everywhere? The comment about this function says it's needed after opening a new window or switching the window focus: https://searchfox.org/mozilla-central/rev/943a6cf31a96eb439db8f241ab4df25c14003bb8/browser/base/content/test/popupNotifications/head.js#6-10 ::: browser/base/content/test/popupNotifications/browser_popupNotification_keyboard.js:227 (Diff revision 3) > + // called with a source of "esc-press". > + { id: "Test#7", > + async run() { > + await waitForWindowReadyForPopupNotifications(window); > + this.checkboxChecked = null; > + this.source = null; These 2 lines don't seem useful, and there's quite a bit of commented out dead code in this test. ::: browser/base/content/test/popupNotifications/browser_popupNotification_keyboard.js:259 (Diff revision 3) > + onHidden(popup) { > + ok(!this.notifyObj.mainActionClicked, "mainAction was not clicked"); > + ok(this.notifyObj.secondaryActionClicked, "secondaryAction was clicked"); > + ok(!this.notifyObj.dismissalCallbackTriggered, "dismissal callback not triggered"); > + ok(this.notifyObj.removedCallbackTriggered, "removed callback was triggered"); > + is(this.source, "esc-press", "Checkbox should have been reported as unchecked in the secondary callback"); To have good testing coverage, you need to modify at least one of the existing tests to verify that we are not just reporting source == esc-press all the time. ::: browser/modules/PermissionUI.jsm:315 (Diff revision 3) > if (promptAction.callback) { > promptAction.callback(); > } > > if (this.permissionKey) { > - > + if (state.source !== "esc-press") { By putting this test here rather than next to the checkboxChecked test, you are also preventing escape from saving a temporary block. Is this intentional? I think it means the prompt could re-appear almost immediately. If I remember correctly, temporary blocks are cleared when the page is reloaded by the user. nit: '!=' rather than '!==' in the browser and toolkit coding style. ::: browser/modules/PermissionUI.jsm:317 (Diff revision 3) > > if (this.permissionKey) { > - > + if (state.source !== "esc-press") { > - // Permanently store permission. > - if ((state && state.checkboxChecked) || > + if ((state && state.checkboxChecked) || > - promptAction.scope == SitePermissions.SCOPE_PERSISTENT) { > + promptAction.scope == SitePermissions.SCOPE_PERSISTENT) { broken indent on this line. ::: toolkit/modules/PopupNotifications.jsm:816 (Diff revision 3) > > popupnotification.setAttribute("id", popupnotificationID); > popupnotification.setAttribute("popupid", n.id); > popupnotification.setAttribute("oncommand", "PopupNotifications._onCommand(event);"); > if (Services.prefs.getBoolPref("privacy.permissionPrompts.showCloseButton")) { > - popupnotification.setAttribute("closebuttoncommand", "PopupNotifications._onButtonEvent(event, 'secondarybuttoncommand');"); > + popupnotification.setAttribute("closebuttoncommand", "PopupNotifications._onButtonEvent(event, 'secondarybuttoncommand', 'button');"); I suspect pressing the 'X' button is the same as pressing the escape key from a user point of view. ::: toolkit/modules/PopupNotifications.jsm:1484 (Diff revision 3) > notificationEl.ownerDocument.getBindingParent(notificationEl) || notificationEl.parentNode; > } > this._setNotificationUIState(notificationEl); > }, > > - _onButtonEvent(event, type, notificationEl = null) { > + _onButtonEvent(event, type, source = "unspecified", notificationEl = null) { When will the source be 'unspecified'? I wonder if the _onButtonEvent method could figure out the source automatically based on the already provided event object.

Comment on attachment 8992775 [details] Bug 1473671 - Don't store persistent block permission if ESC pressed while showing permission doorhanger. https://reviewboard.mozilla.org/r/257626/#review266824 ::: browser/base/content/test/popupNotifications/browser_popupNotification_keyboard.js:28 (Diff revision 3) > var tests = [ > // Test that for persistent notifications, > // the secondary action is triggered by pressing the escape key. > { id: "Test#1", > - run() { > + async run() { > + await waitForWindowReadyForPopupNotifications(window); I had thought I needed this to fix existing intermittent test failures, but I'm not seeing them now, so I can undo these changes. ::: browser/base/content/test/popupNotifications/browser_popupNotification_keyboard.js:227 (Diff revision 3) > + // called with a source of "esc-press". > + { id: "Test#7", > + async run() { > + await waitForWindowReadyForPopupNotifications(window); > + this.checkboxChecked = null; > + this.source = null; Ah oops, I had cleaned this up locally but forgot to hit save before amending and pushing for review. d'oh! Sorry about that. ::: browser/modules/PermissionUI.jsm:315 (Diff revision 3) > if (promptAction.callback) { > promptAction.callback(); > } > > if (this.permissionKey) { > - > + if (state.source !== "esc-press") { Remembering a temporary block here seems reasonable, so I'll move the test to next to the checkboxChecked test. ::: toolkit/modules/PopupNotifications.jsm:816 (Diff revision 3) > > popupnotification.setAttribute("id", popupnotificationID); > popupnotification.setAttribute("popupid", n.id); > popupnotification.setAttribute("oncommand", "PopupNotifications._onCommand(event);"); > if (Services.prefs.getBoolPref("privacy.permissionPrompts.showCloseButton")) { > - popupnotification.setAttribute("closebuttoncommand", "PopupNotifications._onButtonEvent(event, 'secondarybuttoncommand');"); > + popupnotification.setAttribute("closebuttoncommand", "PopupNotifications._onButtonEvent(event, 'secondarybuttoncommand', 'button');"); Agreed, I'll make this report as an esc-press. ::: toolkit/modules/PopupNotifications.jsm:1484 (Diff revision 3) > notificationEl.ownerDocument.getBindingParent(notificationEl) || notificationEl.parentNode; > } > this._setNotificationUIState(notificationEl); > }, > > - _onButtonEvent(event, type, notificationEl = null) { > + _onButtonEvent(event, type, source = "unspecified", notificationEl = null) { The source shouldn't really ever be "unspecified" unless we missed a case... Should we throw if the source isn't specified? It's not obvious to me how to distinguish between a close button click and clicks on the main/secondary button. So it seems easier to just leave it with the code that sets the event handler to opt into what source it reports.

Comment on attachment 8992775 [details] Bug 1473671 - Don't store persistent block permission if ESC pressed while showing permission doorhanger. https://reviewboard.mozilla.org/r/257626/#review266944 Looks good to me. But feel free to re-request review (you'll need to set the flag by hand in bugzilla) if you make more changes than just the indent fix. ::: browser/modules/PermissionUI.jsm:316 (Diff revision 4) > > if (this.permissionKey) { > - > + if ((state && state.checkboxChecked && state.source != "esc-press") || > - // Permanently store permission. > - if ((state && state.checkboxChecked) || > - promptAction.scope == SitePermissions.SCOPE_PERSISTENT) { > + promptAction.scope == SitePermissions.SCOPE_PERSISTENT) { the indent seems off by one space on this line. ::: toolkit/modules/PopupNotifications.jsm:817 (Diff revision 4) > > popupnotification.setAttribute("id", popupnotificationID); > popupnotification.setAttribute("popupid", n.id); > popupnotification.setAttribute("oncommand", "PopupNotifications._onCommand(event);"); > if (Services.prefs.getBoolPref("privacy.permissionPrompts.showCloseButton")) { > - popupnotification.setAttribute("closebuttoncommand", "PopupNotifications._onButtonEvent(event, 'secondarybuttoncommand');"); > + popupnotification.setAttribute("closebuttoncommand", "PopupNotifications._onButtonEvent(event, 'secondarybuttoncommand', 'esc-press');"); 'esc-press' is kinda misleading here. I guess our options are: - use a different string in this case, and check for it too in PermissionUI.jsm - document this behavior in the comment explaining the possible values of the "source" field. - file a follow-up bug to remove the close button and the privacy.permissionPrompts.showCloseButton pref altogether. The pref was added in bug 1323420 and I haven't heard of anybody wanting to change its default value since that. It may be time to remove it as dead code. I'm leaning slightly toward the third option, but the other 2 are fine too. ::: toolkit/modules/PopupNotifications.jsm:1485 (Diff revision 4) > notificationEl.ownerDocument.getBindingParent(notificationEl) || notificationEl.parentNode; > } > this._setNotificationUIState(notificationEl); > }, > > - _onButtonEvent(event, type, notificationEl = null) { > + _onButtonEvent(event, type, source = "unspecified", notificationEl = null) { The reason I was asking about if we ever expect "unspecified" is that I was wondering if we could make the default value "button" here and simplify the callers. I don't really see the point of providing a default value that we never expect to use.

(In reply to Florian Quèze [:florian] from comment #19) > - file a follow-up bug to remove the close button and the > privacy.permissionPrompts.showCloseButton pref altogether. The pref was > added in bug 1323420 and I haven't heard of anybody wanting to change its > default value since that. It may be time to remove it as dead code. Tanvi, Panos told me you pushed for having this pref at the time; are the reasons to have it still relevant?

Comment on attachment 8992775 [details] Bug 1473671 - Don't store persistent block permission if ESC pressed while showing permission doorhanger. https://reviewboard.mozilla.org/r/257626/#review266944 > The reason I was asking about if we ever expect "unspecified" is that I was wondering if we could make the default value "button" here and simplify the callers. I don't really see the point of providing a default value that we never expect to use. Sure, we can make the default "button" instead of "unspecified". That makes sense.

Pushed by cpearce@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/566be48427b6 Don't store persistent block permission if ESC pressed while showing permission doorhanger. r=florian

Verified on Nightly 63.0a1(20180817220110), that the sites are temporarily blocked to autoplay with sound after pressing the ESC key, while the doorhanger is displayed.