1341742 - push permission deny should use a similar strategy as geolocation

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P1)

Description

[Bryan Clark (DevTools PM) [:clarkbw]]

The Geolocation permission uses a temporary deny system that allows for a site to re-request the permission after whatever the algorithm allows for.  I would like the Push permission to use a similar strategy such that denying a site once (perhaps because a user was trying to avoid the immediate interruption) isn't a deny forever as it currently is.

Happy to discuss what the right algorithm might be but would like to initially take what geolocation is using and possibly improve the system in a followup bug.

Comment 1

[Matthew N. [:MattN]]

(In reply to Bryan Clark (DevTools PM) [:clarkbw] from comment #0)
> The Geolocation permission uses a temporary deny system that allows for a
> site to re-request the permission after whatever the algorithm allows for. 

It denies according to the checkbox so it's permanent or temporary depending on that.

> I would like the Push permission to use a similar strategy such that denying
> a site once (perhaps because a user was trying to avoid the immediate
> interruption) isn't a deny forever as it currently is.

Sites really shouldn't be asking for permission unless the user requests it and then they wouldn't have this problem since the user basically already wants the feature then before they see the prompt.

> Happy to discuss what the right algorithm might be but would like to
> initially take what geolocation is using and possibly improve the system in
> a followup bug.

Geolocation isn't working how you think though (as I said above).

I think there are two not too difficult solutions that would address your need though I think both have downsides for users:

A) Have the "deny" permission for permissions expire are N weeks/months - Permission Manager already supports this in the API we would just have to use it.
** Con: Users will think Firefox is forgetting their decision. Perhaps we can change the text of Deny button to "Not now" or something to not imply a permanent decision to help with this? We would also maybe need to rephrase the checkbox. We may also want a second deny option that actually is permanent in a dropdown menu on Don't Allow.

B) Change the buttons on the doorhanger to the following with no checkbox:

[ Not Now  |\/] [ Allow Notifications ]
| Never allow |
---------------

** Con: Makes it harder for users to never allow on sites they never want notifications from e.g. ones they rarely visit. Perhaps we can use history frecency to auto-deny requests in some cases?

Comment 2

[Johann Hofmann [:johannh]]

I can see why push notifications may require special handling (it doesn't make sense to temporarily allow them) and I guess Matt's suggestions would work. What I don't like about this proposal in general is that right now we have a pretty consistent UI for permission requests and that deviating from that would make using these doorhangers less intuitive. But I guess notifications is already different by having the checkbox checked by default.

> Sites really shouldn't be asking for permission unless the user requests it and then they wouldn't have this problem since the user basically already wants the feature then before they see the prompt.

100% agreed. I think the fact that many sites prompt for this permission on load makes this bug a bit more problematic. I can see folks being annoyed by the fact that they are being re-prompted for notifications on every reload by a certain site and disable the feature entirely (in the upcoming preferences UI for that).

Comment 3

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #1)
> (In reply to Bryan Clark (DevTools PM) [:clarkbw] from comment #0)
> > The Geolocation permission uses a temporary deny system that allows for a
> > site to re-request the permission after whatever the algorithm allows for. 
> 
> It denies according to the checkbox so it's permanent or temporary depending
> on that.

Right, I was just alluding to how with the defaults being the opposite a deny isn't permanent.  I understand the concern with Geolocation and with Push not defaulting to remember just doesn't make sense.

> > I would like the Push permission to use a similar strategy such that denying
> > a site once (perhaps because a user was trying to avoid the immediate
> > interruption) isn't a deny forever as it currently is.
> 
> Sites really shouldn't be asking for permission unless the user requests it
> and then they wouldn't have this problem since the user basically already
> wants the feature then before they see the prompt.

A very debatable situation.  You're answer seems clear cut and simple but in practice the simple answer doesn't generally work. 

I'd be happy to also look at stopping sites from requesting push permission on load because I think that's generally a bad thing.

I've also heard that Chrome is collecting stats on deny rates for sites and then using those aggregated deny rate to automatically deny sites for most users.  This goes after sites that abuse the permission to spam people.  And I'd be happy to pursue a similar avenue.

> > Happy to discuss what the right algorithm might be but would like to
> > initially take what geolocation is using and possibly improve the system in
> > a followup bug.
> 
> Geolocation isn't working how you think though (as I said above).

> I think there are two not too difficult solutions that would address your
> need though I think both have downsides for users:
> 
> A) Have the "deny" permission for permissions expire are N weeks/months -
> Permission Manager already supports this in the API we would just have to
> use it.
> ** Con: Users will think Firefox is forgetting their decision. Perhaps we
> can change the text of Deny button to "Not now" or something to not imply a
> permanent decision to help with this? We would also maybe need to rephrase
> the checkbox. We may also want a second deny option that actually is
> permanent in a dropdown menu on Don't Allow.

We'd like to look into using the "Not Now" text for this as well.  I think its the right way to message to users what is really happening.

I also think it would be good to record the 3rd "Not Now" choice as "Never Allow".  Which would mitigate the user feeling like we aren't actually remembering their choice.

> B) Change the buttons on the doorhanger to the following with no checkbox:
> 
> [ Not Now  |\/] [ Allow Notifications ]
> | Never allow |
> ---------------
> 
> ** Con: Makes it harder for users to never allow on sites they never want
> notifications from e.g. ones they rarely visit. Perhaps we can use history
> frecency to auto-deny requests in some cases?

This also looks good to me and I think allows for people who more clearly understand what's happening to have access to an option that does exactly what they want.  And allows for the default option to be something that's more of a human choice, allowing for errors.

With either (or both) of these I think we should remove the [x] remember checkbox.  Its a confusing option for something like Push and I argue it puts the burden on the user to do extra clicks to have a more reasonable interaction with the site.  And I'm saying "more reasonable" because I believe an option to "Never allow for the rest of time" isn't clearly communicated to users and is likely unexpected as there are few options in life that are forever.  maybe diamonds? :)

Comment 4

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to Johann Hofmann [:johannh] from comment #2)
> I can see why push notifications may require special handling (it doesn't
> make sense to temporarily allow them) and I guess Matt's suggestions would
> work. What I don't like about this proposal in general is that right now we
> have a pretty consistent UI for permission requests and that deviating from
> that would make using these doorhangers less intuitive. But I guess
> notifications is already different by having the checkbox checked by default.

Fair points.  I hope that making sure the Deny / Allow interactions are consistent is what our users need; I assume that's the most interacted with area and so that should behave in their best interests even if that is different actions.  

Push is pretty different in a number of ways.  If you compare to Geolocation, Push doesn't have an immediate loss of privacy once enabled. In other words turning Push on doesn't reveal your location or identity to the site, it just allows them to start sending you messages.  Hopefully those messages are easy to turn off. It does have some risks like revealing your IP to mozilla but I think is fairly low in general.

> > Sites really shouldn't be asking for permission unless the user requests it and then they wouldn't have this problem since the user basically already wants the feature then before they see the prompt.
> 
> 100% agreed. I think the fact that many sites prompt for this permission on
> load makes this bug a bit more problematic. I can see folks being annoyed by
> the fact that they are being re-prompted for notifications on every reload
> by a certain site and disable the feature entirely (in the upcoming
> preferences UI for that).

I would like to address this somehow and would love to discuss ideas elsewhere on how we can do that.  I agree that sites shouldn't be asking for any permissions on first load.  I do worry that this kind of protection becomes a tricky arms race like popup protection is.  Nonetheless, I would like to talk more about this.

Comment 5

[Matthew N. [:MattN]]

(In reply to Bryan Clark (DevTools PM) [:clarkbw] from comment #3)
> I'd be happy to also look at stopping sites from requesting push permission
> on load because I think that's generally a bad thing.

Great. If we don't allow them on load then when will we allow them? After N seconds? My suggestion is to require user interaction in order to open the doorhanger.

> I've also heard that Chrome is collecting stats on deny rates for sites and
> then using those aggregated deny rate to automatically deny sites for most
> users.  This goes after sites that abuse the permission to spam people.  And
> I'd be happy to pursue a similar avenue.

I think the two options above would be very helpful to reduce the negative impact of making it harder to permanently deny and break the consistency of how to do so. The main problem with the latter is potential privacy concerns about submitting the data so is that something currently being tracked/discussed?

> > [ Not Now  |\/] [ Allow Notifications ]
> > | Never allow |
> > ---------------
>
> This also looks good to me and I think allows for people who more clearly
> understand what's happening to have access to an option that does exactly
> what they want.  And allows for the default option to be something that's
> more of a human choice, allowing for errors.

Great so it seems like we agree on potential UI with the checkbox gone it's just a matter of deciding the duration of "Not Now" being effective:
i) N weeks/months
ii) for the session or tab life
iii) just once.

I think if we don't go for (i) above with N being at least 6 weeks or so then we should probably only do this after one of the anti-abuse/spam measures mentioned above.

Comment 6

[Matthew N. [:MattN]]

(In reply to Bryan Clark (DevTools PM) [:clarkbw] from comment #4)
> Push is pretty different in a number of ways.  If you compare to
> Geolocation, Push doesn't have an immediate loss of privacy once enabled. In
> other words turning Push on doesn't reveal your location or identity to the
> site, it just allows them to start sending you messages.  Hopefully those
> messages are easy to turn off. It does have some risks like revealing your
> IP to mozilla but I think is fairly low in general.

Unless we changed things, a site can still send many pushes without showing UI. Those pushes can send a unique tracking identifier to whatever server the service worker wants. That server would also get the users IP.

> I would like to address this somehow and would love to discuss ideas
> elsewhere on how we can do that.  I agree that sites shouldn't be asking for
> any permissions on first load.  I do worry that this kind of protection
> becomes a tricky arms race like popup protection is.  Nonetheless, I would
> like to talk more about this.

If you would like to talk about this Friday, we can bring it up as a discussion topic during the FxPrivacy work week going on in MV.

Comment 7

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #1)
> B) Change the buttons on the doorhanger to the following with no checkbox:
> 
> [ Not Now  |\/] [ Allow Notifications ]
> | Never allow |

Because allowing notifications is always a permanent decision that persists across browser restarts, the main perceived advantage of this UI over the one we already have for all other permissions is that we don't have two UI states that would do exactly the same thing. The two states are clicking "Allow Notifications" with the checkbox checked and clicking "Allow Notifications" with the checkbox unchecked.

However, I wonder how much this matters to users in practice. A checkbox (unchecked by default) to remember the decision is consistent with all other permission requests, and is more visible than a "v" icon.

The problems related to avoiding permission spamming on page load are the same for notifications and other permissions like Geolocation. I see value in using the same UI to solve the same problem. We devised the "Temporary Block" solution (that takes effect for the duration of 1 hour) exactly for this purpose. If we need to make this more powerful, or add heuristics, we should probably file a different bug.

> ** Con: Makes it harder for users to never allow on sites they never want
> notifications from e.g. ones they rarely visit. Perhaps we can use history
> frecency to auto-deny requests in some cases?

By unifying the UI with the other permissions, instead of using the dropdown, we don't make it harder to never allow push compared to other permissions, and I see this as a positive.

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

We had a discussion with Bryan and decided to a) get rid of the checkbox and b) have a drop-down for the deny button: a "not now" default behavior with a "never" option (like the password manager prompt).

There was a concern that showing a checkbox that doesn't do anything (for the allow case) is miscommunicating to users.

Comment 9

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1341742 - Split secondary action for push notification permission prompt into "not now" and "never".

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

Comment 10

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8841148 [details]
Bug 1341742 - Split secondary action for push notification permission prompt into "not now" and "never".

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

Comment 11

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8841148 [details]
Bug 1341742 - Split secondary action for push notification permission prompt into "not now" and "never".

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

Comment 12

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8841148 [details]
Bug 1341742 - Split secondary action for push notification permission prompt into "not now" and "never".

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

Comment 13

[Johann Hofmann [:johannh]]

Comment on attachment 8841148 [details]
Bug 1341742 - Split secondary action for push notification permission prompt into "not now" and "never".

https://reviewboard.mozilla.org/r/115478/#review117748

Clearing review since the general direction seems good but there are some details around PBM and strings to figure out. Also, it seems to me that there are still try failures.

::: browser/locales/en-US/chrome/browser/browser.properties:493
(Diff revision 4)
> -webNotifications.rememberForSession=Remember decision for this session
>  webNotifications.allow=Allow Notifications
>  webNotifications.allow.accesskey=A
> -webNotifications.dontAllow=Don’t Allow
> -webNotifications.dontAllow.accesskey=n
> +webNotifications.notNow=Not Now
> +webNotifications.notNow.accesskey=n
> +webNotifications.never=Never

In the UI, "Never" looks a bit confusing to me. Maybe "Never Allow" would be clearer.

Also, I wonder if we should say "Never for this session" in PBM.

I guess we'll have to ask someone from UX.

::: browser/modules/PermissionUI.jsm:586
(Diff revision 4)
> +      {
> +        label: gBrowserBundle.GetStringFromName("webNotifications.never"),
>          accessKey:
> -          gBrowserBundle.GetStringFromName("webNotifications.dontAllow.accesskey"),
> +          gBrowserBundle.GetStringFromName("webNotifications.never.accesskey"),
>          action: SitePermissions.BLOCK,
> +        scope: SitePermissions.SCOPE_PERSISTENT,

This needs to consider the case we're in private browsing mode (PrivateBrowsingUtils.isBrowserPrivate) and switch to SCOPE_SESSION accordingly, to not persist data about the website.

::: browser/modules/test/head.js:190
(Diff revision 4)
> + *
>   * @return {Promise}
>   *         Resolves once the panel has fired the "popuphidden"
>   *         event.
>   */
> -function clickSecondaryAction() {
> +function clickSecondaryAction(aActionIndex) {

Nit: Since there's no consistent style around hungarian notation in this file I'd just call this actionIndex

::: browser/modules/test/head.js:203
(Diff revision 4)
>  
> +  return Task.spawn(function* () {
> +    // Click the dropmarker arrow and wait for the menu to show up.
> +    let dropdownPromise =
> +      BrowserTestUtils.waitForEvent(popupNotification.menupopup, "popupshown");
> +    yield EventUtils.synthesizeMouseAtCenter(popupNotification.menuButton, {}, window);

Nit: no need to pass window here I think

::: browser/modules/test/head.js:211
(Diff revision 4)
> +    // The menuitems in the dropdown are accessible as direct children of the panel,
> +    // because they are injected into a <children> node in the XBL binding.
> +    // The target action is the menuitem at index aActionIndex - 1, because the first
> +    // secondary action (index 0) is the button shown directly in the panel.
> +    let actionMenuItem = popupNotification.querySelectorAll("menuitem")[aActionIndex - 1];
> +    yield EventUtils.synthesizeMouseAtCenter(actionMenuItem, {}, window);

Nit: No need for window

::: toolkit/content/widgets/notification.xml:518
(Diff revision 4)
>          <children includes="button"/>
>          <xul:button anonid="secondarybutton"
>                      class="popup-notification-button"
>                      xbl:inherits="oncommand=secondarybuttoncommand,label=secondarybuttonlabel,accesskey=secondarybuttonaccesskey,hidden=secondarybuttonhidden"/>
>          <xul:toolbarseparator xbl:inherits="hidden=dropmarkerhidden"/>
> -        <xul:button type="menu"
> +        <xul:button anonid="menuButton"

I think the other anonids are all lowercase, we should keep it like that.

Comment 14

[Johann Hofmann [:johannh]]

Bryan, since it's merge day and this is not done yet we have a couple of options:

a) Pre-land the strings for this and uplift the rest of the patch. It's quite late for this, but it could work. I'm not sure how viable this is since I'd still like to get someone from UX to approve the strings (I'll needinfo Philipp) and this will only get us to 54, so one version after the new prompts were introduced.

b) Implement Paolo's idea from comment 7 that we discussed in our meeting, where the checkbox would have the same behavior for Allow and be unchecked by default. We could likely get this uplifted to 53. We could still do the solution that we originally agreed on for 55 then.

c) Land this normally and live with the fact that it's in 55. See how the prompt from 53 performs and know that we have a potentially better solution upcoming.

Comment 15

[Johann Hofmann [:johannh]]

Philipp, can you please find someone who can sign off the changed strings for this? Right now it looks like this:

-----------------------------------------------------
                                                    |
Will you allow example.com to send notifications?   |
                                                    |
                                                    |
[ Not Now |\/] [ Allow Notifications ]
     | Never |
     ---------

See my concerns about "Never" in comment 5.

Thank you!

Comment 16

[Bryan Clark (DevTools PM) [:clarkbw]]

Hey, I was out until just today.  

Since we don't have a) signed off I don't want to ship strings that we'd end up needing to change later.  I guess we go with c) which can give us a little more time to get feedback and get this done right.

Adding Michelle who can take a look at the strings and overall proposal.

Comment 17

[:Paolo Amadini]

(In reply to Bryan Clark (DevTools PM) [:clarkbw] from comment #16)
> Since we don't have a) signed off I don't want to ship strings that we'd end
> up needing to change later.  I guess we go with c) which can give us a
> little more time to get feedback and get this done right.

Johann's question spanned multiple versions, but I think we need to think specifically about what we want for Firefox 53, which is the first version that users will see with the new design that includes a "Don't Allow" button in a notification that is not dismissed by clicking elsewhere in the page.

For Firefox 53 we have to work with the limitation of not being able to add any new strings. I see these options:

1) Don't do anything

It will be very easy for users to deny the request permanently. Even if we change the UI in a later version, users that are prompted during the six weeks of Release 53 might have denied permanently, and they won't be prompted again when the next version is shipped, unless we devise a way to prompt them again, which is subject to further discussion.

So, no effort required right now, potentially more thought to be put on a migration strategy later.

2) Checkbox disabled by default

This solves the main issue with solution (1), and requires no string change. It brings the concern from comment 8 that showing a checkbox that doesn't do anything (for the allow case) is miscommunicating to users. As you know, a few people including me put less weight on this concern than the permanent deny issue.

3) Implement the dropdown with string patchwork

You can implement the solution of the dropdown by reusing existing strings like:

addonInstallRestartIgnoreButton=Not Now

I think we've done this string patchwork before in a few occasions. This has the disadvantage that the translation may, and probably will, look awkward in some languages, so we'll solve both the issue with (1) and the slight concern in (2) with the disadvantage that for six weeks the prompt may have an awkward translation.

Bryan, if going with (1) is your choice, I think the team will be fine with that, but I want to make sure that we've considered the consequences of all the options. I still recommend option (2) for Firefox 53.

For Firefox 54, we're still on time to pre-land strings and get uplift approval from Release Management since we're in the first few days of the Aurora cycle, so we can implement the final solution there.

Comment 18

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #17)
> Bryan, if going with (1) is your choice, I think the team will be fine with
> that, but I want to make sure that we've considered the consequences of all
> the options. I still recommend option (2) for Firefox 53.

And just to clarify, at yesterday's team meeting I got that there was consensus that going with (2) was a good option, although we didn't have all the people from the original meeting mentioned in comment 8, so we wanted to include more people in the discussion by posting on the bug.

Comment 19

[mheubusch]

Please use the strings developed per our earlier discussion re: Facebook with the addition of the drop down.  So, remove checkbox, add a link to learn more (cap L, small m, no ellipsis), new string for main message, and the user options below: 



[domain name] wants to send you notifications. 
Learn more
[Not Now]\/ [Allow Notifications]
[Never Allow]

For reference, documentation herehttps://docs.google.com/document/d/143nEfWfIvFZD2-pFqE8GD85inFjFIhE2J8QeegQufc0/edit#  (but needs to be updated if we add the Never Allow) which I will do once philipp approves this interaction pattern.

Comment 20

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Yeah, the interaction pattern looks viable (assuming that it is essentially the same as we have for passwords)

Comment 21

[Peter Dolanjski [:pdol]]

Re-adding clarkbw to reply to comment 17

Comment 22

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8841148 [details]
Bug 1341742 - Split secondary action for push notification permission prompt into "not now" and "never".

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

Comment 23

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8841148 [details]
Bug 1341742 - Split secondary action for push notification permission prompt into "not now" and "never".

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

Comment 24

[(inactive) Nihanth Subramanya [:nhnt11]]

The latest patch shows "Never Allow" for normal windows and "Never for this session" in private browsing windows. Michelle, does this sound ok?

Comment 25

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Leave "remember" checkbox unchecked by default

Here's a patch to uncheck the "remember" checkbox by default, in the interest of saving time once a decision is made.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6244e5a29d26e7d40193b9ce64befb158465331

Comment 26

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Leave "remember" checkbox unchecked by default, v2

Added a test that tests the permission after enabling the checkbox.

Comment 27

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Nihanth Subramanya [:nhnt11] from comment #26)
> Created attachment 8845408 [details] [diff] [review]
> Leave "remember" checkbox unchecked by default, v2
> 
> Added a test that tests the permission after enabling the checkbox.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02ce2eb6f60c4f3968dce41750a208022567ea07

Comment 28

[mheubusch]

(In reply to Nihanth Subramanya [:nhnt11] from comment #24)
> The latest patch shows "Never Allow" for normal windows and "Never for this
> session" in private browsing windows. Michelle, does this sound ok?

Hi Nihanth - I don't see anything in the interaction spec that differentiates private and non-private browsing, so didn't know the language was different.  Can you tell me what all 3/4? strings are in private browsing for this notification door hanger?  The main message and the button labels?  Thanks in advance for your help, and let me know it's easier to chat over Vidyo.

Also NI'ing Philipp in case he has a spec that is different from the one I worked on with Ashlinn.

Comment 29

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to mheubusch from comment #28)
> (In reply to Nihanth Subramanya [:nhnt11] from comment #24)
> > The latest patch shows "Never Allow" for normal windows and "Never for this
> > session" in private browsing windows. Michelle, does this sound ok?
> 
> Hi Nihanth - I don't see anything in the interaction spec that
> differentiates private and non-private browsing, so didn't know the language
> was different.  Can you tell me what all 3/4? strings are in private
> browsing for this notification door hanger?  The main message and the button
> labels?  Thanks in advance for your help, and let me know it's easier to
> chat over Vidyo.
> 
> Also NI'ing Philipp in case he has a spec that is different from the one I
> worked on with Ashlinn.

Bad communication from me.

To be clear:

a) As it currently exists, the string for the "remember" checkbox in the notification has a different version for private browsing.

b) Johann wondered in his review comments for this patch whether we should have an alternate string for the "Never" button. I think this is fair, since we are removing the checkbox, the alternate string would be analogous to the existing one that we're removing.

c) Johann said we should probably ask UX to make the decision. I decided to include the extra string in the patch because it was just a couple of lines, and then ask whether we want it in the bug. I seem to have neglected to ni? UX in addition to you.

I hope this makes things clear. If we don't want the string, it's just a couple of lines to remove from the patch.

Comment 30

[mheubusch]

(In reply to Nihanth Subramanya [:nhnt11] from comment #29)
> (In reply to mheubusch from comment #28)
> > (In reply to Nihanth Subramanya [:nhnt11] from comment #24)
> > > The latest patch shows "Never Allow" for normal windows and "Never for this
> > > session" in private browsing windows. Michelle, does this sound ok?
> > 
> > Hi Nihanth - I don't see anything in the interaction spec that
> > differentiates private and non-private browsing, so didn't know the language
> > was different.  Can you tell me what all 3/4? strings are in private
> > browsing for this notification door hanger?  The main message and the button
> > labels?  Thanks in advance for your help, and let me know it's easier to
> > chat over Vidyo.
> > 
> > Also NI'ing Philipp in case he has a spec that is different from the one I
> > worked on with Ashlinn.
> 
> Bad communication from me.
> 
> To be clear:
> 
> a) As it currently exists, the string for the "remember" checkbox in the
> notification has a different version for private browsing.
> 
> b) Johann wondered in his review comments for this patch whether we should
> have an alternate string for the "Never" button. I think this is fair, since
> we are removing the checkbox, the alternate string would be analogous to the
> existing one that we're removing.
> 
> c) Johann said we should probably ask UX to make the decision. I decided to
> include the extra string in the patch because it was just a couple of lines,
> and then ask whether we want it in the bug. I seem to have neglected to ni?
> UX in addition to you.
> 
> I hope this makes things clear. If we don't want the string, it's just a
> couple of lines to remove from the patch.

Thanks for explaining that - I wonder if in practice, the Not Now decision would persist thru an entire session so in PBM there is really no need for the dropdown action. I've reread the bug and can't tell how we've defined the duration of Not Now - see comment 5: 
Great so it seems like we agree on potential UI with the checkbox gone it's just a matter of deciding the duration of "Not Now" being effective:
i) N weeks/months
ii) for the session or tab life
iii) just once.

If we opted for ii, then no need to include this. If we opted for i, let words the dropdown string as "Not for This Session" 

Thanks for clarifying (also, I am in UX, I just ni'ed Philipp in case he had more context than I did).I will clear the NI from his name.

Comment 31

[Johann Hofmann [:johannh]]

(In reply to mheubusch from comment #30)
> Great so it seems like we agree on potential UI with the checkbox gone it's
> just a matter of deciding the duration of "Not Now" being effective:
> i) N weeks/months
> ii) for the session or tab life
> iii) just once.
> 
> If we opted for ii, then no need to include this. If we opted for i, let
> words the dropdown string as "Not for This Session" 

Unfortunately "Not Now" is a little more complicated and does not fit any of the options. It's a temporary block only active on this tab that lasts until the user reloads the page or navigates to a site with a different origin (or closes the tab).

But yeah we might be fine with this kind of "Not Now" in PBM and just remove the dropdown.

Comment 32

[Johann Hofmann [:johannh]]

Comment on attachment 8841148 [details]
Bug 1341742 - Split secondary action for push notification permission prompt into "not now" and "never".

https://reviewboard.mozilla.org/r/115478/#review120988

r=me, keeping in mind that we still have to get consensus on a solution for PBM (this patch works on the technical side but we might do it differently)

::: browser/locales/en-US/chrome/browser/browser.properties:497
(Diff revision 6)
>  webNotifications.allow=Allow Notifications
>  webNotifications.allow.accesskey=A
> -webNotifications.dontAllow=Don’t Allow
> -webNotifications.dontAllow.accesskey=n
> +webNotifications.notNow=Not Now
> +webNotifications.notNow.accesskey=n
> +webNotifications.never=Never Allow
> +webNotifications.neverForSession=Never for this session

We still have to debate the exact wording here but I just wanted to remark that the capitalization is different from "webNotifications.never"

::: browser/modules/test/browser/browser_PermissionUI_prompts.js:82
(Diff revision 6)
>  
> -    // Test denying the permission request with the checkbox checked.
> +    // Test denying the permission request with the checkbox checked (for geolocation)
> +    // or by clicking the "never" option from the dropdown (for notifications).
>      popupNotification = getPopupNotificationNode();
> +    let secondaryActionToClickIndex = 0;
> +    if (Prompt == PermissionUI.GeolocationPermissionPrompt) {

It might be better/more maintainable to special-case consistently in this function, so check if Prompt == PermissionUI.DesktopNotificationPermissionPrompt instead here. And you could put it in a variable like isNotificationPrompt then :)

Comment 33

[Johann Hofmann [:johannh]]

Comment on attachment 8845408 [details] [diff] [review]
Leave "remember" checkbox unchecked by default, v2

Review of attachment 8845408 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine, but we should treat PBM consistently if we were to go with this solution. I'm also surprised that there weren't more test changes required. Did you do a try run already?

Thinking about it, this actually sets a precedent for the PBM decision regarding the other patch. We should probably not show the dropdown in PBM mode, that would be more consistent with Geolocation. Let's see what Bryan thinks about this.

::: browser/modules/PermissionUI.jsm
@@ +540,5 @@
>      let learnMoreURL =
>        Services.urlFormatter.formatURLPref("app.support.baseURL") + "push";
>  
>      let checkbox = {
>        show: true,

We should not show the checkbox in PBM, like geolocation does it.

@@ +541,5 @@
>        Services.urlFormatter.formatURLPref("app.support.baseURL") + "push";
>  
>      let checkbox = {
>        show: true,
> +      checked: false,

Nit: you can just remove this line

Comment 34

[:Paolo Amadini]

Bryan, the needinfo is for comment 17.

Comment 35

[mheubusch]

Hi - did anyone on this thread delete the copy spec for doorhangers/control center notifications. i linked to it in one of the comments: https://docs.google.com/document/d/143nEfWfIvFZD2-pFqE8GD85inFjFIhE2J8QeegQufc0/edit#

I can't access it now and it doesn't appear in a search of google drive.

Comment 36

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to :Paolo Amadini from comment #17)
> (In reply to Bryan Clark (DevTools PM) [:clarkbw] from comment #16)
> > Since we don't have a) signed off I don't want to ship strings that we'd end
> > up needing to change later.  I guess we go with c) which can give us a
> > little more time to get feedback and get this done right.
> 
> Johann's question spanned multiple versions, but I think we need to think
> specifically about what we want for Firefox 53, which is the first version
> that users will see with the new design that includes a "Don't Allow" button
> in a notification that is not dismissed by clicking elsewhere in the page.
> 
> For Firefox 53 we have to work with the limitation of not being able to add
> any new strings. I see these options:
> 
> 1) Don't do anything
> 
> It will be very easy for users to deny the request permanently. Even if we
> change the UI in a later version, users that are prompted during the six
> weeks of Release 53 might have denied permanently, and they won't be
> prompted again when the next version is shipped, unless we devise a way to
> prompt them again, which is subject to further discussion.

Yes, I'll open up a couple new bugs for continuing this.  One for creating a permanent deny when the user has hit "Not Now" a certain number of times.  And we have a couple more ideas to research as well that I'll bring up later.

> So, no effort required right now, potentially more thought to be put on a
> migration strategy later.
> 
> 2) Checkbox disabled by default
> 
> This solves the main issue with solution (1), and requires no string change.
> It brings the concern from comment 8 that showing a checkbox that doesn't do
> anything (for the allow case) is miscommunicating to users. As you know, a
> few people including me put less weight on this concern than the permanent
> deny issue.

I think this is clever but I don't like how it feels deceptive.  I'd prefer removing the remember all together instead.

> 3) Implement the dropdown with string patchwork
> 
> You can implement the solution of the dropdown by reusing existing strings
> like:
> 
> addonInstallRestartIgnoreButton=Not Now
> 
> I think we've done this string patchwork before in a few occasions. This has
> the disadvantage that the translation may, and probably will, look awkward
> in some languages, so we'll solve both the issue with (1) and the slight
> concern in (2) with the disadvantage that for six weeks the prompt may have
> an awkward translation.

Hmm, I'm not sure how to make the call here.  I have seen these issues before, but if we felt the translations will be close and then within a release the improved translations will be updated that doesn't sound too bad.  But again I don't feel like I have enough context to make a good call here.

> Bryan, if going with (1) is your choice, I think the team will be fine with
> that, but I want to make sure that we've considered the consequences of all
> the options. I still recommend option (2) for Firefox 53.

I'm not comfortable with that option, I'd rather wait and allow for a single release to pass.

> For Firefox 54, we're still on time to pre-land strings and get uplift
> approval from Release Management since we're in the first few days of the
> Aurora cycle, so we can implement the final solution there.

I'd rather focus on this.

Comment 37

[Johann Hofmann [:johannh]]

(In reply to Bryan Clark (DevTools PM) [:clarkbw] from comment #36)
> (In reply to :Paolo Amadini from comment #17)
> > For Firefox 54, we're still on time to pre-land strings and get uplift
> > approval from Release Management since we're in the first few days of the
> > Aurora cycle, so we can implement the final solution there.
> 
> I'd rather focus on this.

I don't think we can still land those strings in 54. It wouldn't be pre-land anyway since this patch has an r+ and could just be uplifted then, but yeah, it's quite late now.

Comment 38

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8841148 [details]
Bug 1341742 - Split secondary action for push notification permission prompt into "not now" and "never".

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

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a60007195df5
Split secondary action for push notification permission prompt into "not now" and "never". r=johannh

Comment 40

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a60007195df5

Comment 41

[Francesco Lodolo [:flod]]

(In reply to Johann Hofmann [:johannh] from comment #37)
> I don't think we can still land those strings in 54. It wouldn't be pre-land
> anyway since this patch has an r+ and could just be uplifted then, but yeah,
> it's quite late now.

Correct. Worth pointing out that comment 17 was made 2 weeks ago, we're in the middle of the cycle now. See also bug 1348735 regarding l10n.

(In reply to mheubusch from comment #35)
> Hi - did anyone on this thread delete the copy spec for doorhangers/control
> center notifications. i linked to it in one of the comments:
> https://docs.google.com/document/d/143nEfWfIvFZD2-
> pFqE8GD85inFjFIhE2J8QeegQufc0/edit#
> 
> I can't access it now and it doesn't appear in a search of google drive.

Same here. I'd really like to see those specs. If one of the strings is available only in private mode, worth calling it out in a comment.

Comment 42

[Hani Yacoub, Desktop QA]

Build ID: 20170327030203
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

Verified as fixed on Firefox Nightly 55.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.