Allow `showHeartbeat` to change the icon

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: glind, Assigned: Fischer, Mentored)

Tracking

unspecified
Firefox 50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [lang=js][good first bug])

Attachments

(2 attachments)

Currently, all `showHeartbeat` doorhangers show the Heartbeat icon.  

We would like to be able to give more appropriate icons for other Shield related projects.

Gross solution:  add another param to the `showHeartbeat` options.  

something like: 
  - icon:  'firefox'|'heartbeat'|<url>
 
We could define aliases for a few 'common' choices.


--
Also, all of showHeartbeat is gross, but wow is it useful.

Thanks!
(MattN, kick it somewhere else if you need to.)
Assignee: nobody → MattN+bmo
This isn't a priority for me at the moment.
Assignee: MattN+bmo → nobody
How about if it just takes an optional URL? The default would be the heartbeat one but you could specify "chrome://branding/content/icon48.png" for the browser icon or any other URL?

Does this work?

https://mxr.mozilla.org/mozilla-central/source/browser/components/uitour/UITour.jsm?rev=2eb7b3fd4cc0#1155
The `icon` would be an optional property on the options argument at https://mxr.mozilla.org/mozilla-central/source/browser/components/uitour/UITour-lib.js?rev=8b57efc13045#101
Mentor: MattN+bmo
Flags: needinfo?(glind)
Whiteboard: [lang=js][good first bug]
I love that solution.  It could be a data url too, then?

Thanks for this!  It would help with with branding and making specific prompts easier.

Thanks!

GL
Can anyone provide a reproducible steps to check how it happens?
Code from a whitelisted domain[1] and with UITour-lib.js[2] can run: ```
Mozilla.UITour.showHeartbeat("Would you like to share feedback with Mozilla? No information will be shared unless you take the survey.",
"Thank you!",
"myflowid",
"http://www.mozilla.org/?fake=bar", null, null, {
  engagementButtonLabel: "Take Survey",
  icon: "chrome://branding/content/icon48.png",
});
```

From a Browser Console the equivalent is: ```
UITour.showHeartbeat(window, {
  message: "Would you like to share feedback with Mozilla? No information will be shared unless you take the survey.",
  thankyouMessage: "Thank you!",
  flowId: "myflowid",
  engagementButtonLabel: "Take Survey",
  engagementURL: "http://www.mozilla.org/",
  icon: "https://www.google.com/favicon.ico",
});
```

In this case it's probably easiest to test with browser/components/uitour/test/uitour.html in a browser hosted on localhost once you change the UITour-lib.js path to include "../" as a prefix.

[1] https://mxr.mozilla.org/mozilla-central/source/browser/app/permissions and the CSV pref browser.uitour.testingOrigins
[2] https://mxr.mozilla.org/mozilla-central/source/browser/components/uitour/UITour-lib.js
Hi, Matthew, just tried your Comment 6's code example. Kind of grab the direction. I would like to take this as my good 1st bug, thanks :)
Assignee: nobody → fliu
Hi Matthew,

Would you please see the patch and provide some feedbacks ?

(1) I am not sure we should check if the optional icon URL could only be “chrome://“. Currently the patch doesn’t do this kind of checking.

(2) For testing, currently the patch checks if the optional icon URL does being set to the notification UI element’s image. If yes, then it passes the test. Please let me know if there is a better way for checking.

(3) I am not sure what a whitelisted domain[1] mentioned in Comment 6 is for. Currently I can successfully change the icon by specifying one optional icon url. And showing the default heartbeat icon if no optional icon url given. Is there any modification I should do about the whitelisted domain?

[1] https://mxr.mozilla.org/mozilla-central/source/browser/app/permissions and the CSV pref browser.uitour.testingOrigins


Thank you
Attachment #8757830 - Flags: feedback?(MattN+bmo)
Comment on attachment 8757830 [details] [diff] [review]
Bug 1229927 - Allow `showHeartbeat` to change the icon.patch

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

Hi Fischer,

Could you please include 8 lines of context for future patches (or use MozReview)? It makes it easier to review.

You'll also want to fix the user/author settings of HG as it's currently a local address.

(In reply to [:Fischer]Fischer from comment #8)
> (1) I am not sure we should check if the optional icon URL could only be
> “chrome://“. Currently the patch doesn’t do this kind of checking.

I was suggesting that we use `ensureTrustedOrigin` (which allows chrome://) but forgot that it's in the content process in content-UITour.js and we don't have shared tour code between the two processes yet. Since refactoring that so it's usable is non-trivial and I've since suggested moving showHeartbeat to a system extension (code name normandy), I think limiting to chrome URIs for now is fine.

> (2) For testing, currently the patch checks if the optional icon URL does
> being set to the notification UI element’s image. If yes, then it passes the
> test. Please let me know if there is a better way for checking.

That method is fine.

> (3) I am not sure what a whitelisted domain[1] mentioned in Comment 6 is
> for. Currently I can successfully change the icon by specifying one optional
> icon url. And showing the default heartbeat icon if no optional icon url
> given. Is there any modification I should do about the whitelisted domain?

See answer for #1

::: browser/components/uitour/UITour-lib.js
@@ -106,4 @@
>        flowId: flowId,
>        engagementURL: engagementURL,
>        learnMoreLabel: learnMoreLabel,
> -      learnMoreURL: learnMoreURL,

Please revert this. We prefer trailing commas on object and array literals so blame doesn't change when an entry is added after it.

::: browser/components/uitour/test/browser_UITour_heartbeat.js
@@ +231,5 @@
> +  validateTimestamp('Heartbeat:Offered', data.timestamp);
> +
> +  // Check the icon URL
> +  let notification = getHeartbeatNotification(flowId);
> +  is(notification.image, iconURL, 'The optional icon URL is not taken correctly');

Nit: please use double-quotes for most JS strings in mozilla-central.
Attachment #8757830 - Flags: feedback?(MattN+bmo) → feedback+
Comment on attachment 8760636 [details]
Bug 1229927 - Allow `showHeartbeat` to change the icon.

Cancel this auto-generated review request since the commit message still needs change.
Attachment #8760636 - Flags: review?(MattN+bmo)
Comment on attachment 8760636 [details]
Bug 1229927 - Allow `showHeartbeat` to change the icon.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58174/diff/1-2/
Attachment #8760636 - Attachment description: Bug 1229927 - Allow to change the icon; → Bug 1229927 - Allow `showHeartbeat` to change the icon.
Attachment #8760636 - Flags: review?(MattN+bmo)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/2996589ecd4e
Allow `showHeartbeat` to change the icon. r=MattN
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2996589ecd4e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
You need to log in before you can comment on or make changes to this bug.