Closed Bug 1219454 Opened 4 years ago Closed 4 years ago

Change Notification text in Preferences from "show" to "receive"

Categories

(Core :: DOM: Push Notifications, defect)

44 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla45
Tracking Status
firefox44 --- affected
firefox45 --- verified

People

(Reporter: tanvi, Assigned: tanvi)

References

Details

Attachments

(2 files, 2 obsolete files)

Notification text has changed from "show" to "receive" now that it includes the new Push capability.

The doorhanger text says:
Would you like to receive notifications from this site?
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties#384

The preferences text should match that:
Choose which sites are allowed to receive notifications
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/preferences/content.dtd#11

Control which websites are always or never allowed to receive notifications. If you remove a site, it will need to request permission again.
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/preferences/preferences.properties#28

This is just a s/show/receive/ in 2 places.
Group: core-security
Sounds good. 

There is another use of shown, but I don't think we should change it. For DND (on Windows) the string says: 

"No Notification will be shown until you restart Nightly" 

but we should keep it that way, since it really is about showing visible notifications to users.
Attached patch Bug1219454-part1.patch (obsolete) — Splinter Review
Bill can you comment on this?
Flags: needinfo?(wmaggs)
Attached patch Bug1219454-part2.patch (obsolete) — Splinter Review
A few more updates:
1) Show Notifications to Receive Notifications in control center
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/sitePermissions.properties#11

2) Show Notifications to Receive Notifications in about:permissions
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/preferences/aboutPermissions.dtd#44

3) "Change whether the site can show you notifications" to "Change whether the site can receive notifications"
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#204
(In reply to Tanvi Vyas [:tanvi] from comment #2)
> Created attachment 8680285 [details] [diff] [review]
> Bug1219454-part1.patch
> 
> Bill can you comment on this?

Sorry, looks like you already did.  Please also see comment 3.
Flags: needinfo?(wmaggs)
Attachment #8680291 - Flags: review?(MattN+bmo)
Attachment #8680285 - Flags: review?(MattN+bmo)
Comment on attachment 8680285 [details] [diff] [review]
Bug1219454-part1.patch

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

::: browser/locales/en-US/chrome/browser/preferences/content.dtd
@@ +7,5 @@
>  <!ENTITY  blockPopups.label           "Block pop-up windows">
>  <!ENTITY  blockPopups.accesskey       "B">
>  
>  <!ENTITY  notificationsPolicy.label            "Notifications">
> +<!ENTITY  notificationsPolicyDesc.label        "Choose which sites are allowed to receive notifications">

Please change the ID and references

::: browser/locales/en-US/chrome/browser/preferences/preferences.properties
@@ +24,5 @@
>  addonspermissionstext=You can specify which websites are allowed to install add-ons. Type the exact address of the site you want to allow and then click Allow.
>  addons_permissions_title=Allowed Sites - Add-ons Installation
>  popuppermissionstext=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.
>  popuppermissionstitle=Allowed Sites - Pop-ups
> +notificationspermissionstext2=Control which websites are always or never allowed to receive notifications. If you remove a site, it will need to request permission again.

Please change the ID and references.

I thought I told jaws to fix this word in my review…
Attachment #8680285 - Flags: review?(MattN+bmo)
Attachment #8680291 - Flags: review?(MattN+bmo)
Attachment #8680285 - Attachment is obsolete: true
Attachment #8680305 - Flags: review?(MattN+bmo)
Attachment #8680305 - Flags: review?(MattN+bmo) → review+
Assignee: nobody → tanvi
Status: NEW → ASSIGNED
"desktop-notification" label for aboutPermissions and sitePermissions is not done yet.

Feel free to update those if I haven't updated this bug.
Attachment #8680319 - Flags: review?(MattN+bmo)
Attachment #8680291 - Attachment is obsolete: true
Landed Part 1.  Matt is going to work on updating Part 2 tonight, which turns out to be a little more complicated than we originally thought.
Keywords: leave-open
Comment on attachment 8680319 [details] [diff] [review]
Bug1219454-part2-10-28-15.patch

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

r=me with some fixes landed
Attachment #8680319 - Flags: review?(MattN+bmo) → review+
I'm quite confused by the new strings

Choose which sites are allowed to receive notifications
Control which websites are always or never allowed to receive notifications. If you remove a site, it will need to request permission again.

Is it the site that receives notification? That's the exact opposite of "Would you like to receive notifications from this site?"
(In reply to Francesco Lodolo [:flod] from comment #13)
> I'm quite confused by the new strings
> 
> Choose which sites are allowed to receive notifications
> Control which websites are always or never allowed to receive notifications.
> If you remove a site, it will need to request permission again.
> 
> Is it the site that receives notification? That's the exact opposite of
> "Would you like to receive notifications from this site?"

Yeah, we should swap "receive" with "send" in the 'Control ...' string.
Flags: needinfo?(tanvi)
Flags: needinfo?(MattN+bmo)
Same for the "Choose which sites..." string too.
Needinfo'ing Matej to see if he has thoughts.(In reply to (Away 10/30 - 11/3) Jared Wein [:jaws] (please needinfo? me) from comment #14)

> (In reply to Francesco Lodolo [:flod] from comment #13)
> > I'm quite confused by the new strings
> > 
> > Choose which sites are allowed to receive notifications
> > Control which websites are always or never allowed to receive notifications.
> > If you remove a site, it will need to request permission again.
> > 
> > Is it the site that receives notification? That's the exact opposite of
> > "Would you like to receive notifications from this site?"
> 
> Yeah, we should swap "receive" with "send" in the 'Control ...' string.
> Same for the "Choose which sites..." string too.
Flags: needinfo?(matej)
(In reply to Tanvi Vyas [:tanvi] from comment #16)
> Needinfo'ing Matej to see if he has thoughts.(In reply to (Away 10/30 -
> 11/3) Jared Wein [:jaws] (please needinfo? me) from comment #14)
> 
> > (In reply to Francesco Lodolo [:flod] from comment #13)
> > > I'm quite confused by the new strings
> > > 
> > > Choose which sites are allowed to receive notifications
> > > Control which websites are always or never allowed to receive notifications.
> > > If you remove a site, it will need to request permission again.
> > > 
> > > Is it the site that receives notification? That's the exact opposite of
> > > "Would you like to receive notifications from this site?"
> > 
> > Yeah, we should swap "receive" with "send" in the 'Control ...' string.
> > Same for the "Choose which sites..." string too.

Using "send" instead of "receive" sounds right to me.
Flags: needinfo?(matej)
I assume we need to change this string too?
-<!ENTITY urlbar.webNotsNotificationAnchor.label         "Change whether the site can show you notifications">
+<!ENTITY urlbar.webNotsNotificationAnchor2.label        "Change whether the site can receive notifications">

To: Change whether the site can send notifications
?
Flags: needinfo?(tanvi)
Blocks: 1219832
(In reply to Pulsebot from comment #8)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/01ae6c388605

Tanvi, FYI landing this on inbound caused a merge conflict and led to bug 1218908 getting backed out. Changes to only browser/ should ideally land on fx-team to avoid conflicts.
Flags: needinfo?(MattN+bmo)
https://hg.mozilla.org/mozilla-central/rev/33f17a45547f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
> https://hg.mozilla.org/mozilla-central/diff/01ae6c388605/browser/locales/en-US/chrome/browser/preferences/content.dtd
> -<!ENTITY  notificationsPolicyDesc.label        "Choose which sites are allowed to show notifications">
> +<!ENTITY  notificationsPolicyDesc2.label        "Choose which sites are allowed to receive notifications">

Now, websites (and not browser) are are receiving notifications?
Verified that:
- the notification text is changed from "show" to "receive" in about:permissions and in the control center.
- the notification text is changed from "show" to "send" in about:preferences. 

Verified as fixed using the latest Nightly 45 on Ubuntu 14.04x64, Windows 7x64, Windows 10, and Mac OS X 10.9.
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:45.0) Gecko/20100101 Firefox/45.0
Mozilla/5.0 (Windows NT 10.0; rv:45.0) Gecko/20100101 Firefox/45.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20151103030248
Status: RESOLVED → VERIFIED
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
You need to log in before you can comment on or make changes to this bug.