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.

Comment 9

[Swarup Ukil]

Hi could I work on this bug?

If its available, I'd like to clarify if I am understanding the desired behavior. Right now as it stands, anyone clicking on the keyboard "context key" results in Firefox considering it a primary mouse click. As it was not a mouse click, we should assign eNotPressed to mButton when the "context key" is pressed (regarding WidgetPointerEvent's constructor specifically)?

Also in considering the two constructors listed here: https://searchfox.org/mozilla-central/source/widget/MouseEvents.h line 813-827, we can only account for this behavior in the 818-827 constructor through aContextMenuTrigger right? Meaning we have to leave the 813 constructor as it is?

Thank you very much!

Comment 10

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

(In reply to Swarup Ukil from comment #9)

Hi could I work on this bug?

Sure thing! Thank you for being interested in working on this.

If its available, I'd like to clarify if I am understanding the desired behavior. Right now as it stands, anyone clicking on the keyboard "context key" results in Firefox considering it a primary mouse click. As it was not a mouse click, we should assign eNotPressed to mButton when the "context key" is pressed (regarding WidgetPointerEvent's constructor specifically)?

Also in considering the two constructors listed here: https://searchfox.org/mozilla-central/source/widget/MouseEvents.h line 813-827, we can only account for this behavior in the 818-827 constructor through aContextMenuTrigger right? Meaning we have to leave the 813 constructor as it is?

Forwarding the needinfo request to :smaug, who can clarify and explain better than I. :)

Thank you very much!

Comment 11

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

For the mButton handling, worth to check what other browsers do.

And both constructors should be tweaked, I think. For consistency.

Comment 12

[Swarup Ukil]

Attachment: Bug 1322876 - Assign mInputSource to keyboard for "contextmenu" event when context key is pressed. r=smaug,masayuki

Comment 13

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

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

Comment 14

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

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit BugBot documentation.