Bug 187998

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 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?

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

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

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

Created attachment 346464 [details]
Patch

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?

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

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

Implemented in bug 189697.