Closed Bug 1071238 Opened 10 years ago Closed 10 years ago

UI Tour: add ability to put a widget in the toolbar

Categories

(Firefox :: General, defect)

defect
Not set
normal
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 35
Iteration:
35.3
Tracking Status
firefox33 --- fixed
firefox34 --- fixed
firefox35 --- fixed

People

(Reporter: Dolske, Assigned: Dolske)

References

Details

Attachments

(1 file, 3 obsolete files)

Bug 1069300 is implementing a new button for Firefox's UI, but it won't be place in the navbar or menu panel by default. We're going to use UITour to promote the it, and would like a way to make it easy for the user to add the button to their UI.

It should be fairly simple to create a new UITour API that allows the firstrun page to place a requested widget into the toolbar. EG:

1) Firstrun/whatsnew page is shown
2) User reads description of awesome new feature
3) User clicks "Add this to Firefox!" button in the firstrun page (ie, in content)
4) Awesome new feature widget is placed in navbar (next to menubutton)

The page will likely then use existing UITour APIs to hilight the newly-added widget.

(We were previously thinking about doing this a different way, bug 1068332, but this is preferred as being simpler and more actionable for the user.)
Flags: qe-verify+
Flags: firefox-backlog+
Flags: qe-verify+ → qe-verify-
Are there design aspects to this work? For example, do we need a transition or glow, etc., to make the newly added toolbar item noticeable?
Oh, as a suggestion, maybe a slight size pulse on the icon, the way that the bookmark menu icon grows and shrinks when you add a new bookmark?
The idea is that the website can show a highlight or open a panel anchored on the new icon to draw attention to it.

I guess that means that this API should provide a callback to let the page know when the process of inserting the widget has finished so it knows it's ready to be highlighted.
Yeah, to keep scope down we should just reuse the existing hilight mechanism. The UITour would call this new API to add the button, and then the existing hilight API to hilight it. Hilight.
What happens if the navbar is overflowing when this button is pressed?

Does the page need a way to detect if the button is still in the palette / already on the toolbar? (which may be privacy-sensitive)
(In reply to :Gijs Kruitbosch from comment #5)
> What happens if the navbar is overflowing when this button is pressed?

UITour should be updated to automatically open the overflow panel if this is the case. It already opens the menu panel for things placed in it.

> Does the page need a way to detect if the button is still in the palette /
> already on the toolbar? (which may be privacy-sensitive)

It already has this capability via getConfiguration("availableTargets").
Attached patch Patch v.0 (obsolete) — Splinter Review
This seems to basically to the job, needs a little more work and a test.
Assignee: nobody → dolske
Iteration: --- → 34.3
Points: --- → 3
Status: NEW → ASSIGNED
Iteration: 34.3 → 35.3
No longer blocks: fx10
Attached patch Patch v.1 (obsolete) — Splinter Review
Attachment #8496215 - Attachment is obsolete: true
Attachment #8498983 - Flags: review?(MattN+bmo)
Comment on attachment 8498983 [details] [diff] [review]
Patch v.1

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

::: browser/modules/UITour.jsm
@@ +99,5 @@
>      ["home",        {query: "#home-button"}],
>      ["loop",        {query: "#loop-call-button"}],
> +    ["forget", {
> +      query: "#panic-button",
> +      widgetName: "panic-button",

In the past widgetName only needed to be specified if the element returned by the query isn't the widget container. In targetIsInAppMenu we handled this with aTarget.widgetName || aTarget.node.id but I recall you explained that the problem is that since the icon is in the palette, the aTarget.node.id won't work since the target can't be found in the document yet.

@@ +1189,5 @@
>        });
>      });
>    },
>  
> +  addToolbarWidget: function (aTarget, aContentDocument, aCallbackID) {

I would prefer if this wasn't hardcoded for the nav-bar as this is an API with which we need to preserve compatibility with old Firefox versions and I'd rather not have to duplicate this logic if we want to add something to the menu panel. I think the area e.g. "NAVBAR" should be passed from the page and then we can lookup with CustomizableUI.["AREA_" + NAVBAR]. The name like addWidgetToArea would be fine.

@@ +1199,5 @@
> +      Cu.reportError("UITour: not allowed to add this widget: " + data.target);
> +      return;
> +    }
> +
> +    CustomizableUI.addWidgetToArea(aTarget.widgetName, CustomizableUI.AREA_NAVBAR);

You don't check that widgetName is specified. Was the intention to assume that any target with allowAdd = true should have widgetName specified?

::: browser/modules/test/browser_UITour.js
@@ +294,5 @@
> +        gContentAPI.getConfiguration("availableTargets", (data) => {
> +          let available = (data.targets.indexOf("forget") != -1);
> +          ok(available, "Forget button should now be available");
> +
> +          // Cleanup

I don't see a call to "done" in this file so I'm not sure how this would work?
Attachment #8498983 - Flags: review?(MattN+bmo) → feedback+
(In reply to Matthew N. [:MattN] from comment #9)

> > +  addToolbarWidget: function (aTarget, aContentDocument, aCallbackID) {
> 
> I would prefer if this wasn't hardcoded for the nav-bar as this is an API
> with which we need to preserve compatibility with old Firefox versions and
> I'd rather not have to duplicate this logic if we want to add something to
> the menu panel. I think the area e.g. "NAVBAR" should be passed from the
> page and then we can lookup with CustomizableUI.["AREA_" + NAVBAR]. The name
> like addWidgetToArea would be fine.

If we do end up wanting a "addMenupanelWidget" function, I think it will trivial and minor refactoring. I'd rather not over-generalize code we don't have known usecases for. [I think it's unlikely we'd do it anyway, since it would be weird to promote a feature but put it into the menupanel.]

> > +    CustomizableUI.addWidgetToArea(aTarget.widgetName, CustomizableUI.AREA_NAVBAR);
> 
> You don't check that widgetName is specified. Was the intention to assume
> that any target with allowAdd = true should have widgetName specified?

Hmm. I can add a check just for an obvious error, but I suspect this would be pretty easy to discover during development for someone adding a allowAdd widget.

> > +          // Cleanup
> 
> I don't see a call to "done" in this file so I'm not sure how this would
> work?

Oh, haha. I probably just made sure the test passed in the output, but it didn't register that the test hadn't finished.
Attached patch Patch v.2 (obsolete) — Splinter Review
Attachment #8498983 - Attachment is obsolete: true
Attachment #8499751 - Flags: review?(MattN+bmo)
Comment on attachment 8499751 [details] [diff] [review]
Patch v.2

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

r+ if you don't leave addToolbarWidget as-is since the name is inaccurate.

(In reply to Justin Dolske [:Dolske] from comment #10)
> (In reply to Matthew N. [:MattN] from comment #9)
> 
> > > +  addToolbarWidget: function (aTarget, aContentDocument, aCallbackID) {
> > 
> > I would prefer if this wasn't hardcoded for the nav-bar as this is an API
> > with which we need to preserve compatibility with old Firefox versions and
> > I'd rather not have to duplicate this logic if we want to add something to
> > the menu panel. I think the area e.g. "NAVBAR" should be passed from the
> > page and then we can lookup with CustomizableUI.["AREA_" + NAVBAR]. The name
> > like addWidgetToArea would be fine.
> 
> If we do end up wanting a "addMenupanelWidget" function, I think it will
> trivial and minor refactoring. I'd rather not over-generalize code we don't
> have known usecases for. [I think it's unlikely we'd do it anyway, since it
> would be weird to promote a feature but put it into the menupanel.]

I wouldn't want/need to add a addMenupanelWidget action if the area was passed in so it would be less code.

I can definitely see cases where we wouldn't want to keep piling onto the nav-bar with new buttons. If you don't want to change this to have the area passed then you at least need to change the action name to addNavBarWidget since it only works for that one toolbar.
Attachment #8499751 - Flags: review?(MattN+bmo) → review+
Attached patch Patch v.3Splinter Review
Attachment #8499751 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ac12f6f24545
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
I've opened a pull request to add this to mozilla-uitour: 

https://github.com/Unfocused/mozilla-uitour/pull/8
I think it would be better to track this code in mozilla-central, where it already exists as http://mxr.mozilla.org/mozilla-central/source/browser/modules/test/uitour.js (and tends to get updated by the tests that make use of it).
Comment on attachment 8500711 [details] [diff] [review]
Patch v.3

a+ for beta as this bug will be landing in the 33.x anniversary release. (It's already on Nightly + Aurora, so only needs a beta landing at this point.)
Attachment #8500711 - Flags: approval-mozilla-beta+
Landed in alder: https://hg.mozilla.org/projects/alder/rev/f4c017d24f92
Whiteboard: [fixed-alder]
Depends on: 1098482
You need to log in before you can comment on or make changes to this bug.