935823 - UITour: Add close button to info panel

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Current thinking is we'd like a close button on the info panels. This is going against the established norm for Firefox, but we also have to be more careful with new users unfamiliar with Firefox here.

Comment 1

[Holly Habstritt Gaal [:Habber]]

Details regarding the logic of the 'x' button:
Many users will select 'x' and remove the Update doorhanger when seeing it for the first time. However, we would like one more chance to catch these users the next time they open their browser. 

Selecting 'x' will act as "Not Now" ONLY the first time the user selects the 'x'. Selecting 'x' the first time the user sees the doorhanger means that the next time the browser opens we will display the doorhanger once more. Selecting 'x' to close the doorhanger for a second time means we will not show this to them ever again.


Question: Can the amount of times we recall the doorhanger after selecting 'x' be a variable so that we have control over it? At this time I don't think we would want to show the doorhanger more than once more, but there may be a situation in the future where we would want to do so more than once. 


How is this different than clicking the "Not Now" link?  
The user can select "Not Now" over and over until we discontinue this experience from our end. So each time the user selects "Not Now" we will show them the doorhanger and Update page the next time they open the browser.

Comment 2

[Holly Habstritt Gaal [:Habber]]

It is also important to note that this doesn't apply to just the doorhanger, but the URL/webpage that accompanies it.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Button added in the patch in bug 958673, but without the behaviour described in comment 1. So leaving this open for that to be fixed.

Comment 4

[Matthew N. [:MattN]]

Bumping to P4 since there are other ways to exit the tour and other styling bugs are more important.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Alex Gibson [:agibson] from comment #28)
> Blair, can the web page tell if the user has clicked the close (x) button in
> the new style door hanger? 
> 
> Currently the door-hanger closes but nothing else happens.

Seems we want some form of callback too.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Note: Although the button was added in bug 958673, it's been temporarily disabled in bug 966913 at least until we can get the callback implemented.

Comment 7

[Matthew N. [:MattN]]

I think this bug should just focus on making the close button visible and calling a callback. Bug 938079 will handle re-displaying the tour at later times.

Holly, there are concerns that users aren't going to understand the difference between the X and "Not now" so for now I think they should have the same behaviour if this gets implemented.

Comment 8

[Matthew N. [:MattN]]

Attachment: v.1 Add a close callback argument

Having the close button visible will help make the desired design for bug 971108.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8387369 [details] [diff] [review]
v.1 Add a close callback argument

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

::: browser/modules/UITour.jsm
@@ +861,5 @@
>  
>      this._setAppMenuStateForAnnotation(aWindow, "highlight", false);
>    },
>  
> +  showInfo: function(aContentDocument, aAnchor, aTitle = "", aDescription = "", aIconURL = "", aButtons = [], closeCallbackID = null) {

This could do with some wrapping.

::: browser/modules/test/browser_UITour3.js
@@ +135,5 @@
> +    function closeButtonCallback() {
> +      ok(true, "Close button callback called");
> +      done();
> +    }
> +    gContentAPI.showInfo("urlbar", "Close me", "X marks the spot", null, null, closeButtonCallback);

Hm, I'm starting to worry about the parameters we have for this method - with the latest addition it feels unwieldy. How about a general options param we can put all the extra misc additions into:


  gContentAPI.showInfo("urlbar", "Close me", "X marks the spot", null, null, {
    onClose: closeCallback
  });

(Not intending any structural change to how it's actually sent via _sendEvent, though I don't think it needs to be called "closeButtonCallback" per se.)

Comment 10

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/048536bb76ef

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/048536bb76ef

Comment 12

[Alex Gibson [:agibson]]

Blair / Matt - could one of you please update australis-uitour.js so I know what to use for the callback here?

Thanks

Comment 13

[Alex Gibson [:agibson]]

I can see the close button in the menu panel now, but I can't seem to get the callback to fire? Perhaps the code for the callback has not made it in just yet, so will try again tomorrow.

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Ugh, wish we had a smoother process for this. It's in the library on Github now:
https://github.com/Unfocused/mozilla-uitour/commit/51fad56eea77b5cb224032b05baccfbedde80fa2

It has *not* landed on Aurora yet. So if you're expecting to be able to test it, you'll need to be running Nightly for now.

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8387369 [details] [diff] [review]
v.1 Add a close callback argument

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: No close button on tour info panel. Can use one of the main buttons, but some people expect to see a close button.
Testing completed (on m-c, etc.): Unit tests on m-c
Risk to taking this patch (and alternatives if risky): Minimal
String or IDL/UUID changes made by this patch: None, strings were already in tree

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7dfa048bd345