Closed Bug 1219832 Opened 4 years ago Closed 4 years ago

Change Notification text in Preferences from "receive' to "send"

Categories

(Firefox :: Preferences, defect)

44 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox44 --- fixed

People

(Reporter: tanvi, Assigned: Lina)

References

Details

Attachments

(1 file, 1 obsolete file)

Given the early merge date, we rushed to get some changes in for the new notifications in Firefox 44.  And with the rush, we got some things wrong.

1) 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

sites aren't receiving the notification, the user/browser is.

2) 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

3) Change whether the site can receive notifications
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#204

This will need to be landed and uplifted this week.

+++ This bug was initially created as a clone of Bug #1219454 +++
Change to:

1) Choose which sites are allowed to send you notifications

2) Control which websites are always or never allowed to send you notifications. If you remove a site, it will need to request permission again.

3) Change whether the site can send you notifications
Assignee: nobody → kcambridge
Attached patch 1219832.patch (obsolete) — Splinter Review
Attachment #8680870 - Flags: review?(MattN+bmo)
Comment on attachment 8680870 [details] [diff] [review]
1219832.patch

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

Don't you need to also change the initial doorhanger string too? urlbar.webNotsNotificationAnchor3.label should match it since they're used together.

webNotifications.receiveFromSite=Would you like to receive notifications from this site?
Attachment #8680870 - Flags: review?(MattN+bmo) → feedback+
Ah, I asked about that on IRC...

> 11:47 <kitcambridge> tanvi: should we change the doorhanger prompt, too? "Would you like to receive notifications from this site?" -> "Would you like this site to send you notifications?"
> 11:47 <tanvi> kitcambridge: i think we should leave that as is for now
> 11:48 <kitcambridge> cool, will do. thanks!
> 11:48 <tanvi> that is what we got from talking to bmaggs, phlsa, etc.  and we were tryign to capture the fact that the messages might be received int eh background
> 11:48 <tanvi> its not a bad idea, but i think at this point we shoudl leave it
> 11:48 <kitcambridge> makes sense
> 11:50 <edwong> yeah - no more string changes ;)

Are you OK with changing the doorhanger text, Tanvi?
Flags: needinfo?(tanvi)
The main thing is that we need the two strings mentioned in comment 3 to be consistent as they're both for the doorhanger permission prompt.
At this point, I think we should leave urlbar.webNotsNotificationAnchor2.label as it is then.  To change the main permission string (the only one that most users are going to see), we should talk to Matej and Philipp too.
Flags: needinfo?(tanvi)
I agree with Tanvi. We should get input from UX on this important permission. It can stay for now.
Attached patch 1219832.patchSplinter Review
Attachment #8680870 - Attachment is obsolete: true
Attachment #8681008 - Flags: review?(MattN+bmo)
(In reply to Bill Maggs (bmaggs) from comment #7)
> I agree with Tanvi. We should get input from UX on this important
> permission.

No rationale was given for why the 2 related strings shouldn't be unified.

> It can stay for now.

I don't want to uplift another bug so we should sort it out once and for all now and this needs to land ASAP.
Flags: needinfo?(wmaggs)
Attachment #8681008 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8681008 [details] [diff] [review]
1219832.patch

Approval Request Comment
[Feature/regressing bug #]: Notifications
[User impact if declined]: Confusing string about where the notifications is coming from and/or going to.
[Describe test coverage new/current, TreeHerder]: N/A string change
[Risks and why]: Low risk string change which would very obviously fail if done wrong
[String/UUID change made/needed]: yes, string-change for clarity
Attachment #8681008 - Flags: approval-mozilla-aurora?
Don't know why this had leave-open… probably from cloning.
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(wmaggs)
Keywords: leave-open
Resolution: --- → FIXED
Component: DOM: Push Notifications → Preferences
Product: Core → Firefox
Comment on attachment 8681008 [details] [diff] [review]
1219832.patch

This one had a blanket approval.
Attachment #8681008 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Stefan Plewako [:stef] from comment #18)
> https://hg.mozilla.org/mozilla-central/file/f8b569906e4c/browser/locales/en-
> US/chrome/browser/sitePermissions.properties#l11

Putting a link without any explanation is not really useful. As explained in the past, please file new bugs if you find issues, reopening existing bugs, especially with this much tracking and landing, doesn't help.
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
(In reply to Francesco Lodolo [:flod] from comment #19)
> Putting a link without any explanation is not really useful.

Sorry for reopening this one. Filling new bugs and commenting in existing doesn't seem useful anymore too.
With given link anyone interested will be able to spot the problem and fix it.
You need to log in before you can comment on or make changes to this bug.