1389283 - [UITour] Highlighting appmenu items breaks click functionality of submenus

Status: VERIFIED FIXED

(Firefox :: Tours, defect, P1)

Description

[Verdi [:verdi] Best to slack me]

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

Comment 1

[KM Lee [:rexboy] (inactivate after Sep. 22)]

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.

Comment 2

[KM Lee [:rexboy] (inactivate after Sep. 22)]

For now it looks like a regression from bug 1382579. Fischer is trying on figure out the cause.

Comment 3

[Fischer [:Fischer]]

Attachment: Bug 1389283 - [UITour] Highlighting appmenu items breaks click functionality of submenus,

Review commit: https://reviewboard.mozilla.org/r/168002/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168002/

Comment 4

[Fischer [:Fischer]]

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

Clear the accidental r? triggered by MozReview

Comment 5

[Fischer [:Fischer]]

(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

Comment 6

[Fischer [:Fischer]]

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

Comment 7

[:Gijs (he/him)]

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.

Comment 8

[Fischer [:Fischer]]

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 9

[Fischer [:Fischer]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168002/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/040126f6241c
[UITour] Highlighting appmenu items breaks click functionality of submenus, r=Gijs

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/040126f6241c

Comment 12

[Justin [:JW_SoftvisionQA]]

I have verified this fix on Today's nightly.

Comment 13

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

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.