Bug 187998 – [WPE] Implement MouseEvent.buttons

• Status: RESOLVED
• Resolution: FIXED
• Component: WPE WebKit

[Carlos Garcia Campos]

https://www.w3.org/TR/uievents/#ref-for-dom-mouseevent-buttons-1

This requires new API in WPEBackend.

[Carlos Garcia Campos]

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.

[Adrian Perez]

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?

[Carlos Garcia Campos]

(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.

[Adrian Perez]

(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.)

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Created attachment 346464 [details]
Patch

[Zan Dobersek]

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?

[Carlos Garcia Campos]

Committed r234541: <https://trac.webkit.org/changeset/234541>

[Claudio Saavedra]

(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.

[Claudio Saavedra]

Implemented in bug 189697.