Bug 187998 - [WPE] Implement MouseEvent.buttons
Summary: [WPE] Implement MouseEvent.buttons
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WPE WebKit (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-07-25 04:48 PDT by Carlos Garcia Campos
Modified: 2018-09-18 05:04 PDT (History)
4 users (show)

See Also:


Attachments
WIP (10.26 KB, patch)
2018-07-25 04:52 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (19.43 KB, patch)
2018-08-03 02:22 PDT, Carlos Garcia Campos
zan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2018-07-25 04:48:32 PDT
https://www.w3.org/TR/uievents/#ref-for-dom-mouseevent-buttons-1

This requires new API in WPEBackend.
Comment 1 Carlos Garcia Campos 2018-07-25 04:52:56 PDT
Created attachment 345753 [details]
WIP

The patch should include a WPEBackend version bump, it's WIP because we need a new WPEBackend release with the new API.
Comment 2 Adrian Perez 2018-07-25 10:27:54 PDT
Comment on attachment 345753 [details]
WIP

How about other fields from the MouseEvent DOM object? For example, are
keyboard modifiers (Ctrl, Alt, etc) already being filled or would it be
needed to add more fields in the wpe_input_pointer_event struct?
Comment 3 Carlos Garcia Campos 2018-07-26 00:42:25 PDT
(In reply to Adrian Perez from comment #2)
> Comment on attachment 345753 [details]
> WIP
> 
> How about other fields from the MouseEvent DOM object? For example, are
> keyboard modifiers (Ctrl, Alt, etc) already being filled or would it be
> needed to add more fields in the wpe_input_pointer_event struct?

The spec doesn't say keyboard modifiers should be filled in buttons field.
Comment 4 Adrian Perez 2018-07-26 09:05:42 PDT
(In reply to Carlos Garcia Campos from comment #3)
> (In reply to Adrian Perez from comment #2)
> > Comment on attachment 345753 [details]
> > WIP
> > 
> > How about other fields from the MouseEvent DOM object? For example, are
> > keyboard modifiers (Ctrl, Alt, etc) already being filled or would it be
> > needed to add more fields in the wpe_input_pointer_event struct?
> 
> The spec doesn't say keyboard modifiers should be filled in buttons field.

The spec mentions that there are *other* fields for the keyboard
modifiers. What I wanted to point out is that the proposed patch
does not handle them, and I was wondering whether other field(s)
in the struct should be added to cover those. I guess that could
be handled in a separate bug+patch.

(FWIW, after re-reading the spec a couple of times, to me is quite
clear that the “MouseEvent.{ctrl,shift,alt,meta}Key” properties
are *mandatory* — nothing in the spec hints at them being optional.)
Comment 5 Carlos Garcia Campos 2018-07-26 22:52:36 PDT
(In reply to Adrian Perez from comment #4)
> (In reply to Carlos Garcia Campos from comment #3)
> > (In reply to Adrian Perez from comment #2)
> > > Comment on attachment 345753 [details]
> > > WIP
> > > 
> > > How about other fields from the MouseEvent DOM object? For example, are
> > > keyboard modifiers (Ctrl, Alt, etc) already being filled or would it be
> > > needed to add more fields in the wpe_input_pointer_event struct?
> > 
> > The spec doesn't say keyboard modifiers should be filled in buttons field.
> 
> The spec mentions that there are *other* fields for the keyboard
> modifiers. What I wanted to point out is that the proposed patch
> does not handle them, and I was wondering whether other field(s)
> in the struct should be added to cover those. I guess that could
> be handled in a separate bug+patch.
> 
> (FWIW, after re-reading the spec a couple of times, to me is quite
> clear that the “MouseEvent.{ctrl,shift,alt,meta}Key” properties
> are *mandatory* — nothing in the spec hints at them being optional.)

Ah! other fields. This bug is only about buttons field. I don't know if the keyborad modifier fields are working in WPE, but it's definitely out of scope of this bug.
Comment 6 Carlos Garcia Campos 2018-08-03 02:22:56 PDT
Created attachment 346464 [details]
Patch
Comment 7 Zan Dobersek 2018-08-03 02:30:13 PDT
Comment on attachment 346464 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346464&action=review

> Tools/WebKitTestRunner/wpe/EventSenderProxyWPE.cpp:94
> +    case 1:
> +        modifier = wpe_input_pointer_modifier_button1;
> +        break;

Can these return the relevant modifier value, and the default clause just return 0? Or, if the compiler complains, have the default clause break and then return by default?
Comment 8 Carlos Garcia Campos 2018-08-03 02:38:31 PDT
Committed r234541: <https://trac.webkit.org/changeset/234541>
Comment 9 Claudio Saavedra 2018-09-18 01:58:27 PDT
(In reply to Carlos Garcia Campos from comment #5)
> (In reply to Adrian Perez from comment #4)
> > (FWIW, after re-reading the spec a couple of times, to me is quite
> > clear that the “MouseEvent.{ctrl,shift,alt,meta}Key” properties
> > are *mandatory* — nothing in the spec hints at them being optional.)
> 
> Ah! other fields. This bug is only about buttons field. I don't know if the
> keyborad modifier fields are working in WPE, but it's definitely out of
> scope of this bug.

Checking bug 189664, I see that the wkEventModifiers are not being propagated from WTR down to WK. I don't know what the original intention of the modifiers field in wpe_input_pointer_event was, but I suspect that it was keyboard modifiers, not pressed mouse buttons, as it's implemented in this bug's patch.

I suppose it's possible to serialize both pressed buttons and keyboard modifiers in the same modifiers field (it's 32 bits, after all), so it shouldn't be much of an issue.
Comment 10 Claudio Saavedra 2018-09-18 05:04:13 PDT
Implemented in bug 189697.