935815 - UITour: Allow adding a button with an action to the info panel

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Mockup

We'd like to optionally be able to define a button when showing an info panel, that somehow executes a callback on the webpage. The button text should also be supplied by the page.

Probably easiest to implement if the callback simply involved dispatching a mozUITourResponse (or something) event to the document element, and let the JS library handle the rest.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Update: Got the API and behaviour fully spec'ed out, and the code partially implemented today.

Comment 2

[Jennifer Bertsch [:jbertsch]]

Hi Blair-

Just wanted to check in and see how this one is coming along?  Anything we can help you out with?

Thanks,
Jen

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

Also fixes bug 941864, and implements infrastructure for callbacks for the likes of bug 941862, bug 936195, etc.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Jennifer Bertsch [:jbertsch] from comment #2)
> Just wanted to check in and see how this one is coming along?  Anything we
> can help you out with?

I know I already answered on IRC, but could you needinfo? me next time - questions get lost in the flood of bugmail otherwise :\

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8344551 [details] [diff] [review]
Patch v1

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

This is a partial review since I still need to look at the CSS and test files.

::: browser/modules/UITour.jsm
@@ +17,5 @@
>  
>  const UITOUR_PERMISSION   = "uitour";
>  const PREF_PERM_BRANCH    = "browser.uitour.";
>  
> +const LIMIT_MAX_BUTTONS		= 4;

Nit: Having both "limit" and "max" feels redundant. Isn't MAX_BUTTONS enough? Also, I don't see a reason to separate this from the other const's (in which case the |=| should be aligned).

@@ +320,5 @@
> +
> +    return null;
> +  },
> +
> +  sendPageCallback: function(aDocument, aCallbackID, aData = null) {

|aData = {}| would work too, right? Then you could get rid of the |if (aData)| below unless you want to protect against people explicitly passing null/false/0

@@ +430,5 @@
>      let highlighter = aWindow.document.getElementById("UITourHighlight");
>      highlighter.removeAttribute("active");
>    },
>  
> +  showInfo: function(aContentDocument, aAnchor, aTitle, aDescription, aIconURL, aButtons = []) {

Nit: I think the optional arguments (primarily aIconURL but apparently aTitle & aDescription too) should provide default values to make it clear they are optional. It's more self-documenting.

@@ +443,5 @@
>  
>      tooltip.hidePopup();
>  
> +    tooltipTitle.textContent = aTitle || "";
> +    tooltipDesc.textContent = aDescription || "";

Nit: I don't think this is necessary as I don't believe there is harm in setting textContent to |null| and if you're worried about |undefined|, then you can set the default value on the argument (assuming you do the same for aIconURL). The same is likely true for |src| below.

Am I missing a case where a falsy value would cause a problem? I guess |false| would output "false" but that would be a bug in the caller IMO. Leave it if you like.

@@ +447,5 @@
> +    tooltipDesc.textContent = aDescription || "";
> +    tooltipIcon.src = aIconURL || "";
> +
> +    while (tooltipButtons.firstChild)
> +      tooltipButtons.removeChild(tooltipButtons.firstChild);

Nit: you could use the newer ChildNode.remove()[1] here to be a bit shorter and easier to read.
[1] https://developer.mozilla.org/en-US/docs/Web/API/ChildNode.remove

@@ +458,5 @@
> +
> +      let callbackID = button.callbackID;
> +      el.addEventListener("command", event => {
> +        tooltip.hidePopup();
> +        this.sendPageCallback(aContentDocument, callbackID);

Does the HueyFix prevent this from leaking if a button is left in the document as a child of an info panel? We could be safer about this by removing buttons in hideInfo which gets called at teardown too.

::: browser/modules/test/uitour.js
@@ +74,5 @@
>  		_sendEvent('hideHighlight');
>  	};
>  
> +	Mozilla.UITour.showInfo = function(target, title, text, icon, buttons) {
> +		console.log("buttons", buttons);

leftover console.log

@@ +78,5 @@
> +		console.log("buttons", buttons);
> +		var buttonData = [];
> +		if (Array.isArray(buttons)) {
> +			for (var i = 0; i < buttons.length; i++) {
> +				console.log("Adding button ", buttons[i].label);

ditto

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8344551 [details] [diff] [review]
Patch v1

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

::: browser/base/content/browser.xul
@@ +210,5 @@
> +          <label id="UITourTooltipTitle" flex="1"/>
> +          <description id="UITourTooltipDescription" flex="1"/>
> +        </vbox>
> +      </hbox>
> +      <hbox id="UITourTooltipButtons" flex="1" pack="end"/>

I think the buttons should be in the vbox below <description> according to https://docs.google.com/a/mozilla.com/file/d/0B60Ayj_R5h_jb0gzSWtpSkpSeXM/edit

i.e. the left of the button should align with the left of the description.

::: browser/modules/test/uitour.js
@@ +43,5 @@
> +
> +	function _waitForCallback(callback) {
> +		// Assume a string is a magic identifier the app will handle.
> +		if (typeof callback == "string")
> +			return callback;

Is this still needed? I'm not sure what it's trying to solve.

::: browser/themes/shared/UITour.inc.css
@@ +20,5 @@
>  
> +#UITourTooltipIcon {
> +  max-width: 48px;
> +  max-height: 48px;
> +  padding: 5px;

I don't think 5px is enough. You should get ui-review from mmaslaney though.

@@ +25,5 @@
> +}
> +
> +@media (min-resolution: 2dppx) {
> +  #UITourTooltipIcon {
> +    max-width: 96px;

I don't think we want any of these 2dppx changes as this stuff is handled automatically and your changes make the icons get twice as large relative to the other UI.

@@ +43,5 @@
>  }
> +
> +#UITourTooltipButtons > button > .button-box > .button-icon {
> +  max-width: 16px;
> +  max-height: 16px;

With the 10.9 SDK, the icon bleeds outside the button. This is caused by bug 931040 but demonstrates that this is somewhat fragile. I was going to suggest max-height: fill-available; but it hasn't been implemented yet (bug 527285). See http://grab.by/sMrI Can you make sure this is fine with a different SDK?

@@ +48,5 @@
> +}
> +
> +@media (min-resolution: 2dppx) {
> +  #UITourTooltipButtons > button > .button-box > .button-icon {
> +    max-width: 32px;

AFAICT, this is wrong too and not needed.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Matthew N. [:MattN] from comment #6)
> ::: browser/modules/test/uitour.js
> @@ +43,5 @@
> > +
> > +	function _waitForCallback(callback) {
> > +		// Assume a string is a magic identifier the app will handle.
> > +		if (typeof callback == "string")
> > +			return callback;
> 
> Is this still needed? I'm not sure what it's trying to solve.

More along the lines of not needed *yet*, but yes it is extraneous for now.

I planned to use this for the likes of bug 938079, where we want Firefox itself to do some standard action when a button is pressed. Instead of generating a random ID string and sending that as the callback ID, the page would send something like "__POSTPONE_TOUR__" - and UITour.jsm would treat that specially.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2

Haven't been able to test OSX yet, as I'm in the process of unbreaking my OSX VMs (grrr). But that shouldn't hold up reviews, as I don't expect any problems. Won't land until I've verified it works as expected there.

The various CSS tweaks around the buttons are needed to make them look decent (right spacing in the right places). Because XUL hates me.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8347924 [details] [diff] [review]
Patch v2

(Patch without bitrot everywhere coming up.)

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2.1

Ok, bitrot killed.

Looks good on OSX 10.8. But I can't for the life of me get my OSX VMs to use a high enough resolution to enable HiDPI, so I can't test there. Our HiDPI support on Win8 is kinda crap, but it seems to be ok there.

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8348478 [details] [diff] [review]
Patch v2.1

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

r+ with the hideInfo fix. HiDPI is much better now.

Don't forget 'Australis' in the commit message.

::: browser/modules/UITour.jsm
@@ +628,5 @@
>      let tooltip = aWindow.document.getElementById("UITourTooltip");
>      tooltip.hidePopup();
>      this._setAppMenuStateForAnnotation(aWindow, "info", false);
> +
> +    let tooltipButtons = document.getElementById("UITourTooltipButtons");

document is undefined. You want aWindow.document here.

::: browser/modules/test/uitour.html
@@ +4,5 @@
>      <meta charset="utf-8" />
>      <title>UITour test</title>
>      <script type="application/javascript" src="uitour.js">
>      </script>
> +    <script type="application/javascript">

Nit: @type on <script> defaults to JS so the attribute isn't necessary.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-bc orange.
https://hg.mozilla.org/integration/fx-team/rev/cd24da528bf3

https://tbpl.mozilla.org/php/getParsedLog.php?id=32189601&tree=Fx-Team

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Matthew N. [:MattN] from comment #5)
> > +    tooltipTitle.textContent = aTitle || "";
> > +    tooltipDesc.textContent = aDescription || "";
> 
> Nit: I don't think this is necessary as I don't believe there is harm in
> setting textContent to |null| and if you're worried about |undefined|, then
> you can set the default value on the argument (assuming you do the same for
> aIconURL). The same is likely true for |src| below.

This was the cause of the orange :\ Knew there was a reason why I did that - allowing that to be set to null (which is falsey), results in it getting converted to "null" (ie, the string "null"). Unexpected, unwanted, and was breaking the tests.

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/fx-team/rev/5a8d6a009942

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5a8d6a009942

Comment 17

[Carsten Book [:Tomcat]]

backedout this change again in central and integreation trees for frequent failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=32624776&tree=Mozilla-Inbound

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Suspect this is just bug 956202.

Comment 19

[Blair McBride [:Unfocused] (UNAVAILABLE)]

X time lucky!

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

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d96ba52e5423