Closed Bug 949659 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: General, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified

People

(Reporter: agibson, Assigned: MattN)

References

Details

Attachments

(1 file)

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
Summary: When highlighting item in menu panel, hiding the menu does not hide the highlight → UITour: When highlighting item in menu panel, hiding the menu does not hide the highlight
Blocks: fx-UITour
Priority: -- → P2
Whiteboard: [maybe-fix-on-aurora]
Whiteboard: [maybe-fix-on-aurora] → [can-fix-on-aurora]
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.
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.
Good point, they may be able to have the same/similar fix so we can leave it here for now.
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
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.
Attachment #8369897 - Flags: review?(bmcbride)
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.
(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 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?
Attachment #8369897 - Flags: review?(bmcbride) → review+
Yep, removed that line.

https://hg.mozilla.org/integration/fx-team/rev/a07a4dba6a8d
Whiteboard: [can-fix-on-aurora] → [fixed-in-fx-team]
Flags: in-testsuite+
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
Attachment #8369897 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/a07a4dba6a8d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
Attachment #8369897 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.