949659 - UITour: When highlighting item in menu panel, hiding the menu does not hide the highlight

Status: VERIFIED FIXED

(Firefox :: General, defect, P2)

Description

[Alex Gibson [:agibson]]

STR:

- Visit: https://www-demo3.allizom.org/b/en-US/firefox/australis/update/

- Start the tour and enter step where the menu panel shows, highlighting addons.

- Within the menu panel, click on 'Find'

Expected results:

- The menu panel should hide, as well as the highlighted icon

Actual results:

- The menu panel hides, but the highlight remains visible

Comment 1

[Matthew N. [:MattN]]

I wish this could be handled in the popup code so that if a target is specified and it disappears that the panel would also disappear. This would reduce or remove the need for the _isElementVisible checks I added too. That wouldn't be perfect as appMenuOpenForAnnotation would be out-of-sync (see below) but that is usually a smaller issue for the user.

We'll probably have to add a listener to clear appMenuOpenForAnnotation and hide any annotations that were in that Set when the menu panel closes due to something outside of UITour.jsm.

Comment 2

[Zhenshuo Fang (:fang) - Firefox UX Team]

Also related to this, when you click on an item in the menu that has a subview(say history or help), the highlight is still at the same location, we should hind it. 

Not sure this is the same bug, if not I can file a separate bug.

Comment 3

[Matthew N. [:MattN]]

Good point, they may be able to have the same/similar fix so we can leave it here for now.

Comment 4

[Matthew N. [:MattN]]

Attachment: v.1 Add @targetName to panels and listen for popuphiding

This patch depends on bug 967370.

@targetName is used to know the target that an an annotation panel is supposed to be anchored to in order to see if it's in the menu panel. (I could have just checked the popup's anchorNode and traversed the DOM to the root to see if it's in the menu panel but that seemed less efficient).

@targetName will also be useful for custom styling of certain targets e.g. whether a target should be a circle, or making the account button use a rectangular highlight (no border-radius) as was proposed at one point.

The popuphiding listener checks to see if any of the annotation types is open with a target in the app menu. If so, it hides that annotation.

I didn't end up fixing comment 2 in this bug so I will file a follow-up.

Comment 5

[Matthew N. [:MattN]]

As an FYI, once we hide an annotation due to the menu panel closing, we won't re-show the annotation if the user re-opens the menu panel on their own. If re-annoating is desired, I think we can deal with that in the webpage once bug 941862 is fixed.

I think it's probably fine to stay out of the user's way until they click the arrow to go to the next step of the tour.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Matthew N. [:MattN] from comment #4)
> I didn't end up fixing comment 2 in this bug so I will file a follow-up.

For reference, bug 967391.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8369897 [details] [diff] [review]
v.1 Add @targetName to panels and listen for popuphiding

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

::: browser/modules/UITour.jsm
@@ +736,5 @@
>  
>      if (aMenuName == "appMenu") {
>        aWindow.PanelUI.panel.removeAttribute("noautohide");
>        aWindow.PanelUI.hide();
> +      this.appMenuOpenForAnnotation.clear();

Isn't this already handled by onAppMenuHiding?

Comment 8

[Matthew N. [:MattN]]

Yep, removed that line.

https://hg.mozilla.org/integration/fx-team/rev/a07a4dba6a8d

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8369897 [details] [diff] [review]
v.1 Add @targetName to panels and listen for popuphiding

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis UI tour for initial Aurora release
User impact if declined: A highlight will be left floating on the screen if the user closes the menu panel themselves (through various means).
Testing completed (on m-c, etc.): Manual and browser-chrome test on m-c
Risk to taking this patch (and alternatives if risky): Worst case is that this code interferes with the menu panel outside of the tour tab. The risk is low-moderate but it should be easy to pinpoint if there is an issue.
String or IDL/UUID changes made by this patch: None

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a07a4dba6a8d

Comment 11

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8b8bd4fa5fd2

Comment 12

[Cornel Ionce [:noni] [Hubs QA]]

Verified as fixed on latest Nightly (build ID: 20140302030203) latest Aurora (build ID: 20140303004002).

Tested on: Ubuntu 12.04 32bit, Windows 7 64 bit, Windows 8.1 32bit and Mac OS X 10.8.