Closed
Bug 936187
Opened 11 years ago
Closed 11 years ago
UITour: Make highlight and info panel work for items in panel menu
Categories
(Firefox :: General, defect, P1)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 29
People
(Reporter: Unfocused, Assigned: MattN)
References
Details
Attachments
(2 files, 8 obsolete files)
8.99 KB,
patch
|
Unfocused
:
review+
|
Details | Diff | Splinter Review |
29.25 KB,
patch
|
Unfocused
:
review+
|
Details | Diff | Splinter Review |
In Australis, we need the highlight action to work for UI targets that are in the new panel menu. It's possible this may already work, and just needs some of those targets added to the whitelist.
We should consider anything in the default set for that menu, as well as Help and (especially) Customize.
Reporter | ||
Updated•11 years ago
|
Priority: -- → P1
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•11 years ago
|
||
* Fix tooltip/info z-order
* Open panel menu for highlight/info if necessary
* Add some targets
* Make sure panel is ready before trying to getTarget nodes otherwise we won't find ones located there. sync => async fun
Attachment #831390 -
Flags: feedback?(bmcbride)
Assignee | ||
Comment 2•11 years ago
|
||
Attachment 831390 [details] [diff] and this one are both made for the UX branch.
This patch mostly deals with having the highlight feature above the menu panel instead of below it. div => panel conversion was the easiest way.
Attachment #831391 -
Flags: feedback?(bmcbride)
Assignee | ||
Comment 3•11 years ago
|
||
Note to self: bookmark subview doesn't open in the panel yet
Assignee | ||
Comment 4•11 years ago
|
||
Alex, OS X test builds will be available at https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mozilla@noorenberghe.ca-b5d371a940e4/ (use the .dmg). They contain WIP fixes for this bug and bug 936378. Feel free to use them in the morning before I join all of you.
Flags: needinfo?(agibson)
Assignee | ||
Comment 5•11 years ago
|
||
And if that doesn't work, try http://people.mozilla.org/~mnoorenberghe/tmp/ux-936187-28.0a1.en-US.mac64.dmg
Reporter | ||
Comment 7•11 years ago
|
||
Comment on attachment 831390 [details] [diff] [review]
Main - WIP 1 - Make the panel work except the z-order of highlights
Review of attachment 831390 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/base/content/browser.xul
@@ +196,5 @@
>
> <!-- UI tour experience -->
> <panel id="UITourTooltip"
> type="arrow"
> + level="top"
Were you going to make the PanelUI panel not use level=top in this patch as well?
::: browser/modules/UITour.jsm
@@ +23,5 @@
> this.UITour = {
> originTabs: new WeakMap(),
> pinnedTabs: new WeakMap(),
> urlbarCapture: new WeakMap(),
> + openedAppMenuForTour: false, /* TODO: may need more than one variable or an openCount since it's necessary for info and highlight so they could leave this in a bad state */
A count seems like it would work nicely.
@@ +79,2 @@
> return false;
> + targetPromise.then((target) => {
Nit: Can reduce the amount of brackets here substantially (ditto below too). ie:
targetPromise.then(target => this.showHighlight(target));
@@ +396,5 @@
>
> tooltipTitle.textContent = aTitle;
> tooltipDesc.textContent = aDescription;
>
> + if (aAnchor.getAttribute("cui-areatype") == "menu-panel" || (aAnchor.id.startsWith("PanelUI-") && aAnchor.id != "PanelUI-button")) { // TODO: check why PanelUI-button special case wasn't needed in quick testing
Nit: Refactor this out to a targetIsInAppMenu() method.
@@ +405,2 @@
> let alignment = "bottomcenter topright";
> + let anchorRect = aAnchor.getBoundingClientRect(); // TODO: delete unused var? or is it triggering a layout update?
Think this was accidentally leftover from an earlier patch - safe to remove it.
@@ +407,4 @@
>
> tooltip.hidden = false;
> tooltip.openPopup(aAnchor, alignment);
> +}, 250); // TODO: instead of this, disable the transition on the menu panel or have the info panel wait for the menu panel transition. Without something like this, the info panel shows behind the menu panel since it opens first.
Should be able to just wait for the right event.
Attachment #831390 -
Flags: feedback?(bmcbride) → feedback+
Reporter | ||
Updated•11 years ago
|
Attachment #831391 -
Flags: feedback?(bmcbride) → feedback+
Assignee | ||
Comment 8•11 years ago
|
||
I'll do tests in a separate patch.
Attachment #831390 -
Attachment is obsolete: true
Attachment #8335200 -
Flags: review?(bmcbride)
Assignee | ||
Comment 9•11 years ago
|
||
Mostly a rebase on top of bug 939008.
I'm still not sure of a good solution to the animation clipping issue (see "15"s in the patch). I need to think about it some more.
Attachment #831391 -
Attachment is obsolete: true
Reporter | ||
Comment 10•11 years ago
|
||
Comment on attachment 8335200 [details] [diff] [review]
Main - v.1 Make the panel work except the z-order of highlights - wait for menus
Review of attachment 8335200 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/modules/UITour.jsm
@@ +302,2 @@
>
> + aWindow.PanelUI.ensureReady().then(() =>
If targetQuery is a function, it should wait for this too - since it can be in the panel menu.
Attachment #8335200 -
Flags: review?(bmcbride) → review+
Reporter | ||
Comment 11•11 years ago
|
||
Comment on attachment 8335205 [details] [diff] [review]
Annotation z-order - WIP 2 UX - Switch highlight to a <panel> so it overlays the menu panel
Review of attachment 8335205 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/themes/shared/UITour.inc.css
@@ +8,5 @@
>
> +#UITourHighlightContainer {
> + -moz-appearance: none;
> + background-color: transparent;
> + padding: 15px; /* TODO: This is the animation buffer zone hack */
Hm, yea. Can't think of any other solution off the top of my head.
Attachment #8335205 -
Flags: feedback+
Assignee | ||
Comment 12•11 years ago
|
||
Good catch about targetQuery as a function. I didn't consider that possibility for the future.
Attachment #8335200 -
Attachment is obsolete: true
Attachment #8335679 -
Flags: review+
Assignee | ||
Comment 13•11 years ago
|
||
As dicussed on IRC, I tweaked the zoom animation so it doesn't go outside the bounds of the panel otherwise it would clip. The wobble animation uses fixed offsets so I added a 4px buffer for that with a comment in both places.
Note that removing level="top" from PanelUI-popup also makes it so the menu panel is anchored to the button.
Attachment #8335205 -
Attachment is obsolete: true
Attachment #8335813 -
Flags: review?(bmcbride)
Reporter | ||
Comment 14•11 years ago
|
||
Comment on attachment 8335813 [details] [diff] [review]
Annotation z-order - v.1 - Switch highlight to a <panel> so it overlays the menu panel
Review of attachment 8335813 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/modules/UITour.jsm
@@ +412,5 @@
> + highlighter.parentElement.hidePopup();
> + }
> + /* -4 offsets come from the padding in CSS for UITourHighlightContainer
> + for the wobble animation */
> + highlighter.parentElement.openPopup(aTarget, "overlap", -4, -4);
This is still kinda hacky, but at least it's less hacky with the CSS in content rather than theme...
Attachment #8335813 -
Flags: review?(bmcbride) → review+
Assignee | ||
Comment 15•11 years ago
|
||
This just switches to using getPlacementOfWidget instead of cui-areatype as it seems more stable and I was looking into using it to avoid the PanelUI.ensureReady. That would require doing some modification to the target list though since it currently uses selectors instead of IDs so I left that.
I also added tests for the changes here which I almost forgot about.
Attachment #8335679 -
Attachment is obsolete: true
Attachment #8338866 -
Flags: review?(bmcbride)
Assignee | ||
Comment 16•11 years ago
|
||
I needed to fix some tests now.
Attachment #8335813 -
Attachment is obsolete: true
Attachment #8338867 -
Flags: review?(bmcbride)
Reporter | ||
Comment 17•11 years ago
|
||
Comment on attachment 8338866 [details] [diff] [review]
Main - v.1.2 Make the panel work except the z-order of highlights - Use getPlacementOfWidget + tests
Review of attachment 8338866 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/modules/UITour.jsm
@@ +308,5 @@
> + return deferred.promise;
> + },
> +
> + targetIsInAppMenu: function(aTarget) {
> + let placement = CustomizableUI.getPlacementOfWidget(aTarget.id);
Er, hmm - this made me realise: Neither this nor using cui-type is a good way to determine if the target is in the app menu, since it may be a child element of a widget. Such as the search provider dropdown.
One idea to solve this would be to add more info to the UITour.targets map - also specifying the ID of the base widget the target is contained in.
@@ +312,5 @@
> + let placement = CustomizableUI.getPlacementOfWidget(aTarget.id);
> + return (placement && placement.area == CustomizableUI.AREA_PANEL)
> + // Handle the non-customizable buttons at the bottom of the menu.
> + || (aTarget.id.startsWith("PanelUI-")
> + && aTarget.id != "PanelUI-menu-button");
This is kinda awful to try to read :)
Attachment #8338866 -
Flags: review?(bmcbride) → review-
Reporter | ||
Updated•11 years ago
|
Attachment #8338867 -
Flags: review?(bmcbride) → review+
Assignee | ||
Comment 18•11 years ago
|
||
I didn't end up using the lazy getter approach (from IRC) and instead went with creating a new object to return from getTarget (which we also talked about).
This may have regressed my fixes to prevent the panel from flickering open and closed when a highlight moves from one widget in the panel to another.
Attachment #8338866 -
Attachment is obsolete: true
Attachment #8342946 -
Flags: feedback?(bmcbride)
Assignee | ||
Comment 19•11 years ago
|
||
Comment on attachment 8342946 [details] [diff] [review]
Main - v.2 Make the panel work except the z-order of highlights - Addressing comment 17
(In reply to Matthew N. [:MattN] from comment #18)
> Created attachment 8342946 [details] [diff] [review]
> This may have regressed my fixes to prevent the panel from flickering open
> and closed when a highlight moves from one widget in the panel to another.
Never mind this, it just because of the menu name change from "appmenu" to "appMenu" and mozilla.org isn't updated yet.
Attachment #8342946 -
Attachment description: Main - v.2 WIP Make the panel work except the z-order of highlights - Addressing comment 17 → Main - v.2 Make the panel work except the z-order of highlights - Addressing comment 17
Attachment #8342946 -
Attachment filename: 936187_panel_highlight_v2_wip.patch → 936187_panel_highlight_v2.patch
Attachment #8342946 -
Flags: feedback?(bmcbride) → review?(bmcbride)
Assignee | ||
Comment 20•11 years ago
|
||
Fix the search widgetName so it can detect whether it's in the menu panel.
Attachment #8342946 -
Attachment is obsolete: true
Attachment #8342946 -
Flags: review?(bmcbride)
Attachment #8343298 -
Flags: review?(bmcbride)
Assignee | ||
Comment 21•11 years ago
|
||
Alex, can you update your server-side code in the next few days to use camelCase for target and menu name arguments. I think this only affects you for "appmenu" => "appMenu"
Thanks
Flags: needinfo?(agibson)
Reporter | ||
Comment 22•11 years ago
|
||
(In reply to Matthew N. [:MattN] from comment #21)
> Alex, can you update your server-side code in the next few days to use
> camelCase for target and menu name arguments. I think this only affects you
> for "appmenu" => "appMenu"
This makes me think we should do case-insensitive matching.
Reporter | ||
Comment 23•11 years ago
|
||
Comment on attachment 8343298 [details] [diff] [review]
Main - v.2 Make the panel work except the z-order of highlights - Fix search widgetName
Review of attachment 8343298 [details] [diff] [review]:
-----------------------------------------------------------------
Couple of small fixups, but otherwise good to go!
::: browser/modules/UITour.jsm
@@ +32,4 @@
>
> highlightEffects: ["wobble", "zoom", "color"],
> targets: new Map([
> + ["addons", {target: "#add-ons-button"}],
I'm finding the name of the "target" property mildly confusing, since each of these objects is describing a target. Rename to "query"?
@@ +103,2 @@
> return false;
> + targetPromise.then(target => {
Should add an error handler to these promises, otherwise we'll get a warning logged - intended for accidental absence of an error handler.
Also, seems cleaner to just have getTarget always return a promise, and have all error conditions go through the error handler.
@@ +332,5 @@
> + let targetQuery = targetObject.target;
> + aWindow.PanelUI.ensureReady().then(() => {
> + if (typeof targetQuery == "function") {
> + deferred.resolve({
> + targetEl: targetQuery(aWindow.document),
This feels akin to saying "ATM machine".
s/targetEl/node/ ?
@@ +448,2 @@
>
> + let randomEffect = Math.floor(Math.random() * this.highlightEffects.length);
Bitrot alert!
@@ +535,2 @@
> aWindow.PanelUI.show();
> + } else if (aMenuName == "bookmarks")
Nit: Unbalanced braces of if and else.
@@ +549,2 @@
> aWindow.PanelUI.hide();
> + } else if (aMenuName == "bookmarks")
Ditto.
Attachment #8343298 -
Flags: review?(bmcbride) → review+
Comment 24•11 years ago
|
||
(In reply to Matthew N. [:MattN] from comment #21)
> Alex, can you update your server-side code in the next few days to use
> camelCase for target and menu name arguments. I think this only affects you
> for "appmenu" => "appMenu"
>
> Thanks
Thanks, Matt
I'll update the code on my end as soon as it lands in Nightly.
Flags: needinfo?(agibson)
Assignee | ||
Comment 25•11 years ago
|
||
(In reply to Blair McBride [:Unfocused] from comment #23)
> Comment on attachment 8343298 [details] [diff] [review]
> Main - v.2 Make the panel work except the z-order of highlights - Fix search
> widgetName
>
> Also, seems cleaner to just have getTarget always return a promise, and have
> all error conditions go through the error handler.
I intentionally didn't do this in order to allow onPageEvent to return false in cases where the target is unknown so we can avoid doing a teardown in some cases. It isn't ideal that the return value is only accurate in some cases so maybe we can get rid of that return value check? Is the page able to see the return value (directly or indirectly)? If we want to know whether a UITour event was actually handled, we should switch to an async method with a callback or promises.
Thoughts?
Flags: needinfo?(bmcbride)
Reporter | ||
Comment 26•11 years ago
|
||
(In reply to Matthew N. [:MattN] from comment #25)
> I intentionally didn't do this in order to allow onPageEvent to return false
> in cases where the target is unknown so we can avoid doing a teardown in
> some cases. It isn't ideal that the return value is only accurate in some
> cases so maybe we can get rid of that return value check?
Ah. That's really only an opportunistic performance optimization - as long as onPageEvent will never return false when the event is handled, we're fine. ie, false has to be accurate, true doesn't have to be.
> Is the page able to see the return value (directly or indirectly)?
No.
> If we want to know whether
> a UITour event was actually handled, we should switch to an async method
> with a callback or promises.
Indeed. So the question is, is it worth it?
IMO, if keeping the current behaviour isn't costing us much (which I don't think it is), we should just keep it as-is for now. It lets us avoid unneeded work for the simple cases of when the event is malformed. It's not perfect, but it doesn't have to be.
I don't think moving to be async and trying to make it always accurate is worth the cost at the moment.
Flags: needinfo?(bmcbride)
Assignee | ||
Comment 27•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/d52f80876573
https://hg.mozilla.org/integration/fx-team/rev/8c0d1c91905c
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Updated•11 years ago
|
Flags: in-testsuite+
Comment 28•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d52f80876573
https://hg.mozilla.org/mozilla-central/rev/8c0d1c91905c
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
You need to log in
before you can comment on or make changes to this bug.
Description
•