795832 - Implement synthesized input events in metro widget

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Jim Mathies [:jimm]]

http://mxr.mozilla.org/mozilla-central/source/widget/nsIWidget.h#1300

We need these to build up a set of tests.

Comment 1

[Jim Mathies [:jimm]]

How goes this work Tim?

Comment 2

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch

This patch implements synthesized mouse, keyboard, and scroll events.  I've tested this patch by modifying browser.js to call `nsIDOMWindowUtils.sendNativeMouseEvent`, `nsIDOMWindowUtils.sendNativeKeyEvent`, and `nsIDOMWindowUtils.sendNativeMouseScrollEvent`.  Those functions in turn call the ones implemented in this patch.

Comment 3

[Jim Mathies [:jimm]]

Comment on attachment 677477 [details] [diff] [review]
Patch

Review of attachment 677477 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/winrt/MetroWidget.cpp
@@ +376,5 @@
> +{
> +  Log(L"ENTERED SynthesizeNativeKeyEvent\n");
> +
> +  // Source: http://msdn.microsoft.com/en-us/library/windows/desktop/ms646271%28v=vs.85%29.aspx
> +  NS_ENSURE_ARG_RANGE(aNativeKeyCode, 1, 254);

nit - add a comment explaining these constants rather than a link (or have both).

@@ +539,5 @@
> +  // inputs[len-1]: modifier key (e.g. shift, ctrl, etc) up
> +  uint32_t const len = keySequence.Length() * 2 + 1;
> +  INPUT* inputs = new INPUT[len];
> +  memset(inputs, 0, len * sizeof(INPUT));
> +  for (uint32_t i = 0; i < keySequence.Length(); ++i) {

You seem to be reusing this block of code in each call, can we split this out into a utility function along with it's comment block?

@@ +559,5 @@
> +  // processed by the time we leave this function, so we have to manually
> +  // pump them here.
> +  Log(L"  Waiting for input messages to clear\n");
> +  MSG msg;
> +  while (::PeekMessage(&msg, NULL, 0, 0, PM_REMOVE | PM_QS_INPUT)) {

ditto with this block.

I'm a little concerned about using this since we won't be going through the winrt dispatcher, but I think we can leave it be and address later if any problems crop up.

Comment 4

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: patch

(In reply to Jim Mathies [:jimm] from comment #3)
> Comment on attachment 677477 [details] [diff] [review]
> Patch
> 
> Review of attachment 677477 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/winrt/MetroWidget.cpp
> @@ +376,5 @@
> > +{
> > +  Log(L"ENTERED SynthesizeNativeKeyEvent\n");
> > +
> > +  // Source: http://msdn.microsoft.com/en-us/library/windows/desktop/ms646271%28v=vs.85%29.aspx
> > +  NS_ENSURE_ARG_RANGE(aNativeKeyCode, 1, 254);
> 
> nit - add a comment explaining these constants rather than a link (or have
> both).

Added a comment.

> @@ +539,5 @@
> > +  // inputs[len-1]: modifier key (e.g. shift, ctrl, etc) up
> > +  uint32_t const len = keySequence.Length() * 2 + 1;
> > +  INPUT* inputs = new INPUT[len];
> > +  memset(inputs, 0, len * sizeof(INPUT));
> > +  for (uint32_t i = 0; i < keySequence.Length(); ++i) {
> 
> You seem to be reusing this block of code in each call, can we split this
> out into a utility function along with it's comment block?

Pulled this out into a separate function.

> @@ +559,5 @@
> > +  // processed by the time we leave this function, so we have to manually
> > +  // pump them here.
> > +  Log(L"  Waiting for input messages to clear\n");
> > +  MSG msg;
> > +  while (::PeekMessage(&msg, NULL, 0, 0, PM_REMOVE | PM_QS_INPUT)) {
> 
> ditto with this block.

Pulled this out into the new function.
 
> I'm a little concerned about using this since we won't be going through the
> winrt dispatcher, but I think we can leave it be and address later if any
> problems crop up.

Agreed; let's see if any problems come up once we start using this functionality in our tests.

Comment 5

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch used during testing

This is the patch that I used during testing.  At various points I had it use additional synthesized input events (right-click to bring up the url bar, left click to click in the url bar, etc).

An issue I've discovered is that synthesizing "ctrl+f" and "ctrl+l" do not have their intended effects.  The keys are definitely being pressed, but for a reason I haven't yet been able to track down, synthesizing "ctrl+l" does not cause focus to be set in the URL bar, and synthesizing "ctrl+f" does not cause "find in page" to appear.

Everything else I've tested does work:
  Synthesizing right-clicks to make the context menu and tab bar appear
  Synthesizing left clicks in the URL bar to focus it
  Synthesizing key presses, including capital letters by passing `nsIWidget::SHIFT_L` and `nsIWidget::SHIFT_R` as flags
  Closing the browser by synthesizing "alt+f4"
  Scrolling a page with synthesized scroll events

Comment 6

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: patch

I updated the patch to remove the use of `::SetCursor` and instead move the mouse to the desired location with additional inputs to `::SendInput`.  Also updated some logging.

Comment 7

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

https://hg.mozilla.org/projects/elm/rev/0adee2dd527e

Comment 8

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

This was backed out due to bustage:
  https://hg.mozilla.org/projects/elm/rev/cf4c200f5ccf

And re-submitted with a bustage fix:
  https://hg.mozilla.org/projects/elm/rev/1b7a541acf76

Comment 9

[Jim Mathies [:jimm]]

Lets get the nsWindowDefs.h change over on mc. The top level widget changes have merged so this shows up in an mc-elm diff.

Comment 10

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: nsWindowDefs.h patch for m-c

I'm building this locally as a sanity check, and requesting review so that it lands with an r+ :)

Comment 11

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: nsWindowDefs.h patch for m-c

Updating patch - a misc other change crept into the previous version

Comment 12

[Jim Mathies [:jimm]]

Comment on attachment 680691 [details] [diff] [review]
nsWindowDefs.h patch for m-c

Looks ok to me.

Comment 13

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 680691 [details] [diff] [review]
nsWindowDefs.h patch for m-c

https://hg.mozilla.org/mozilla-central/rev/303a14307763

Comment 14

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Marking my [completed-elm] bugs as resolved