1636243 - Disfunctional AppMenu behaviour during specific interactions

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, defect)

Description

[Alessandro Castellani [:aleca]]

Attachment: cursor-appmenu.png

I stumbled upon a couple of glitches in the main AppMenu that should be addressed.

• The AppMenu opens and closes immediately if opened the first time in the Chat Tab.
  To recreate the issue you have to be in the Chat Tab and open the AppMenu for the first time.
  If the AppMenu has been previously opened in any other tab, the issue cannot be reproduced.

• Unable to close the AppMenu if clicking on the AppMenu Button when the cursor is on top of the area where the chevron overlaps the button (attachment for reference).
  The console returns: JavaScript error: resource:///modules/CustomizableUI.jsm, line 2040: TypeError: target.getAttribute is not a function

This problem affects also Firefox, and I think we could resolve it by pushing the AppMenu down in order to not have any overlap with the button.

Comment 1

[Richard Marti (:Paenglab)]

(In reply to Alessandro Castellani (:aleca) from comment #0)

• The AppMenu opens and closes immediately if opened the first time in the Chat Tab.
  To recreate the issue you have to be in the Chat Tab and open the AppMenu for the first time.
  If the AppMenu has been previously opened in any other tab, the issue cannot be reproduces.

The same happens on the calendar tab.

Comment 2

[Alessandro Castellani [:aleca]]

Found another tiny issue.

Clicking on Messages causes this console error:

JavaScript error: chrome://messenger/content/mailWindowOverlay.js, line 831:
TypeError: openFeedView.children.forEach is not a function

Comment 3

[Alessandro Castellani [:aleca]]

Attachment: 1636243-appmenu.diff

This patch fixes the issues listed above.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9181447 [details] [diff] [review]
1636243-appmenu.diff

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

Seems ok, r=mkmelin

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/cca8a5a4b9d1
Fix disfunctional AppMenu behaviour during specific interactions. r=mkmelin DONTBUILD

Comment 6

[Magnus Melin [:mkmelin]]

Backed out due to test failures: https://hg.mozilla.org/comm-central/rev/a7b7754ea5e9fe9daca61bef86fe21ca79408cdb

Comment 7

[Alessandro Castellani [:aleca]]

Sorry about that.
The test opens the main app menu without passing any event, so no originalTarget is available.
I fixed it and launched a try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d29d5c7c7ba4d05137d56bfe762fa63772b1f450

Comment 8

[Alessandro Castellani [:aleca]]

Attachment: 1636243-appmenu.diff

Patch updated with tests fixed.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9181649 [details] [diff] [review]
1636243-appmenu.diff

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

::: mail/components/customizableui/content/panelUI.js
@@ +275,5 @@
>          domEvent = aEvent;
>        }
>  
> +      // If the opening of the menu is triggered from a test, we don't have an
> +      // event nor a target. We try to use the originalTarget to account for

I don't think the main code should cater for tests - then you would get tests that don't test what they should.
But I think it's valid also for other cases, especially since the documentation says " @param aEvent the event (if any) that triggers showing the menu."

So please adjust the comment not to be about catering for tests.

Comment 10

[Alessandro Castellani [:aleca]]

Attachment: 1636243-appmenu.diff

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/54b01f3c6348
Fix disfunctional AppMenu behaviour during specific interactions. r=mkmelin

Comment 12

[Magnus Melin [:mkmelin]]

78 uplift?

Comment 13

[Alessandro Castellani [:aleca]]

Comment on attachment 9181662 [details] [diff] [review]
1636243-appmenu.diff

[Approval Request Comment]
Regression caused by (bug #): -
User impact if declined: Minor, as the fixed issues are not blockers
Testing completed (on c-c, etc.): on c-c and beta 83
Risk to taking this patch (and alternatives if risky): very low

Comment 14

[Wayne Mery (:wsmwk)]

Comment on attachment 9181662 [details] [diff] [review]
1636243-appmenu.diff

[Triage Comment]
Approved for esr78

Comment 15

[Rob Lemley [:rjl]]

Thunderbird 78.4.1:
https://hg.mozilla.org/releases/comm-esr78/rev/0e009df26a2f