998787 - Rename options in Geolocation doorhanger notification to disambiguate "Don't Share" vs. "Not Now"

Status: RESOLVED FIXED

(SeaMonkey :: Passwords & Permissions, defect)

Description

[rsx11m]

Firefox removed it as part of bug 615474 stating "got rid of geolocation.dontShareLocation (since it's not used anywhere)".

The "Not Now" option is the equivalent action, not doing anything.
See bug 494424 attachment 8409454 [details] for the current design.

I don't think that we need to change the other strings, given that the main button label reads "Share Location" already the "Location" in the menuitems feel unnecessary.

Thus, proposed new layout of the doorhanger menu:

  [Share Location v]
   +---------------+
   | Always Share  |
   | Never Share   |
   |---------------|
   | Not Now    [x]|
   +---------------+

Comment 1

[rsx11m]

(In reply to [e-mail] from bug 494424 comment #3)
> (In reply to rsx11m from bug 494424 comment #1)
> > I've noticed that "Don't Share" seems to be doing the same thing as "Not Now"
> "Not Now" wasn't originally part of the popup notification, it got added later.
> 
> (I can't remember whether either or both of them cancel the notification or just close it.)

The notifications.xml code calls cancelCallback(false); for dontShareLocation. It's not clear to me what "Not Now" does (I see xbl:inherits="oncommand=closeitemcommand" which seems to be handled in the Toolkit code), but if Firefox says that they are equivalent it's hopefully safe to assume that.

Comment 2

[rsx11m]

Attachment: Proposed patch

Here the patch modifying the binding. It removes the "Don't share" item and moves "Always Share" up to get the logical Allow/Deny order. In a Private Browsing window, the "Always" options remain hidden, thus "Not Now" will be the only option here.

Note that in contrast to Firefox, dontShareLocation can't be removed as a property given that the differently designed conventional notification bar still uses it (i.e., browser.doorhanger.enabled is set to false).

While the patch applies on current trunk, this was tested in a SM 2.25 build due to bug 903439.

Comment 3

[[e-mail]]

(In reply to rsx11m from comment #1)
> (In reply to bug 494424 comment #3)
> > (In reply to rsx11m from bug 494424 comment #1)
> > > I've noticed that "Don't Share" seems to be doing the same thing as "Not Now"
> > "Not Now" wasn't originally part of the popup notification, it got added later.
> > 
> > (I can't remember whether either or both of them cancel the notification or just close it.)
> 
> The notifications.xml code calls cancelCallback(false); for
> dontShareLocation. It's not clear to me what "Not Now" does (I see
> xbl:inherits="oncommand=closeitemcommand" which seems to be handled in the
> Toolkit code), but if Firefox says that they are equivalent it's hopefully
> safe to assume that.

cancelCallback(false) eventually calls back to the original geolocation getCurrentPosition request's errorCallback function. "Not Now" appears to simply hide the doorhanger, but leaving it available to allow later.

Comment 4

[Ian Neal]

Do you have an easy way for me to test this?

Comment 5

[rsx11m]

As said, I've cheated a little and actually tested the patch with a 2.25 build which still presents the doorhanger. I don't know when and by which mechanism it was switched off on trunk, but it didn't show up regardless of any pref setting I'm aware of ("Share location" permissions were at "Always ask").

There is a demo at https://developer.mozilla.org/en-US/demos/detail/geolocation/launch which I've used to get the prompt with that build. And, geo.enabled obviously has to be set to true for it to work (that pages also triggers the "unsafe content" warning which you may need to confirm or disable).

Comment 6

[rsx11m]

IanN: Does this work for you now?

Comment 7

[Ian Neal]

Comment on attachment 8411478 [details] [diff] [review]
Proposed patch

Code seems fine to me r+
Are there any other doorhangers that need similar work?

Comment 8

[rsx11m]

Don't know, but I'll file a new bug if I find one.

Comment 9

[rsx11m]

Nei: Any opinion on this?

Comment 10

[rsx11m]

s/Nei/Neil/ (pending ui-r? for 6 weeks now...)

Comment 11

[rsx11m]

(In reply to Ian Neal from comment #7)
> Are there any other doorhangers that need similar work?

There is only one more very similar case in the Desktop Notifications that I could find in notification.xml; I've opened bug 1019986 for that instance.

Comment 12

[[e-mail]]

(In reply to rsx11m from comment #5)
> There is a demo at
> https://developer.mozilla.org/en-US/demos/detail/geolocation/launch which
> I've used to get the prompt with that build. And, geo.enabled obviously has
> to be set to true for it to work (that pages also triggers the "unsafe
> content" warning which you may need to confirm or disable).

I also used 2.25 to try the demo at https://developer.mozilla.org/en/docs/WebAPI/Using_geolocation (near the end of the page) and "Not Now" just leaves the page waiting, while "Don't Share" lets the page know that geolocation is unavailable.

Comment 13

[rsx11m]

So, how is Firefox doing it in this case? Is there some magic on their end that associates the "Not Now" selection with the "Don't Share" item?

http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/nsBrowserGlue.js#1895 looks quite different from the SeaMonkey part, thus I can't tell what cancelling actually does. Judging from the bug 1019986 case, the "Not Now" for Desktop Notifications behaves the same in FF and SM, asking for permission again during the same session, thus I'd assume the same happens for the "Don't Share" vs. "Not Now" selection here too.

Comment 14

[[e-mail]]

(In reply to rsx11m from comment #5)
> As said, I've cheated a little and actually tested the patch with a 2.25
> build which still presents the doorhanger. I don't know when and by which
> mechanism it was switched off on trunk, but it didn't show up regardless of
> any pref setting I'm aware of ("Share location" permissions were at "Always
> ask").

Oops, looks like bug 853356 changed the request handed to the content permission prompt, so we don't recognise it.

(In reply to rsx11m from comment #13)
> So, how is Firefox doing it in this case? Is there some magic on their end
> that associates the "Not Now" selection with the "Don't Share" item?

No, they have the same behaviour; "Not Now" just leaves the page waiting in ignorance.

Comment 15

[[e-mail]]

Comment on attachment 8411478 [details] [diff] [review]
Proposed patch

So, I have to say, I don't really want to remove the "Don't Share" option. Sorry for taking far too long to get around to this. (I'm annoyed at myself too of course as it means I didn't discover the fallout from bug 853356.)

Comment 16

[rsx11m]

Ok, thus let's look into the alternatives given that the menu is similarly confusing as the Desktop Notification ones which I'm covering in bug 1019986. Simply rephrasing the labels may not help, though.

The other option would be to just remove the "Not Now" option entirely, which would still leave the user with the explicit "Do Nothing"-[x] in the notification, leaving the menu itself as just

  [Share Location v]
   +---------------+
   | Don't Share   |
   | Always Share  |
   | Never Share   |
   +---------------+

or

  [Share Location v]
   +---------------+
   | Always Share  |
   | Never Share   |
   | Don't Share   |
   +---------------+

with the "Not Now"-like option at the bottom.

http://mxr.mozilla.org/comm-central/source/suite/common/bindings/notification.xml#3321 should be the spot to modify. I'd still swap the items within the menu to have some consistency.

Comment 17

[rsx11m]

(In reply to [e-mail] from comment #14)
> Oops, looks like bug 853356 changed the request handed to the content
> permission prompt, so we don't recognise it.

Ok, this would be important for bug 1020630 for the Desktop Notifications (i.e., fixing it there may also fix it for the Geolocation prompt, or at least could serve as a template).

Comment 18

[rsx11m]

Maybe the "Not Now" label is the one causing the confusion? If it would be clear that it's affecting the dialog only without telling anything to the website, this should be less ambiguous:

  [Share Location v]
   +---------------+
   | Don't Share   |
   | Always Share  |
   | Never Share   |
   +---------------+
   | Dismiss   [x] |
   +---------------+

Comment 19

[rsx11m]

In analogy to bug 1019986 comment #3, alternative strings for the action items here might be:

  [Share Location v]
   +-----------------------+
   | Not for This Request  |
   | Always for This Site  |
   | Never for This Site   |
   |-----------------------|
   | Dismiss           [x] |
   +-----------------------+

making it clear that something is happening with the "Request" whereas "Dismiss" doesn't involve any action at all (or maybe label it "Close" instead?).

Annoyingly, renaming that specific item would require to add a new suite/.../notification.dtd file just for that single string, given that "Not Now" is defined in toolkit/'s version. But anyway, if it avoids potential confusion it should be worth it.

Comment 20

[rsx11m]

Attachment: Proposed patch (v2)

This implements comment #19 with clarified labels, similar to bug 1019986 attachment 8436327 [details] [diff] [review]. Also, the last item is renamed from "Not Now" to "Dismiss Notification" (rather than just "Dismiss") to be explicit that it affects the dialog but not the sharing request.

Comment 21

[rsx11m]

Attachment: Screenshot (v2)

For testing on trunk, remember to apply bug 1020630 attachment 8434563 [details] [diff] [review] as well.

Comment 22

[[e-mail]]

Comment on attachment 8436423 [details]
Screenshot (v2)

(Oops, we're missing the close button for this and the addon progress notification!)

Comment 23

[[e-mail]]

Comment on attachment 8436422 [details] [diff] [review]
Proposed patch (v2)

>diff --git a/suite/locales/en-US/chrome/common/notification.dtd b/suite/locales/en-US/chrome/common/notification.dtd
>new file mode 100644
>--- /dev/null
>+++ b/suite/locales/en-US/chrome/common/notification.dtd
>@@ -0,0 +1,10 @@
>+<!-- This Source Code Form is subject to the terms of the Mozilla Public
>+   - License, v. 2.0. If a copy of the MPL was not distributed with this
>+   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>+
>+<!-- LOCALIZATION NOTE (dismissNotificationItem.label): Use this entity
>+     for instances where Toolkit's default closeNotificationItem.label
>+     ("Not Now") may be ambiguous. Thus, make sure to select a phrase
>+     which clearly relates to closing the current doorhanger. -->
>+
>+<!ENTITY dismissNotificationItem.label "Dismiss Notification">
Given the number of strings involved, it might be better to override the global notification.dtd so that we can replace the label on all notifications.

Comment 24

[rsx11m]

Attachment: Proposed patch (v3)

(In reply to [e-mail] from comment #23)
> Given the number of strings involved, it might be better to override the
> global notification.dtd so that we can replace the label on all notifications.

Done, but this only affects the 3 instances where we extend Toolkit's popup-notification.

(In reply to [e-mail] from comment #22)
> (Oops, we're missing the close button for this and the addon progress notification!)

Found it, they were missing the close-icon class, hence the icon wasn't shown.

Comment 25

[rsx11m]

Attachment: Proposed patch (v4)

(In reply to rsx11m from comment #24)
> (In reply to [e-mail] from comment #23)
> > Given the number of strings involved, it might be better to override the
> > global notification.dtd so that we can replace the label on all notifications.
> 
> Done, but this only affects the 3 instances where we extend Toolkit's popup-notification.

Ok, I've peeked into Thunderbird's locale manifest and some jar.mn magic does the trick.

Comment 26

[rsx11m]

Attachment: Proposed patch (v5)

Thanks Neil. I've noticed that the localization note in notification.dtd needs updating, given that the label is used now for all notifications and not just that single instance. No other modifications.

Comment 27

[Ian Neal]

Comment on attachment 8436548 [details] [diff] [review]
Proposed patch (v5)

>+++ b/suite/common/bindings/notification.xml
>@@ -1,16 +1,16 @@
> <?xml version="1.0"?>
> <!-- This Source Code Form is subject to the terms of the Mozilla Public
>    - License, v. 2.0. If a copy of the MPL was not distributed with this
>    - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> 
> 
> <!DOCTYPE bindings [
>-<!ENTITY % notificationDTD SYSTEM "chrome://global/locale/notification.dtd">
>+<!ENTITY % notificationDTD SYSTEM "chrome://communicator/locale/notification.dtd">
> %notificationDTD;
If we're doing the override, do we need to make the above change?
Either way r=me

Comment 28

[rsx11m]

Probably not, but it can't hurt to be explicit that we want the new label for these specific instances, thus I'll keep it.

Push for comm-central, please.

Comment 29

[[e-mail]]

(In reply to rsx11m from comment #28)
> Probably not, but it can't hurt to be explicit that we want the new label
> for these specific instances, thus I'll keep it.

Personally I was leaning towards removing it because otherwise it's less obvious that the strings are the same for all doorhangers.

Comment 30

[rsx11m]

Attachment: Final patch (v6)

Ok, let's remove that change then and keep the reference to chrome://global/... .

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/ee278e65eacc