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

RESOLVED FIXED in Firefox 29

Status

()

defect
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Unfocused, Assigned: Unfocused)

Tracking

unspecified
Firefox 29
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Posted image 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.
Priority: -- → P1
Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
Update: Got the API and behaviour fully spec'ed out, and the code partially implemented today.
Hi Blair-

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

Thanks,
Jen
Posted patch Patch v1 (obsolete) — Splinter Review
Also fixes bug 941864, and implements infrastructure for callbacks for the likes of bug 941862, bug 936195, etc.
Attachment #8344551 - Flags: review?(MattN+bmo)
(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 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
Attachment #8344551 - Flags: feedback+
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.
Attachment #8344551 - Flags: review?(MattN+bmo) → review-
(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.
Posted patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #8344551 - Attachment is obsolete: true
Attachment #8347924 - Flags: review?(MattN+bmo)
Comment on attachment 8347924 [details] [diff] [review]
Patch v2

(Patch without bitrot everywhere coming up.)
Attachment #8347924 - Attachment is obsolete: true
Attachment #8347924 - Flags: review?(MattN+bmo)
Posted patch Patch v2.1Splinter Review
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.
Attachment #8348478 - Flags: review?(MattN+bmo)
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.
Attachment #8348478 - Flags: review?(MattN+bmo) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/5a8d6a009942
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Suspect this is just bug 956202.
Status: REOPENED → ASSIGNED
Target Milestone: Firefox 29 → ---
https://hg.mozilla.org/mozilla-central/rev/d96ba52e5423
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.