936187 - UITour: Make highlight and info panel work for items in panel menu

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 1

[Matthew N. [:MattN]]

Attachment: Main - WIP 1 - Make the panel work except the z-order of highlights

* 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

Comment 2

[Matthew N. [:MattN]]

Attachment: Highlight z-order - WIP 1 UX - Switch highlight to a <panel> so it overlays the menu panel

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.

Comment 3

[Matthew N. [:MattN]]

Note to self: bookmark subview doesn't open in the panel yet

Comment 4

[Matthew N. [:MattN]]

Alex, OS X test builds will be available at https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-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.

Comment 5

[Matthew N. [:MattN]]

And if that doesn't work, try http://people.mozilla.org/~mnoorenberghe/tmp/ux-936187-28.0a1.en-US.mac64.dmg

Comment 6

[Alex Gibson [:agibson]]

Thanks MattN, downloading and testing now :)

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 8

[Matthew N. [:MattN]]

Attachment: Main - v.1 Make the panel work except the z-order of highlights - wait for menus

I'll do tests in a separate patch.

Comment 9

[Matthew N. [:MattN]]

Attachment: Annotation z-order - WIP 2 UX - Switch highlight to a <panel> so it overlays the menu panel

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.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 12

[Matthew N. [:MattN]]

Attachment: Main - v.1.1 Make the panel work except the z-order of highlights - wait for menus. r=Unfocused

Good catch about targetQuery as a function. I didn't consider that possibility for the future.

Comment 13

[Matthew N. [:MattN]]

Attachment: Annotation z-order - v.1 - Switch highlight to a <panel> so it overlays the menu panel

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.

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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...

Comment 15

[Matthew N. [:MattN]]

Attachment: Main - v.1.2 Make the panel work except the z-order of highlights - Use getPlacementOfWidget + tests

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.

Comment 16

[Matthew N. [:MattN]]

Attachment: Annotation z-order - v.1.1 - Switch highlight to a <panel> so it overlays the menu panel - w/ test fixes

I needed to fix some tests now.

Comment 17

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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 :)

Comment 18

[Matthew N. [:MattN]]

Attachment: Main - v.2 Make the panel work except the z-order of highlights - Addressing comment 17

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.

Comment 19

[Matthew N. [:MattN]]

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.

Comment 20

[Matthew N. [:MattN]]

Attachment: Main - v.2 Make the panel work except the z-order of highlights - Fix search widgetName

Fix the search widgetName so it can detect whether it's in the menu panel.

Comment 21

[Matthew N. [:MattN]]

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

Comment 22

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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.

Comment 23

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 24

[Alex Gibson [:agibson]]

(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.

Comment 25

[Matthew N. [:MattN]]

(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?

Comment 26

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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.

Comment 27

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/d52f80876573
https://hg.mozilla.org/integration/fx-team/rev/8c0d1c91905c

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d52f80876573
https://hg.mozilla.org/mozilla-central/rev/8c0d1c91905c