UITour: Give webpage a way to query what UI targets are available

VERIFIED FIXED in Firefox 29

Status

()

defect
P3
normal
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: Unfocused, Assigned: adw)

Tracking

(Depends on 1 bug)

unspecified
Firefox 30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 fixed, firefox30 fixed)

Details

(Whiteboard: [Australis:M-])

Attachments

(2 attachments, 1 obsolete attachment)

Currently, when a tour webpage requests a highlight/info panel for a UI element that has been removed via customization, we just do nothing. This is relatively safe to do.

However, it would be better if the page had a way to query what elements are available, so it can adjust its content to be appropriate.
Priority: -- → P3
Assignee

Comment 1

5 years ago
This seems to work.  It's a little tricky mapping widget IDs/names to targets.
Attachment #8372717 - Flags: review?(bmcbride)
Comment on attachment 8372717 [details] [diff] [review]
Add UITour.getAvailableTargets

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

::: browser/modules/UITour.jsm
@@ +256,5 @@
>          this.getConfiguration(contentDocument, data.configuration, data.callbackID);
>          break;
>        }
> +
> +      case "getAvailableTargets": {

Drive-by comment: I was thinking we would reuse getConfiguration for this

@@ +836,5 @@
>      }
>      this.sendPageCallback(aContentDocument, aCallbackId, config);
>    },
> +
> +  getAvailableTargets: function(aContentDocument, aCallbackId) {

I think this is complex enough to deserve tests.
Assignee

Comment 3

5 years ago
Thanks, Matt.  I'd like to get an r+ or at least a feedback+ from Blair (or anybody else I guess who's familiar with this bug) that the patch is doing the right thing before working on tests.
Comment on attachment 8372717 [details] [diff] [review]
Add UITour.getAvailableTargets

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

Hm, this approach doesn't handle targets that aren't widgets managed by CustomizableUI, such as "quit" and "customize". It also doesn't take into account widgets that aren't available in all windows - such as those not available in private-browsing windows.

Seems easiest/most reliable to just kick off a bunch of parallel getTarget() calls. And either way, we should cache the results per-window (the clear the cache any time we see a customization event).

Also, we should advertise the availability of "pinnedTab", which is a target we dynamically create (so that shouldn't go through getTarget).

::: browser/modules/UITour.jsm
@@ +256,5 @@
>          this.getConfiguration(contentDocument, data.configuration, data.callbackID);
>          break;
>        }
> +
> +      case "getAvailableTargets": {

(In reply to Matthew N. [:MattN] from comment #2)
> Drive-by comment: I was thinking we would reuse getConfiguration for this

What Matt said.

@@ +836,5 @@
>      }
>      this.sendPageCallback(aContentDocument, aCallbackId, config);
>    },
> +
> +  getAvailableTargets: function(aContentDocument, aCallbackId) {

aCallbackID - or someone's going to end up screaming profanities sometime in the future because of us using Id and ID in the same module.
Attachment #8372717 - Flags: review?(bmcbride) → review-
DOOMED
Assignee: nobody → adw
Status: NEW → ASSIGNED
I don't think we're going to need this for the Australis update tour because most things will be in a default position for Australis. Holly, is that fine that this doesn't make it for 29?
Flags: needinfo?(hhabstritt.bugzilla)
Whiteboard: [Australis:M-]
Assignee

Comment 7

5 years ago
I see, thanks.

(In reply to Blair McBride [:Unfocused] from comment #4)
> @@ +836,5 @@
> >      }
> >      this.sendPageCallback(aContentDocument, aCallbackId, config);
> >    },
> > +
> > +  getAvailableTargets: function(aContentDocument, aCallbackId) {
> 
> aCallbackID - or someone's going to end up screaming profanities sometime in
> the future because of us using Id and ID in the same module.

But this module uses both!  Look a couple of lines above the two added lines I just quoted (and you quoted). :-)  ID seems to be more popular, so I did change it.
Attachment #8372717 - Attachment is obsolete: true
Attachment #8384086 - Flags: review?(bmcbride)
Comment on attachment 8384086 [details] [diff] [review]
add UITour.getConfiguration("availableTargets")

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

Most excellent.

::: browser/modules/UITour.jsm
@@ +935,5 @@
>      });
>      aWindow.gBrowser.selectedTab = tab;
>    },
>  
>    getConfiguration: function(aContentDocument, aConfiguration, aCallbackId) {

*sigh* Don't suppose you'd like to just rename this to aCallbackID while you're at it? :)

::: browser/modules/test/browser_UITour_availableTargets.js
@@ +67,5 @@
> +];
> +
> +function ok_targets(actualData, expectedTargets) {
> +  // Depending on how soon after page load this is called, the selected tab icon
> +  // may or may not be showing the loading throbber.  Check for its presence and

Ugh, we should fix that. File a bug?
Attachment #8384086 - Flags: review?(bmcbride) → review+
(In reply to Matthew N. [:MattN] from comment #6)
> I don't think we're going to need this for the Australis update tour because
> most things will be in a default position for Australis. Holly, is that fine
> that this doesn't make it for 29?

I don't consider this a blocker. But saying that, it does allow fixing breakage in the tour, so I would like it uplifted to Aurora. I don't think it comes with much risk in doing so - especially given we can just opt to not use it if it ends up breaking.
Assignee

Comment 10

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/23b6dc0350c8

(In reply to Blair McBride [:Unfocused] from comment #8)
> >    getConfiguration: function(aContentDocument, aConfiguration, aCallbackId) {
> 
> *sigh* Don't suppose you'd like to just rename this to aCallbackID while
> you're at it? :)

Fixed, and fixed the couple of other instances, involving "aId".

> > +  // Depending on how soon after page load this is called, the selected tab icon
> > +  // may or may not be showing the loading throbber.  Check for its presence and
> 
> Ugh, we should fix that. File a bug?

bug 979103
https://hg.mozilla.org/mozilla-central/rev/23b6dc0350c8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Assignee

Comment 12

5 years ago
(In reply to Blair McBride [:Unfocused] from comment #9)
> I don't consider this a blocker. But saying that, it does allow fixing
> breakage in the tour, so I would like it uplifted to Aurora. I don't think
> it comes with much risk in doing so - especially given we can just opt to
> not use it if it ends up breaking.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): It's a new feature.
User impact if declined: Parts of Australis UITour content pages may be broken until Firefox 30.
Testing completed (on m-c, etc.): browser-chrome test on m-c and fx-team.
Risk to taking this patch (and alternatives if risky): Small.
String or IDL/UUID changes made by this patch: None.
Attachment #8385812 - Flags: approval-mozilla-aurora?
Attachment #8385812 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Can we please update australis-uitour.js to reflect the change here? Thanks
No, Unfocused is: https://github.com/Unfocused/mozilla-uitour

I said I would do it but didn't have time on Friday and I'm on PTO today and tomorrow so if someone else wants to that would be good.
Assignee

Comment 17

5 years ago
Oh.  Alex, if it's OK that your repo doesn't match Blair's, you can just replace your getSyncConfiguration with the getConfiguration from this test: http://mxr.mozilla.org/mozilla-central/source/browser/modules/test/uitour.js#168

Then call it with "availableTargets" as the configName.
Assignee

Comment 18

5 years ago
And "sync" for the old getSyncConfiguration behavior.
Flags: needinfo?(hhabstritt.bugzilla)
Thanks, Blair

One question about this API, does it only return a list of available targets, or can it also tell me where in the chrome a target is located? I seem to remember a discussion that the latter would be useful, but I wasn't sure if this bug covers that.
(In reply to Alex Gibson [:agibson] from comment #20)
> One question about this API, does it only return a list of available
> targets, or can it also tell me where in the chrome a target is located? I
> seem to remember a discussion that the latter would be useful, but I wasn't
> sure if this bug covers that.

It's just an array of targets that aren't in the palette. I filed bug 988151 to get more visibility/location information returned.
Keywords: verifyme
Is there a way to verify this manually?

I removed Add-ons and Bookmarks  button from Menu Panel and Toolbar but from an user point of view nothing was changed postfix. The pages from the Ui Tour related to Add-ons and Bookmarks are still shown whether the buttons are visible or not.

Builds used for testing:
Prefix: 
Aurora 29
Build Id: 20140303004002

Postfix:
Beta 29 RC candidate
Build Id:20140421221237
(In reply to Catalin Varga [QA][:VarCat] from comment #22)
> Is there a way to verify this manually?
> 
> I removed Add-ons and Bookmarks  button from Menu Panel and Toolbar but from
> an user point of view nothing was changed postfix. The pages from the Ui
> Tour related to Add-ons and Bookmarks are still shown whether the buttons
> are visible or not.

Note: this API is currently not used in the tour itself, so it will need testing separately.
No human verification is needed at this time.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.