UITour: App menu doesn't open on UX

RESOLVED FIXED in Firefox 28

Status

()

P1
blocker
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Unfocused, Assigned: MattN)

Tracking

unspecified
Firefox 28
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:M9])

Attachments

(1 attachment)

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.
Whiteboard: [Aus
Whiteboard: [Aus → [Australis:P1]
Priority: -- → P1
Whiteboard: [Australis:P1]
Severity: normal → blocker
I'll start on this now.
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Created attachment 831263 [details] [diff] [review]
v.1 Fix PanelUI.show() to not require an event (matching the docs.)
Attachment #831263 - Flags: review?(bmcbride)
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.
Attachment #831263 - Flags: feedback?(jaws)
On a quick glance this looks good, but I want to hear from Jared about the change from bug 890137.
(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 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.
Attachment #831263 - Flags: feedback?(jaws) → feedback+
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).
(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?
(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.
Attachment #831263 - Flags: review?(bmcbride) → review+

Comment 11

5 years ago
https://hg.mozilla.org/mozilla-central/rev/672ff858678a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M9][fixed-in-ux] → [Australis:M9]
Target Milestone: --- → Firefox 28
(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.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.