1111027 - UITour: implement heartbeat UI as a UITour function

Status: RESOLVED FIXED

(Firefox :: Tours, enhancement)

Description

[Benjamin Smedberg]

This bug tracks implementing the heartbeat star UI as a UITour function. The UI spec is tracked by bug 1092376. Gregg or Sevaan, since that bug has several UI options, can you clarify which one we're going with for release? Also Gregg since you already implemented this UI for the telemetry experiment, can you provide a link to that code so that the ultimate owner this bug can reuse it?

Comment 1

[Benjamin Smedberg]

More details about UITour:

Firefox client implementation: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/content-UITour.js and http://mxr.mozilla.org/mozilla-central/source/browser/modules/UITour.jsm?force=1

Usage library from Mozilla websites (translates normal JS into events and back again): https://github.com/mozilla/bedrock/blob/master/media/js/firefox/australis/australis-uitour.js

Comment 2

[Justin Dolske [:Dolske]]

I don't understand what this has to do with UITour. What page is triggering the notificationbar-esqu UI from bug 1092376?

Comment 3

[Benjamin Smedberg]

The self-support/heartbeat page from bug 1111022.

Comment 4

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

Attachment: Screen shot showing 'stars' upper notifiication bar.

This is showing the 'stars' notification bar.

Comment 5

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

UI (as built) will be the 'upper global notification bar', with 'new styling'.

Claim 1:  In V1 it is acceptable to offer the 'stars' version.

Claim 2:  In future versions, the NPS should also be offered.  

Claim 3:  It might be wise to future plan for that now, depending on where one wants to eat the development cost.  


(All claims here to be approved by Sevaan).

There are at least 3 UI problems with this current bar implementation:
1.  Modification of global bar comes too early.  (to fix this, bars or notifications might signal 'about to show' https://bugzilla.mozilla.org/show_bug.cgi?id=1111708)
2.  'breaking' on small screens isn't perfect    (no other bars to this right either, recommend WONTFIX)
3.  No notification bars show during fullscreen mode. (I claim this comes from narrative confusion about presentation mode vs full screen.  https://bugzilla.mozilla.org/show_bug.cgi?id=1111705)

Comment 6

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

Existing code:  https://github.com/gregglind/firefox-pulse/blob/master/heartbeat-telemetry-experiment-1/lib/ui/question-bars.js

I am in process of moving more of it to css.

Comment 7

[Sevaan Franks [:sevaan]]

(In reply to Gregg Lind (User Advocacy - Heartbeat - Test Pilot) from comment #5)
> (All claims here to be approved by Sevaan).
> 
> There are at least 3 UI problems with this current bar implementation:
> 1.  Modification of global bar comes too early.  (to fix this, bars or
> notifications might signal 'about to show'
> https://bugzilla.mozilla.org/show_bug.cgi?id=1111708)

Not sure I fully understand this first one. Do you mind clarifying further in the Bug? But yes to the others.

Comment 8

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

Attachment: Screen Shot 2014-12-16 at 2.36.06 PM.png

Demonstration of how styling can 'leak'.

1.  Create and style notification
2.  Higher priority message is open. 
3.  Some of the styling (height for example) can 'leak'

Comment 9

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

https://bugzilla.mozilla.org/show_bug.cgi?id=1092376

Comment 10

[Alessio Placitelli [:Dexter]]

Attachment: Heartbeat UI WIP

This is a WIP version of the UITour Heartbeat UI. Thanks Gregg, MattN for your extremely valuable help and input!

What's in there:
- UITour is able to show the stars version of the Heartbeat UI. This should be working on all platforms, even though I just tested on my machine (which is on Windows).
- Animations are in place.
- An engagement page is opened in a new tab when user rates.

What's missing:
- Input parameter validation.
- Phonehome features.
- NPS style (even though it should be reasonably easy to add using CSS styling).

What's next:
- Use XUL Buttons instead of XUL Images to have accessibility features. This requires a bit of tinkering since Buttons should be "style reset" first and then completely restyled.
- Implement the phonehome features.
- Write proper tests as discussed with Gregg.
- Check if the Heartbeat UI/UITour doesn't really work with e10s.
- Implement the NPS style.

Comment 11

[Benjamin Smedberg]

What does "phonehome" mean?

Comment 12

[Alessio Placitelli [:Dexter]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #11)
> What does "phonehome" mean?

It's the name of the component, in the experiment's code, which tracks the rating status by sending messages to Mozilla's servers (i.e., the notification bar was shown, it was closed, etc.). It basically sends a message at each step.

Comment 13

[Georg Fritzsche [:gfritzsche]]

I assume this will break down into two parts:
* possible submissions from the remote website that we loaded (not relevant to the client-side implementation) and
* submission of rating/etc. into FHR/Telemetry
... right?

Comment 14

[Benjamin Smedberg]

The phonehome code is not part of this bug. That should be implemented by the web site which uses this API.

Comment 15

[Alessio Placitelli [:Dexter]]

Attachment: Heartbeat UI WIP 2

This enables the caller page to track the rating status ("phonehome") by providing status updates through a callback.

I've also added some mochitests. This patch is still missing XUL buttons, but they will come in soon.

Comment 16

[Alessio Placitelli [:Dexter]]

Attachment: Heartbeat UI WIP 3

At last, a version of the Heartbeat UI using XUL Toolbar Buttons instead of XUL Images.

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8541249 [details] [diff] [review]
Heartbeat UI WIP 3

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

Reminder to test on Retina as we discussed.

I reviewed everything but the test file since it's going to change a bit anyways with the callback => notify change and there are enough comments here for now ;)

::: browser/modules/UITour.jsm
@@ +391,5 @@
>  
> +      case "showHeartbeat": {
> +        // Validate the input parameters.
> +        if (data.type !== "stars") {
> +          log.warn("showHeartbeat: Invalid heartbeat UI type specified.");

Why bother even passing data.type if there is only one value allowed? We can add this check once we actually support multiple types.

@@ +396,5 @@
> +          break;
> +        }
> +
> +        if (typeof data.message !== "string" ||
> +            data.type === "") {

I don't think `data.type === ""` can be true since it's always "stars". Did you mean to check data.message?

Nit: It seems like you are wrapping unnecessarily.

@@ +402,5 @@
> +          break;
> +        }
> +
> +        if (typeof data.flowId !== "string" ||
> +            data.flowId === "") {

Nit: early wrapping

@@ +415,5 @@
> +        }
> +
> +        if (typeof data.callbackID !== "string" ||
> +            data.callbackID === "") {
> +          log.warn("showHeartbeat: Invalid callback ID specified.");

I think all of the log.warn above should be log.error though I can understand why you used log.warn as others in this file are using warnings for invalid data. I think in cases where the bad data leads to a `break` we should probably do log.error since Error is the default log level and passing invalid data should be considered an error IMO.

@@ +421,5 @@
> +        }
> +
> +        // Finally show the Heartbeat UI.
> +        this.showHeartbeat(window, messageManager, data.type, data.message, data.flowId,
> +          data.engagementURL, data.callbackID);

Nit: align the arguments on the second line with the first one to the method (`window`).

@@ +1054,5 @@
>  
>    /**
> +   * Show the Heartbeat UI to request user feedback. This function reports back to the
> +   * caller using |sendPageCallback| through a passed in callback. The callback argument
> +   * data contains the |state| field (admissible values are "offered", "voted", "closed")

Update this comment once we stop using callbacks.

@@ +1059,5 @@
> +   * and a |score| field which only holds a valid value when |state| is "voted".
> +   * Please note that input parameters are already validated by the caller.
> +   *
> +   * @param aWindow
> +   *        The chrome window that the heartbeat notification is displayed in.

Nit: rename aWindow to aChromeWindow. In all new code in this module I'm trying to be more explicit about what type of window is being referred to. I didn't clean up existing "window" variable names.

@@ +1071,5 @@
> +   *        The message, or question, to display on the notification.
> +   * @param aFlowId
> +   *        An identifier for this rating flow. Please note that this is only used to
> +   *        identify the notification box.
> +   * @param aEngagementUrl

aEngagementURL is the more common style

@@ +1076,5 @@
> +   *        The engagement URL to open in a new tab once user has voted.
> +   * @param aCallbackID
> +   *        The callback to use when reporting the status back.
> +   */
> +  showHeartbeat: function(aWindow, aMessageManager, aType, aMessage, aFlowId,

Nit: In some ways it would be nice if this was in a separate self-support JSM that was lazily loaded from this module since it's unlikely kind of separate from things that will be used from actual tours and having it together with the rest of the self-support code will be nicer IMO. Perhaps you could rename SelfSupportBackend.jsm in your other patch to SelfSupport.jsm and put it there.

The downside is that it's creates a circular dependency between the two modules as this method needs to call UITour.notify so I think it's better to leave it here after all.

@@ +1086,5 @@
> +    let notice = nb.appendNotification(aMessage, aFlowId,
> +      "chrome://branding/content/icon64.png", nb.PRIORITY_INFO_HIGH, null, function() {
> +        // Let the consumer know the notification bar was closed. This also happens
> +        // after voting.
> +        context.sendPageCallback(aMessageManager, aCallbackID, {state: "closed"});

Use something like this.notify("Heartbeat:NotificationClosed") instead so we don't have to re-use the callback for multiple purposes.

@@ +1087,5 @@
> +      "chrome://branding/content/icon64.png", nb.PRIORITY_INFO_HIGH, null, function() {
> +        // Let the consumer know the notification bar was closed. This also happens
> +        // after voting.
> +        context.sendPageCallback(aMessageManager, aCallbackID, {state: "closed"});
> +      });

Use .bind(this) instead of `context`. We usually prefer bind over temporary variables referencing `this` from other scopes nowadays.

Nit: Outdent this line to align with the left of the `let`.

@@ +1093,5 @@
> +    // Get the elements we need to style.
> +    let messageImage =
> +      aWindow.document.getAnonymousElementByAttribute(notice, "class", "messageImage");
> +    let messageText =
> +      aWindow.document.getAnonymousElementByAttribute(notice, "class", "messageText");

Use "anonid" instead of "class" as "anonid" is made for this purpose and is less likely to change with styling in the future.

@@ +1099,5 @@
> +    // Create the fragment holding the rating UI.
> +    let frag = aWindow.document.createDocumentFragment();
> +
> +    // Build the Heartbeat star rating.
> +    let numElements = 5;

Nit: numStars would be more descriptive and you can make it a `const`

@@ +1104,5 @@
> +    let ratingContainer = aWindow.document.createElement("hbox");
> +    ratingContainer.id = "star-rating-container";
> +
> +    for (let i = 0; i < numElements; i++) {
> +      // Create a rating element (star or NPS entry).

I think this comment is stale as I believe we aren't implementing NPS atm.

@@ +1110,5 @@
> +
> +      // Style it.
> +      ratingElement.className = "plain star-x";
> +      ratingElement.id = "star" + (i + 1);
> +      ratingElement.setAttribute("data-score", numElements - i);

Why the mismatch in the above 2 lines between the @id and the @data-score. E.g. the element with @id=star2 has @data-score=4. Why not use the same number for both?

@@ +1115,5 @@
> +
> +      // Add the click handler.
> +      ratingElement.addEventListener("click", function (evt) {
> +        let rating =
> +          Number(evt.target.getAttribute("data-score"), 10);

Nit: no need to wrap here. You can go up to column 100.

@@ +1118,5 @@
> +        let rating =
> +          Number(evt.target.getAttribute("data-score"), 10);
> +
> +        // Let the consumer know user voted.
> +        context.sendPageCallback(

Use .bind(this) instead of `context` here too. Also use this.notify e.g.
this.notify("Heartbeat:Voted", { score: rating })

@@ +1135,5 @@
> +        }
> +
> +        // Append the score data to the engagement URL.
> +        let engagementUrl =
> +          aEngagementUrl + "?type=" + aType + "&score=" + rating + "&flowid=" + aFlowId;

This won't handle if there are existing query parameters on the URL. We have WebAPIs to do encoding and appending arguments for you nowadays which you should use:

let engagementURL = new URL(aEngagementURL);
engagementURL.searchParams.append("type", aType);
engagementURL.searchParams.append("score", rating);
…

See https://developer.mozilla.org/en-US/docs/Web/API/URL and https://developer.mozilla.org/en-US/docs/Web/API/URLUtils.searchParams

@@ +1138,5 @@
> +        let engagementUrl =
> +          aEngagementUrl + "?type=" + aType + "&score=" + rating + "&flowid=" + aFlowId;
> +
> +        // Open the engagement URL in a new tab.
> +        let engagementTab = aWindow.gBrowser.addTab(engagementUrl, {

I initially thought the hidden page was going to window.open the tab. Did that cause problems? I suppose it may open a new window instead or fail with the windowless browser

@@ +1142,5 @@
> +        let engagementTab = aWindow.gBrowser.addTab(engagementUrl, {
> +          owner: aWindow.gBrowser.selectedTab,
> +          relatedToCurrent: true
> +        });
> +        aWindow.gBrowser.selectedTab = engagementTab;

Nit: You can combine the previous two statements together to avoid the tempoary variable:

aWindow.gBrowser.selectedTab = aWindow.gBrowser.addTab(engagementUrl, {…

@@ +1146,5 @@
> +        aWindow.gBrowser.selectedTab = engagementTab;
> +
> +        // Remove the notification bar after 3 seconds.
> +        aWindow.setTimeout(() => {
> +          nb.removeCurrentNotification();

I don't think you should assume that your notification is the current one. Use nb.removeNotification(notice);

@@ +1158,5 @@
> +    frag.appendChild(ratingContainer);
> +
> +    // Make sure the stars are not pushed to the right by the spacer.
> +    let rightSpacer = aWindow.document.createElement("spacer");
> +    rightSpacer.setAttribute("flex", 20);

Whenever you have a choice between setting an attribute or property, use the property as it's more performant and I think sometimes does more validation:
rightSpacer.flex = 20;

@@ +1162,5 @@
> +    rightSpacer.setAttribute("flex", 20);
> +    frag.appendChild(rightSpacer);
> +
> +    let leftSpacer = messageText.nextSibling;
> +    leftSpacer.setAttribute("flex", 0);

ditto

@@ +1170,5 @@
> +    notice.classList.add("heartbeat");
> +    messageImage.classList.add("heartbeat", "pulse-onshow");
> +
> +    // Let the consumer know the notification was shown.
> +    this.sendPageCallback(aMessageManager, aCallbackID, {state: "offered"});

this.notify("Heartbeat:…

::: browser/modules/test/browser_UITour_heartbeat.js
@@ +54,5 @@
> +  function test_heartbeat_stars_show(done) {
> +    let flowId = 'heartbeat-ui-ratefirefox';
> +
> +    function callback(data) {
> +      switch(data.state) {

Nit: missing space after switch

::: browser/modules/test/uitour.js
@@ +38,5 @@
>  	function _generateCallbackID() {
>  		return Math.random().toString(36).replace(/[^a-z]+/g, '');
>  	}
>  
> +	function _waitForCallback(callback, keepListening=false) {

Nit: use spaces around operators per the Mozilla coding style.

I think we shouldn't use a callback if we want to get more than one message. We can use UITour.notify+Mozilla.UITour.observe for that instead. i.e. I don't think we should add the keepListening argument.

@@ +49,5 @@
>  				return;
>  
> +      if (!keepListening) {
> +        document.removeEventListener('mozUITourResponse', listener);
> +      }

The indentation here is a bit different but I also realize that you weren't the first to introduce a different indentation style.

@@ +99,5 @@
>  			pageID: pageID
>  		});
>  	};
>  
> +  Mozilla.UITour.showHeartbeat = function(type, message, flowId, engagementURL, callback) {

ditto

::: browser/themes/osx/jar.mn
@@ +34,5 @@
>    skin/classic/browser/Geolocation-16.png
>    skin/classic/browser/[e-mail]
>    skin/classic/browser/Geolocation-64.png
>    skin/classic/browser/[e-mail]
> +  skin/classic/browser/heartbeat-icon.png                   (../shared/heartbeat-icon.png)

We should probably have a @2x version for Retina or ideally switch to SVG

::: browser/themes/shared/UITour.inc.css
@@ +240,5 @@
> +
> +.messageImage.heartbeat {
> +  width: 36px;
> +  height: 36px;
> +  margin-right: 10px;

To properly handle RTL, I think you want `-moz-margin-end: 10px;`. You can test with the ForceRTL extension.

@@ +247,5 @@
> +.messageImage.heartbeat.pulse-onshow {
> +  animation-name: pulse-onshow;
> +  animation-duration: 1.5s;
> +  animation-iteration-count: 1;
> +  animation-timing-function: cubic-bezier(.7, 1.8, .9, 1.1);

I suspect it's preferred to have remove the spaces after the commas to like the rgb[a]

@@ +283,5 @@
> +  background-repeat: no-repeat;
> +  background-size: cover;
> +  cursor: pointer;
> +  width:24px;
> +  height:24px;

Nit: missing spaces on the two lines above

@@ +284,5 @@
> +  background-size: cover;
> +  cursor: pointer;
> +  width:24px;
> +  height:24px;
> +  background: url("chrome://browser/skin/heartbeat-star-off.svg");

Nit: Move this up with the other background properties. Note that the ones above actually have no effect since this is overriding them.

@@ +294,5 @@
> +  background-size: cover;
> +  cursor: pointer;
> +  width:24px;
> +  height:24px;
> +  background: url("chrome://browser/skin/heartbeat-star-lit.svg");

ditto on the space after the colon and the `background`

Comment 18

[Alessio Placitelli [:Dexter]]

Attachment: Heartbeat UI v2

Comment 19

[Alessio Placitelli [:Dexter]]

(In reply to Matthew N. [:MattN] (away until Jan. 7) from comment #17)
> I reviewed everything but the test file since it's going to change a bit
> anyways with the callback => notify change and there are enough comments
> here for now ;)

Thank you for such an in depth review!
I've modified the patch according to your suggestions but not asked for a review, as I have a few doubts (see below).

> @@ +1138,5 @@
> > +        let engagementUrl =
> > +          aEngagementUrl + "?type=" + aType + "&score=" + rating + "&flowid=" + aFlowId;
> > +
> > +        // Open the engagement URL in a new tab.
> > +        let engagementTab = aWindow.gBrowser.addTab(engagementUrl, {
> 
> I initially thought the hidden page was going to window.open the tab. Did
> that cause problems? I suppose it may open a new window instead or fail with
> the windowless browser

It seems to fail (doesn't produce any new window or throw errors!).

> ::: browser/modules/test/uitour.js
> @@ +38,5 @@
> >  	function _generateCallbackID() {
> >  		return Math.random().toString(36).replace(/[^a-z]+/g, '');
> >  	}
> >  
> > +	function _waitForCallback(callback, keepListening=false) {
> 
> Nit: use spaces around operators per the Mozilla coding style.
> 
> I think we shouldn't use a callback if we want to get more than one message.
> We can use UITour.notify+Mozilla.UITour.observe for that instead. i.e. I
> don't think we should add the keepListening argument.

That's definitely a better idea! I've changed the implementation to reflect this suggestion. There's only one problem with the "NotificationClose" notification sent when the heartbeat UI is closed: if a new tab is opened before the |notify| is called (e.g. user votes, the engagement page gets opened), it fails at [1] because |originTabs| is null. Why is |sendPageCallback| taking the messagManager argument while |notify| isn't?

> ::: browser/themes/osx/jar.mn
> @@ +34,5 @@
> >    skin/classic/browser/Geolocation-16.png
> >    skin/classic/browser/[e-mail]
> >    skin/classic/browser/Geolocation-64.png
> >    skin/classic/browser/[e-mail]
> > +  skin/classic/browser/heartbeat-icon.png                   (../shared/heartbeat-icon.png)
> 
> We should probably have a @2x version for Retina or ideally switch to SVG

Good point! How can I request an SVG/@2x version?

[1] - https://dxr.mozilla.org/mozilla-central/source/browser/components/uitour/UITour.jsm#1693

Comment 20

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

Sevaan, SVG of the heartbeat icon would make retina + osx + windows problem go away :)  Thanks!

Comment 21

[Sevaan Franks [:sevaan]]

Attachment: Heartbeat Icon (.svg)

(In reply to Gregg Lind (User Advocacy - Heartbeat - Test Pilot) from comment #20)
> Sevaan, SVG of the heartbeat icon would make retina + osx + windows problem
> go away :)  Thanks!

Your wish is my command!

Comment 22

[Alessio Placitelli [:Dexter]]

Attachment: Heartbeat UI v3

Wow, that was fast! Thank you Sevaan, Gregg!

This new version includes the SVG and reports a timestamp in the state notification.

Matthew, can we add a new function to UITour to open a link in a new tab? We already have an |addPinnedTab| but having an |addTab| function could be useful to remove the engagement bits off the UI (thanks Gregg for the brainstorming!).

Comment 23

[Matthew N. [:MattN]]

(In reply to Alessio Placitelli [:Dexter] from comment #19)
> > ::: browser/modules/test/uitour.js
> > @@ +38,5 @@
> > >  	function _generateCallbackID() {
> > >  		return Math.random().toString(36).replace(/[^a-z]+/g, '');
> > >  	}
> > >  
> > > +	function _waitForCallback(callback, keepListening=false) {
> > 
> > Nit: use spaces around operators per the Mozilla coding style.
> > 
> > I think we shouldn't use a callback if we want to get more than one message.
> > We can use UITour.notify+Mozilla.UITour.observe for that instead. i.e. I
> > don't think we should add the keepListening argument.
> 
> That's definitely a better idea! I've changed the implementation to reflect
> this suggestion. There's only one problem with the "NotificationClose"
> notification sent when the heartbeat UI is closed: if a new tab is opened
> before the |notify| is called (e.g. user votes, the engagement page gets
> opened), it fails at [1] because |originTabs| is null. 

That is bug 1110602. A workaround would be to send the notification before the tab switch.

> Why is |sendPageCallback| taking the messagManager argument while |notify| isn't?

Consider when there are multiple tour tabs open. A callback message should only go to the specific page whereas notifications broadcast user actions to all open tabs.

(In reply to Alessio Placitelli [:Dexter] from comment #22)
> Matthew, can we add a new function to UITour to open a link in a new tab? We
> already have an |addPinnedTab| but having an |addTab| function could be
> useful to remove the engagement bits off the UI (thanks Gregg for the
> brainstorming!).

We could but I would rather avoid it if it isn't needed. Each API we expose adds to the possible attack service so if it's fine as-is then I would leave it. Also, addPinnedTab isn't a good example since it is basically dead code that was never used in production and should probably get removed at some point.

Comment 24

[Alessio Placitelli [:Dexter]]

(In reply to Matthew N. [:MattN] (away until Jan. 7) from comment #23)
> > That's definitely a better idea! I've changed the implementation to reflect
> > this suggestion. There's only one problem with the "NotificationClose"
> > notification sent when the heartbeat UI is closed: if a new tab is opened
> > before the |notify| is called (e.g. user votes, the engagement page gets
> > opened), it fails at [1] because |originTabs| is null. 
> 
> That is bug 1110602. A workaround would be to send the notification before
> the tab switch.

I see, thanks for the bug reference. Unfortunately, "NotificationClose" is sent after the vote takes place (or if user closes the UI), which is a few seconds after the engagement page opens. Should I find another way around this or wait for that bug to land?

Comment 25

[Matthew N. [:MattN]]

(In reply to Alessio Placitelli [:Dexter] from comment #24)
> (In reply to Matthew N. [:MattN] (away until Jan. 7) from comment #23)
> > > That's definitely a better idea! I've changed the implementation to reflect
> > > this suggestion. There's only one problem with the "NotificationClose"
> > > notification sent when the heartbeat UI is closed: if a new tab is opened
> > > before the |notify| is called (e.g. user votes, the engagement page gets
> > > opened), it fails at [1] because |originTabs| is null. 
> > 
> > That is bug 1110602. A workaround would be to send the notification before
> > the tab switch.
> 
> I see, thanks for the bug reference. Unfortunately, "NotificationClose" is
> sent after the vote takes place (or if user closes the UI), which is a few
> seconds after the engagement page opens. Should I find another way around
> this or wait for that bug to land?

What is the purpose of NotificationClosed? i.e. why does the hidden page need to know about it? If you wanted to fix that bug it would be nice but it's not clear that this notification is essential so it could probably left out or postponed to a follow-up bug.

Comment 26

[Alessio Placitelli [:Dexter]]

Attachment: Heartbeat v4

Thanks Matthew, I discussed with Gregg and |NotificationClosed| is not needed in the hidden page itself. It is quite useful in the tests, but maybe tests need to be changed :)

Comment 27

[Alessio Placitelli [:Dexter]]

Attachment: Heartbeat UI tests

I've moved the tests to a separate patch.

Comment 28

[Alessio Placitelli [:Dexter]]

Attachment: Heartbeat v5

This patch fixes OSX (thanks Gregg!!) and returns the flowId along with the notifications.

As discussed with MattN, I've added bug 1110602 as a blocker for this one (notifications are not received when switching tabs). We can land this, as the one it depends on is going to land in the upcoming weeks.

Comment 29

[Alessio Placitelli [:Dexter]]

Attachment: Heartbeat UI tests, v2

I've added tests for the cases of null or invalid engagement URL. Slightly changed the test for the score validation.

Comment 30

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

(I agree that FOR ONE VISIBLE TAB, all stages call back correctly.  

I am now very worried about the cross tab problem.  How will this work from a hidden window / non-existent tab.  Isn't that the actual proposal?)

Comment 31

[Alessio Placitelli [:Dexter]]

IMHO, this should be handled by the blocker bug as well.

Comment 32

[Matthew N. [:MattN]]

Comment on attachment 8549835 [details] [diff] [review]
Heartbeat v5

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

::: browser/components/uitour/UITour.jsm
@@ +1065,5 @@
> +   * @param aEngagementURL
> +   *        The engagement URL to open in a new tab once user has voted. If this is null
> +   *        or invalid, no new tab is opened.
> +   */
> +  showHeartbeat: function(aChromeWindow, aMessageManager, aMessage, aFlowId, aEngagementURL) {

For the optional arguments, indicate that in the JS Doc with square braces e.g. [aEngagementURL] and use default argument syntax to make it clear from the code e.g. `aEngagementURL = null`.

I kinda wonder if we should just pass the whole data object to the method but as long as this isn't a long-term thing where we keep adding arguments it's probably fine.

@@ +1073,5 @@
> +    let engagementURL = null;
> +    try {
> +      engagementURL = new URL(aEngagementURL);
> +    } catch (error) {
> +      log.error("showHeartbeat: Invalid URL specified.");

I think it would be good to move this down to right above where it's used. I also think that this should be log.warn or lower if it's acceptable to not have an engagementURL specified.

Also, you seem to have some Windows line endings in this method.

@@ +1081,5 @@
> +    let notice = nb.appendNotification(aMessage, aFlowId,
> +      "chrome://branding/content/icon64.png", nb.PRIORITY_INFO_HIGH, null, function() {
> +        // Let the consumer know the notification bar was closed. This also happens
> +        // after voting.
> +        this.notify("Heartbeat:NotificationClosed", { flowId: aFlowId, timestamp: Date.now() });

Does the timestamp really need to come from the Browser Chrome? Can't we just get the time in the hidden document or the server receiving the data? It's not a big deal but the idea was to do the minimum possible in Firefox itself AFAIK.

@@ +1117,5 @@
> +        this.notify("Heartbeat:Voted", { flowId: aFlowId, score: rating, timestamp: Date.now() });
> +
> +        // Display the Heart and make it pulse twice.
> +        notice.image = "chrome://browser/skin/heartbeat-icon.svg";
> +        notice.label = "Thank you!";

Are we only going to use this with English users? You're hard-coding the string here. It should probably come from the webpage.

@@ +1128,5 @@
> +          notice.removeChild(notice.firstChild);
> +        }
> +
> +        // Just open the engagement tab if we have a valid engagement URL.
> +        if (engagementURL !== null) {

We generally prefer `if (engagementURL) {`. See the 2nd bullet of https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#JavaScript_practices

::: browser/themes/shared/UITour.inc.css
@@ +240,5 @@
> +
> +.messageText.heartbeat {
> +  color: #333333;
> +  font-weight: normal;
> +  font-family: "Lucida Grande", Segoe, Ubuntu;

Unless UX has a good reason, we shouldn't override the default font-family or the UI will be inconsistent.

Comment 33

[Matthew N. [:MattN]]

Comment on attachment 8549835 [details] [diff] [review]
Heartbeat v5

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

::: browser/components/uitour/UITour.jsm
@@ +1077,5 @@
> +      log.error("showHeartbeat: Invalid URL specified.");
> +    }
> +
> +    // Create the notification.
> +    let notice = nb.appendNotification(aMessage, aFlowId,

I think we should prefix the notification ID here to prevent ID collisions.

Comment 34

[Matthew N. [:MattN]]

Comment on attachment 8549836 [details] [diff] [review]
Heartbeat UI tests, v2

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

::: browser/components/uitour/test/browser_UITour_heartbeat.js
@@ +24,5 @@
> + */
> +function simulateVote(aId, aScore) {
> +  let notification = notificationBox.getNotificationWithValue(aId);
> +
> +  if (notification.hasChildNodes()) {

When isn't this true? If we don't expect it not to be true then we should remove this check or have the test fail in an `else`.

@@ +51,5 @@
> +  /**
> +   * Check that the "stars" heartbeat UI correctly shows.
> +   */
> +  function test_heartbeat_stars_show(done) {
> +    let flowId = "heartbeat-ui-ratefirefox-" + Math.random();

The "heartbeat-" prefix can be what is hard-coded in UITour.jsm to avoid notification bar conflicts.

@@ +169,5 @@
> +          break;
> +        }
> +        case "Heartbeat:Voted": {
> +          info("'Heartbeat:Voted' notification received (timestamp " + aData.timestamp.toString() + ").");
> +          is(aData.score, expectedScore, "Should report a score of " + expectedScore);

You're not verifying that this message was ever received at the moment but I think you should. You can have a boolean variable that keeps track of whether it's called and check that in Heartbeat:NotificationClosed or move done() to here (if the messageManager issues get resolved).

@@ +175,5 @@
> +        }
> +        case "Heartbeat:NotificationClosed": {
> +          info("'Heartbeat:NotificationClosed' notification received (timestamp " + aData.timestamp.toString() + ").");
> +          // |done()| needs to be called here and not in "voted" otherwise the messageManager
> +          // will throw a NOT_INITIALIZED error.

Which line does this happen on? Is this something we should protect against in UITour?

@@ +193,5 @@
> +   * Test that the engagement page is correctly opened when voting.
> +   */
> +  function test_heartbeat_engagement_tab(done) {
> +    todo(false, "This test cannot happen until bug 1110602 lands.");
> +    done();

FYI: There is a Windows line ending on this line

Comment 35

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

Re: points from #c32

1) the arg list shouldn't grow.  I would prefer not to pass objects around unless those get validated at every step.  If we have a good lib for it, then it's fine :)

2) I am okay with passing the 'thank you' message as an arg.  (which directly violates my opinion preceding :)


3)  I want timestamp at *every layer*.  I can make decisions about it at the server, if  I want, but I prefer to have them be as close to the metal as possible.  The reaction happened *in firefox* not at the page.  Also putting this into the firefox message is one less thing that a recipe author will have to remember to do.


4) Font family does need to be populated.  This is the first "new style" notification bar.  Per Sevaan, Mike Maslaney.  Once styles are applied globally in Firefox notification bars, these can be unstyled.  Previous experiments tested 'current' nbs vs. 'new' ones.  New ones won.  Inconsistency is intentional here.
(see https://bugzilla.mozilla.org/show_bug.cgi?id=1092376)

Comment 36

[Alessio Placitelli [:Dexter]]

Attachment: Heartbeat v6

MattN, thank you for your time reviewing this (and thanks Gregg!). This patch:

- adds the thank you as a parameter;
- marks the engagement URL as optional;
- fixes the other nits.

Comment 37

[Alessio Placitelli [:Dexter]]

Attachment: Heartbeat UI tests, v3

For some reason, the message manager problem is not there anymore in the latest m-c. I've removed the comments and changed the example accordingly.

Comment 38

[Matthew N. [:MattN]]

Comment on attachment 8553327 [details] [diff] [review]
Heartbeat UI tests, v3

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

::: browser/components/uitour/test/browser_UITour_heartbeat.js
@@ +56,5 @@
> +
> +    gContentAPI.observe(function (aEventName, aData) {
> +      switch (aEventName) {
> +        case "Heartbeat:NotificationOffered": {
> +          info("'Heartbeat:Offered' notification received (timestamp " + aData.timestamp.toString() + ").");

I think there should be a test to confirm aData.timestamp is a Number for each of the types of notifications.

Comment 39

[Alessio Placitelli [:Dexter]]

Attachment: Heartbeat UI tests, v4

Thanks MattN for the review.

Comment 40

[Alessio Placitelli [:Dexter]]

This is a try-push for the patches here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c65c30508e43

Comment 41

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/c93764d04a24
https://hg.mozilla.org/integration/fx-team/rev/b80b6de33ff2

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c93764d04a24
https://hg.mozilla.org/mozilla-central/rev/b80b6de33ff2

Comment 43

[Matthew N. [:MattN]]

Does this need to be uplifted to aurora or beta? I'm going to be uplifting many Hello UITour changes in the next day so uplifting this at the same time will reduce conflicts.

Comment 44

[Benjamin Smedberg]

I'd like to uplift this to Aurora. There's no need to do it for beta.

Comment 45

[Matthew N. [:MattN]]

Comment on attachment 8553325 [details] [diff] [review]
Heartbeat v6

Approval Request Comment
[Feature/regressing bug #]: Minimal v0 self-support feature (bug 1111016)
[User impact if declined]: Not able to help users without them seeking support on their own
[Describe test coverage new/current, TreeHerder]: m-bc test
[Risks and why]: Low risk isolated code. Worst case is that we don't deploy any self-support tasks.
[String/UUID change made/needed]: None. Strings come from the webpage.

I can uplift this with the rest of the UITour uplifts I'm doing since they will likely conflict.

Comment 46

[Matthew N. [:MattN]]

Comment on attachment 8555185 [details] [diff] [review]
Heartbeat UI tests, v4

Approval Request Comment: See comment 45

Comment 47

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8553325 [details] [diff] [review]
Heartbeat v6

I'm accepting this new feature work in Aurora because:
- Heartbeat will improve support for users
- this work is a prereq for self heal and needs time in front of an audience
- most of the large change set is due to the addition of svgs
- the code change is isolated and my understanding is that if issues are discovered, we can simply not use Heartbeat in 37
- the feature comes with tests

Aurora+

Comment 48

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8555185 [details] [diff] [review]
Heartbeat UI tests, v4

Test only patch. Aurora+

Comment 49

[Lawrence Mandel [:lmandel] (use needinfo)]

Release Note Request (optional, but appreciated)
[Why is this notable]: New user rating and self support system
[Suggested wording]: New: Heartbeat user rating system
[Links (documentation, blog post, etc)]:

Matt/Tyler/Gregg - I would appreciate your input on how to note this new feature.

Comment 50

[Matt Grimes [:Matt_G]]

Hey Lawrence. You could pull something from this page: https://mana.mozilla.org/wiki/display/ADVOCACY/Firefox+Heartbeat

Perhaps: Heartbeat is User Voice in Firefox. It ties user perception to technical information. Heartbeat provides real-time understanding of our existing Desktop user population, allowing us to measure our success and identify opportunities for growth.  

Feel free to wordsmith.

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9455f35972cb
https://hg.mozilla.org/releases/mozilla-aurora/rev/3963cf9ee860

Comment 52

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

Thanks Tim, Ryan, Matt, Benjamin, Sevaan, Georg, and Alessio for getting this done!  Awesome job!

Comment 53

[Lawrence Mandel [:lmandel] (use needinfo)]

relnoted as "New: Heartbeat user rating system - your feedback about Firefox"

The suggestion in comment 50 is a bit long. I went with something shorter. I would really like to link to documentation about this feature and didn't find something suitable with a quick search. Is there a public blog post or other doc about this feature?

Comment 54

[Matt Grimes [:Matt_G]]

Lawrence and I spoke over IRC. Relnote looks great and we've got a public wiki up here: https://wiki.mozilla.org/Advocacy/heartbeat

Feedback is welcome if there are additional bits that should be added to the page.