[UITour] Highlighting appmenu items breaks click functionality of submenus

VERIFIED FIXED in Firefox 57

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: verdi, Assigned: Fischer)

Tracking

({regression})

57 Branch
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57 verified)

Details

(Whiteboard: [photon-onboarding])

Attachments

(2 attachments)

Reporter

Description

2 years ago
Posted video library.mp4
This is probably going to take a video to fully explain...

STR:
1. Open the tour to the add-on panel (it really doesn't matter which one though).
2. Click the show add-ons in menu button
3. Instead of clicking on the highlighted add-ons item in the menu, accidentally click on the Library button.

Expected result: The library submenu should appear.
Actual result: The menu disappears. Clicking the show button again opens the menu with library button highlighted and none of the buttons in the menu work.

You then have to close the overlay to get out this situation. Opening the menu again shows the library button highlighted and the menu becomes responsive after a few clicks.

This will also happen if you accidentally click on some of the other menu items like Help or Web developer.
Reporter

Updated

2 years ago
Summary: Clicking the library button in the menu breaks the onboarding tour → Clicking the wrong button in the menu breaks the onboarding tour
Just gave it a try and seems it just happens similarily on library tour, which means on library tour you can't open the library...
This is going to be a defeat. Investigating.
For now it looks like a regression from bug 1382579. Fischer is trying on figure out the cause.
Depends on: 1382579
Keywords: regression
Summary: Clicking the wrong button in the menu breaks the onboarding tour → [UITour] Highlighting appmenu items breaks click functionality of submenus
Assignee: nobody → fliu
Flags: qe-verify+
Priority: -- → P1
QA Contact: jwilliams
Whiteboard: [photon-onboarding][triage] → [photon-onboarding]
Version: 56 Branch → 57 Branch
Comment hidden (mozreview-request)
Assignee

Comment 4

2 years ago
Comment on attachment 8896703 [details]
Bug 1389283 - [UITour] Highlighting appmenu items breaks click functionality of submenus,

Clear the accidental r? triggered by MozReview
Attachment #8896703 - Flags: review?(gijskruitbosch+bugs)
Assignee

Updated

2 years ago
Component: General → Tours
Assignee

Comment 5

2 years ago
(In reply to Verdi [:verdi] from comment #0)
> Created attachment 8896022 [details]
> library.mp4
> 
> This is probably going to take a video to fully explain...
> 
> STR:
> 1. Open the tour to the add-on panel (it really doesn't matter which one
> though).
> 2. Click the show add-ons in menu button
> 3. Instead of clicking on the highlighted add-ons item in the menu,
> accidentally click on the Library button.
> 
> Expected result: The library submenu should appear.
> Actual result: The menu disappears. Clicking the show button again opens the
> menu with library button highlighted and none of the buttons in the menu
> work.
> 
Root Cause: 

In the old codes, UITour both did `hideAppMenuAnnotations` in the "popuphiding" and "ViewShowing" event. So
1. When the submenu(subview) was being shown, `hideAppMenuAnnotations` would be called[1]
2. It cleared `appMenuOpenForAnnotation` while hiding annotations for the appMenu[2]
3. later it would call `hideHighlight` to hide the highlight annotation[3]
4. In `hideHighlight`, it not only hid the highlight element but also closed the appMenu[4]
5. However, because `appMenuOpenForAnnotation` was cleared in the step 2, it would think no need to close the menu (to some degree initially the appMenu was opened for `showHighlight`).[5]
6. As a result, UITour wouldn't close the appMenu while showing the library subview.

However, in the new codes, UITour no longer uses `appMenuOpenForAnnotation` so it would close the appMenu while the library subview is being shown. Thus, the unexpected issue comes.

In the "popuphiding" event, we want to hide both annotation and menu. But in the "ViewShowing", in fact, we only want to hide annotations and don't have to hide menu. Thus we probably could make these intentions more explicit, which is to hide both annotation and menu in the "popuphiding" and to only hide annotations in the "ViewShowing" event.

[1] http://searchfox.org/mozilla-central/rev/d6ba3d20ff8f57ef674544a0ba22e566caba1cce/browser/components/uitour/UITour.jsm#1286
[2] http://searchfox.org/mozilla-central/rev/d6ba3d20ff8f57ef674544a0ba22e566caba1cce/browser/components/uitour/UITour.jsm#1423
[3] http://searchfox.org/mozilla-central/rev/d6ba3d20ff8f57ef674544a0ba22e566caba1cce/browser/components/uitour/UITour.jsm#1419
[4] http://searchfox.org/mozilla-central/rev/d6ba3d20ff8f57ef674544a0ba22e566caba1cce/browser/components/uitour/UITour.jsm#1110
[5] http://searchfox.org/mozilla-central/rev/d6ba3d20ff8f57ef674544a0ba22e566caba1cce/browser/components/uitour/UITour.jsm#963
Assignee

Comment 6

2 years ago
Comment on attachment 8896703 [details]
Bug 1389283 - [UITour] Highlighting appmenu items breaks click functionality of submenus,

Hi Gijs,
 
Please see the comment 5 for the root cause.
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d29cb546226 

Thanks
Attachment #8896703 - Flags: review?(gijskruitbosch+bugs)

Comment 7

2 years ago
mozreview-review
Comment on attachment 8896703 [details]
Bug 1389283 - [UITour] Highlighting appmenu items breaks click functionality of submenus,

https://reviewboard.mozilla.org/r/168002/#review173382

Thanks for the detailed explanation and test.

::: browser/components/uitour/UITour.jsm:1387
(Diff revision 1)
>        }
>        if (aOpenCallback) {
>          menu.node.addEventListener("popupshown", aOpenCallback, { once: true });
>        }
>        menu.node.addEventListener("popuphidden", menu.onPanelHidden);
> -      menu.node.addEventListener("popuphiding", menu.hideMenuAnnotations);
> +      menu.node.addEventListener("popuphiding", menu.onPopuphiding);

Nit: here and elsewhere, camel-case should be `onPopupHiding`, to be consistent with `onPanelHidden`, `onViewShowing`, etc.
Attachment #8896703 - Flags: review?(gijskruitbosch+bugs) → review+
Assignee

Comment 8

2 years ago
mozreview-review-reply
Comment on attachment 8896703 [details]
Bug 1389283 - [UITour] Highlighting appmenu items breaks click functionality of submenus,

https://reviewboard.mozilla.org/r/168002/#review173382

> Nit: here and elsewhere, camel-case should be `onPopupHiding`, to be consistent with `onPanelHidden`, `onViewShowing`, etc.

Updated, thanks
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 10

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/040126f6241c
[UITour] Highlighting appmenu items breaks click functionality of submenus, r=Gijs
Keywords: checkin-needed

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/040126f6241c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Assignee

Updated

2 years ago
No longer blocks: 1391071
I have verified this fix on Today's nightly.
Status: RESOLVED → VERIFIED
I can confirm this issue is fixed on beta as well. I verified using Fx 57.0b7 on Windows 10 x64, Ubuntu 14.04 LTS and macOS X 10.12.6.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.