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)
Firefox
General
Tracking
()
People
(Reporter: Dolske, Assigned: Dolske)
References
Details
Attachments
(1 file, 3 obsolete files)
6.03 KB,
patch
|
Dolske
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.)
Updated•10 years ago
|
Flags: qe-verify+
Flags: firefox-backlog+
Updated•10 years ago
|
Flags: qe-verify+ → qe-verify-
Comment 1•10 years ago
|
||
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?
Comment 2•10 years ago
|
||
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?
Comment 3•10 years ago
|
||
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.
Assignee | ||
Comment 4•10 years ago
|
||
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.
Comment 5•10 years ago
|
||
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)
Comment 6•10 years ago
|
||
(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").
Assignee | ||
Comment 7•10 years ago
|
||
This seems to basically to the job, needs a little more work and a test.
Assignee: nobody → dolske
Assignee | ||
Updated•10 years ago
|
Iteration: --- → 34.3
Points: --- → 3
Updated•10 years ago
|
Status: NEW → ASSIGNED
Iteration: 34.3 → 35.3
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8496215 -
Attachment is obsolete: true
Attachment #8498983 -
Flags: review?(MattN+bmo)
Comment 9•10 years ago
|
||
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+
Assignee | ||
Comment 10•10 years ago
|
||
(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.
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8498983 -
Attachment is obsolete: true
Attachment #8499751 -
Flags: review?(MattN+bmo)
Comment 12•10 years ago
|
||
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+
Assignee | ||
Comment 13•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8499751 -
Attachment is obsolete: true
Assignee | ||
Comment 14•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/ac12f6f24545
Comment 15•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ac12f6f24545
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Comment 16•10 years ago
|
||
I've opened a pull request to add this to mozilla-uitour: https://github.com/Unfocused/mozilla-uitour/pull/8
Assignee | ||
Comment 17•10 years ago
|
||
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).
Assignee | ||
Comment 18•10 years ago
|
||
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+
Comment 19•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/1c96180e6a5b
status-firefox34:
--- → fixed
Updated•10 years ago
|
status-firefox33:
--- → affected
status-firefox35:
--- → fixed
Assignee | ||
Comment 20•10 years ago
|
||
Landed in alder: https://hg.mozilla.org/projects/alder/rev/f4c017d24f92
Whiteboard: [fixed-alder]
Comment 21•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-release/rev/f4c017d24f92
Flags: in-testsuite+
Whiteboard: [fixed-alder]
Updated•10 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•