967349 - PopupNotifications should support a "Learn More" link

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

When a notification's purpose might be unclear to the user, we sometimes show a "Learn More…" link in the dialog to help the user make an informed choice. This is a bit of a pain, even with <popupnotificationcontent>. It would be nice if PopupNotification callers could simply provide a URL through the API, and might help encourage people adding new permission prompts to make use of this.

Attached is a patch to do this, along with fixup of a number of existing callers to use the new API.

Comment 1

[Justin Dolske [:Dolske]]

Comment on attachment 8369831 [details] [diff] [review]
Patch v.1


>--- a/browser/base/content/browser.js
...
>     <popupnotification id="servicesInstall-notification" hidden="true">
>       <popupnotificationcontent orient="vertical" align="start">
>-        <separator class="thin"/>
>-        <label id="servicesInstall-learnmore-link" class="text-link"/>
>+        <!-- XXX tests are looking for this, can't remove yet? -->
>       </popupnotificationcontent>
>     </popupnotification>

A bunch of SocialAPI tests are doing |document.getElementById("servicesInstall-notification")|, and I was kind of loath to dive in and change them all, or discover they were relying on subtle persisted state or something. Not sure if I should just leave the useless <popupnotification>, or go change everything. Or maybe make it a followup for Shane? :)

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8369831 [details] [diff] [review]
Patch v.1

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

::: browser/base/content/popup-notifications.inc
@@ +38,3 @@
>      <popupnotification id="servicesInstall-notification" hidden="true">
>        <popupnotificationcontent orient="vertical" align="start">
> +        <!-- XXX tests are looking for this, can't remove yet? -->

Reminder to do something

@@ -45,3 @@
>      <popupnotification id="servicesInstall-notification" hidden="true">
>        <popupnotificationcontent orient="vertical" align="start">
> -        <separator class="thin"/>

This is going to be a visual change. Does it look fine? Should this be moved to the popup-notification binding so all notifications with a learn more link get this?

@@ -59,5 @@
>      <popupnotification id="mixed-content-blocked-notification" hidden="true">
>        <popupnotificationcontent orient="vertical" align="start">
>          <separator/>
>          <description id="mixed-content-blocked-moreinfo">&mixedContentBlocked.moreinfo;</description>
> -        <separator/>

Ditto

::: browser/components/nsBrowserGlue.js
@@ +1904,5 @@
> +      if (!aOptions)
> +        aOptions = {};
> +
> +      aOptions.removeOnDismissal = autoAllow;
> +      aOptions.eventCallback = function(type) {

y u no liek arrow functions?

So this is so we don't clobber existing options when the type is "pointerLock" but as far as I can tell, this changes doesn't really need to happen in this patch as you're not adding a learn more link in this case. Is this to fix a different bug or just a forward-looking fix?

FYI this hunk has now bitrotten due to nearby changes. Sorry about that.

@@ +1970,5 @@
>          },
>        });
>      }
>  
> +    var options = { learnMoreURL: Services.urlFormatter.formatURLPref("browser.geolocation.warning.infoURL") };

Nit: I would prefer learnMoreURL on its own line to help with blame when another options gets added. Be sure to add the trailing comma.

::: toolkit/components/social/SocialService.jsm
@@ +573,5 @@
>          aAddonInstaller.install();
>        },
>      };
>  
> +    let options = { learnMoreURL: Services.urlFormatter.formatURLPref("app.support.baseURL") + "social-api" };

Same comment as nsBrowserGlue.js: I'd like learnMoreURL on a new line with the trailing comma.

::: toolkit/content/widgets/notification.xml
@@ +455,5 @@
>        <xul:vbox flex="1">
>          <xul:description class="popup-notification-description"
>                           xbl:inherits="xbl:text=label"/>
>          <children includes="popupnotificationcontent"/>
> +        <xul:label class="text-link popup-notification-learnmore-link"

Why after <children> instead of before? I can the learn more link wanting to follow the description sometimes but follow the popupnotificationcontent other times. Just want to make sure you've thought about this.

::: toolkit/locales/en-US/chrome/global/notification.dtd
@@ +7,5 @@
>  <!ENTITY closeNotificationItem.label "Not Now">
>  
>  <!ENTITY checkForUpdates "Check for updates…">
> +
> +<!ENTITY learnMore "Learn more…">

Perhaps this entity name is too generic and should be more specific to notifications both because of it's location in this file and so localizers can localize it appropriately for the context.
e.g. notification.learnMore

Also, shouldn't we preserve the localization note about the ellipsis?

::: toolkit/modules/PopupNotifications.jsm
@@ +261,5 @@
>     *                     Normally specified in CSS using list-style-image and the
>     *                     .popup-notification-icon[popupid=...] selector.
> +   *        learnMoreURL:
> +   *                     A string URL. Setting this property will make the
> +   *                     prompt displaya "Learn More" link that, when clicked,

Nit: missing space at "displaya"

::: toolkit/themes/linux/global/notification.css
@@ +58,3 @@
>  }
>  
>  /* Popup notification */

Ahem, toolkit/themes/shared would be nice ;)

@@ +62,5 @@
>  .popup-notification-description {
>    max-width: 24em;
>  }
>  
> +.popup-notification-learnmore-link {

Is the "-moz-margin-start: 0;" not necessary anymore for this link on Linux?

browser/themes/linux/browser.css was setting it

@@ +63,5 @@
>    max-width: 24em;
>  }
>  
> +.popup-notification-learnmore-link {
> +  margin-top: 1em !important;

I find it weird that global.css applies after chrome://global/skin/notification.css (thus making the !important necessary) but I don't know much about XBL <stylesheet> elements.

Comment 3

[Justin Dolske [:Dolske]]

(In reply to Matthew N. [:MattN] from comment #2)

> > +        <!-- XXX tests are looking for this, can't remove yet? -->
> 
> Reminder to do something

Filed bug 974146.

> 
> @@ -45,3 @@
> >      <popupnotification id="servicesInstall-notification" hidden="true">
> >        <popupnotificationcontent orient="vertical" align="start">
> > -        <separator class="thin"/>
> 
> This is going to be a visual change. Does it look fine? Should this be moved
> to the popup-notification binding so all notifications with a learn more
> link get this?

The |margin-top: 1em| effectively replaces it, looks the same to me.


> ::: browser/components/nsBrowserGlue.js
> @@ +1904,5 @@
> > +      if (!aOptions)
> > +        aOptions = {};
> > +
> > +      aOptions.removeOnDismissal = autoAllow;
> > +      aOptions.eventCallback = function(type) {
> 
> y u no liek arrow functions?

Hmm, not sure why I changed that. Maybe because "foo = bar => baz { }" read a little weird? Changed it back.
  
> So this is so we don't clobber existing options when the type is
> "pointerLock" but as far as I can tell, this changes doesn't really need to
> happen in this patch as you're not adding a learn more link in this case. Is
> this to fix a different bug or just a forward-looking fix?

Just a forward-looking fix since I noticed it.


> ::: toolkit/content/widgets/notification.xml
> @@ +455,5 @@
> >        <xul:vbox flex="1">
> >          <xul:description class="popup-notification-description"
> >                           xbl:inherits="xbl:text=label"/>
> >          <children includes="popupnotificationcontent"/>
> > +        <xul:label class="text-link popup-notification-learnmore-link"
> 
> Why after <children> instead of before? I can the learn more link wanting to
> follow the description sometimes but follow the popupnotificationcontent
> other times. Just want to make sure you've thought about this.

I suppose it could go either way in theory, but existing uses of <popupnotificationcontent> seem to want it after.


> >  <!ENTITY checkForUpdates "Check for updates…">
> > +
> > +<!ENTITY learnMore "Learn more…">
> 
> Perhaps this entity name is too generic and should be more specific to
> notifications both because of it's location in this file and so localizers
> can localize it appropriately for the context.
> e.g. notification.learnMore
> 
> Also, shouldn't we preserve the localization note about the ellipsis?

I don't think it's any different than the line right above it.


> @@ +62,5 @@
> >  .popup-notification-description {
> >    max-width: 24em;
> >  }
> >  
> > +.popup-notification-learnmore-link {
> 
> Is the "-moz-margin-start: 0;" not necessary anymore for this link on Linux?

Correct. Only OS X has differing margin for label vs description (see global.css).

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Fixed nits.

Comment 5

[Justin Dolske [:Dolske]]

https://hg.mozilla.org/integration/fx-team/rev/8ffd0d235aab

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8ffd0d235aab

Comment 7

[Francesco Lodolo [:flod]]

Any specific reason for keeping an empty file around?
http://hg.mozilla.org/mozilla-central/file/84c9885475e7/mobile/android/locales/en-US/chrome/notification.dtd