936378 - UITour: App menu doesn't open on UX

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Was just testing something on the prototype tour page earlier today, and found the app menu wasn't opening for me on UX branch. We, uh, should fix that.

Comment 1

[Matthew N. [:MattN]]

I'll start on this now.

Comment 2

[Matthew N. [:MattN]]

Attachment: v.1 Fix PanelUI.show() to not require an event (matching the docs.)

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 831263 [details] [diff] [review]
v.1 Fix PanelUI.show() to not require an event (matching the docs.)

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

::: browser/components/customizableui/content/panelUI.js
@@ -128,5 @@
>          updateEditUIVisibility();
>        }
>  
>        let anchor;
> -      if (aEvent.type == "mousedown" ||

Jared, I can't tell why you added this in bug 890137 but it seems unnecessary.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

On a quick glance this looks good, but I want to hear from Jared about the change from bug 890137.

Comment 5

[Matthew N. [:MattN]]

(In reply to Blair McBride [:Unfocused] from comment #4)
> On a quick glance this looks good, but I want to hear from Jared about the
> change from bug 890137.

I can obviously leave that out since it's not directly related to this bug. I just noticed that it seemed out of place while I was adding another condition here.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 831263 [details] [diff] [review]
v.1 Fix PanelUI.show() to not require an event (matching the docs.)

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

(In reply to Matthew N. [:MattN] (catching up on reviews) from comment #3)
> > -      if (aEvent.type == "mousedown" ||
> 
> Jared, I can't tell why you added this in bug 890137 but it seems
> unnecessary.

I was just adding to the list of aEvent.type == "command" when I added the mousedown event listener. This change looks fine.

Comment 7

[Alex Gibson [:agibson]]

fyi - I notice in testing this, if I call Mozilla.UITour.showInfo() when targeting appmenu, it opens the menu as well as showing an info panel, which seems unexpected (it should just show an info panel in this instance).

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Alex Gibson [:agibson] from comment #7)
> fyi - I notice in testing this, if I call Mozilla.UITour.showInfo() when
> targeting appmenu, it opens the menu as well as showing an info panel, which
> seems unexpected (it should just show an info panel in this instance).

Hmm, I can't reproduce this. Show it to Matt?

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Blair McBride [:Unfocused] from comment #8)
> (In reply to Alex Gibson [:agibson] from comment #7)
> > fyi - I notice in testing this, if I call Mozilla.UITour.showInfo() when
> > targeting appmenu, it opens the menu as well as showing an info panel, which
> > seems unexpected (it should just show an info panel in this instance).
> 
> Hmm, I can't reproduce this. Show it to Matt?

Er, n/m - I see that was due to bug 936187, and Matt knows about it already.

Comment 10

[Matthew N. [:MattN]]

Thanks.

https://hg.mozilla.org/projects/ux/rev/672ff858678a

Comment 11

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/672ff858678a

Comment 12

[u279076]

(In reply to :Gijs Kruitbosch (Unavailable Dec 19 - Jan 2) from comment #11)
> https://hg.mozilla.org/mozilla-central/rev/672ff858678a

Assuming this is sufficiently covered in-testsuite. Please add the verifyme keyword if this needs QA testing.