1322876 - "contextmenu" event has mozInputSource == MOZ_SOURCE_MOUSE even when hitting the context menu key on the keyboard

Status: NEW

(Core :: DOM: Events, defect, P3)

Description

[Jim Porter (:squib)]

Per the bug title, the "contextmenu" event always claims to be from the mouse even when it's actually from the keyboard. That's because when the WidgetMouseEvent is created, the input source is never overridden: <https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/widget/MouseEvents.h#252>.

I have a test case and a patch that I'll upload shortly. This is my first (or maybe my second?) m-c bug, so I might get some of the review protocol wrong; if I do anything silly, just let me know!

Comment 1

[Jim Porter (:squib)]

Attachment: A simple test case

Here's a simple test case that logs the input source in the error console. Currently, it always outputs 1 (mouse), even when the event comes from the keyboard.

Comment 2

[Jim Porter (:squib)]

Attachment: "contextmenu" event has mozInputSource == MOZ_SOURCE_MOUSE even when hitting the context menu key on the keyboard

Comment 3

[Jim Porter (:squib)]

Comment on attachment 8819081 [details] [diff] [review]
"contextmenu" event has mozInputSource == MOZ_SOURCE_MOUSE even when hitting the context menu key on the keyboard

Hopefully I flagged the right person for review and managed to upload this patch correctly. Where should I put tests for this? I'm not super-familiar with how m-c organizes its tests.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8819081 [details] [diff] [review]
"contextmenu" event has mozInputSource == MOZ_SOURCE_MOUSE even when hitting the context menu key on the keyboard

It is not super important where to put tests. Possibly dom/events/test/*

You aren't setting button to any value when mContextMenuTrigger == eContextMenuKey.

Comment 5

[Jim Porter (:squib)]

(In reply to Olli Pettay [:smaug] from comment #4)
> It is not super important where to put tests. Possibly dom/events/test/*

Thanks! I'll look there and see if I can cargo-cult an existing test into something that checks this.

> You aren't setting button to any value when mContextMenuTrigger ==
> eContextMenuKey.

I was debating what to do here; It's automatically set to 0 in the WidgetMouseEventBase constructor (as is inputSource)[1], but it's theoretically possible that that would change in the future and then we'd have an uninitialized member. I suppose I'd want to either a) do what I'm doing now and rely on the base class to initialize its members, or b) set `button` *and* `inputSource` in both branches. I'm open to either; is there a general guideline I should follow here for Mozilla code?

[1] https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/widget/MouseEvents.h#94

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

The general guideline is something along the lines "whatever makes the code easiest to read and maintain, unless there are some performance or security related reasons to do otherwise".
So, in this case explicit initialization would be nice.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 8

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Still valid, and the WIP in the attachment 8819081 [details] [diff] [review] is still pointing out the right direction.