812351 - [metro] Insure pointer/keyboard input is delivered to windowless plugins

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Jim Mathies [:jimm]]

Since we are doing a lot of work on widget input currently, filing this to be sure we have the basics working for windowless plugins:

- precise/keyboard input events need to be delivered properly
- touch input should get translated to precise input initially

We want to emulate desktop event system as much as possible here. Windowless plugins receive standard win32 windowing events for this stuff, so we need to figure out how this is going to be connected up to the winrt widget backend.

We should also leverage existing / or write new tests for this using the test plugin, and get the test plugin enabled in metro mode.

Comment 1

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

I'll take a look at this for metro-it1

Comment 2

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

Looks like I managed to set myself as the QA contact instead of the assignee.  Fixing that.

Comment 3

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

Attachment: WIP Patch 1

This hasn't been tested yet, but the code is pretty straightforward to write after the work in bug 817061

Comment 4

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

Just a quick update: I tested this patch by enabling plugins in metro (plugin.disable = false) then going to a website that uses Flash.  I was able to interact with the Flash content using the mouse.  Typing characters didn't work, likely due to the fact that this patch sends WM_UNICHAR messages but doesn't send WM_CHAR messages.

Comment 5

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

Attachment: Patch v2 (WIP)

I "tested" this patch by going to Kongregate and playing a couple of the games.  First set "plugin.disable=false"

Mouse and keyboard input are both working.

Still TODO:
  Handle Unicode characters that don't fit into a single UTF-16 character
  Figure out why only left-mouse-button seems to be working (it's possible that we're correctly sending WM_RBUTTONDOWN but that flash can't do what it normally does)
  Misc cleanup

Comment 6

[Jim Mathies [:jimm]]

Do we have event simulation calls such that we can write tests that send touch input that gets translated and picked up by the test plugin?

See bug 806523 for an example mochitest-metro-chrome test that uses the test plugin.

Comment 7

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

(In reply to Jim Mathies [:jimm] from comment #6)
> Do we have event simulation calls such that we can write tests that send
> touch input that gets translated and picked up by the test plugin?

We cannot yet simulate touch input; only mouse and keyboard input.

> See bug 806523 for an example mochitest-metro-chrome test that uses the test
> plugin.

Thanks for the link!  This afternoon I'll be posting an updated patch for review and attempting to write some tests for this patch that utilize the test plugin

Comment 8

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

Attachment: Patch v3

I'm currently working on the tests for this change (and won't submit without them) but I think this much is ready for review.

A fun way to test this patch is to set "plugin.disable = false" and simply enjoy Youtube.

Comment 9

[Jim Mathies [:jimm]]

Comment on attachment 699502 [details] [diff] [review]
Patch v3

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

Looks great!

::: widget/windows/winrt/MetroInput.cpp
@@ +172,5 @@
> +  //   WM_KEYDOWN
> +  //         /en-us/library/windows/desktop/ms646280%28v=vs.85%29.aspx
> +  //
> +  //   WM_KEYUP
> +  //         /en-us/library/windows/desktop/ms646281%28v=vs.85%29.aspx

nit - these uris break over time, and everybody knows how to punch something into google so, lets ditch the msdn links in comments headers. :)

@@ +256,5 @@
> +  //        /en-us/library/windows/desktop/ms646242%28v=vs.85%29.aspx
> +  //      WM_MOUSEHWHEEL
> +  //        /en-us/library/windows/desktop/ms645614%28v=vs.85%29.aspx
> +  //      WM_MOUSEWHEEL
> +  //        /en-us/library/windows/desktop/ms645617(v=vs.85).aspx

nit - same here.

Comment 10

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

Attachment: Patch v4

Un-bitrotted patch.  This is different enough that I think I'd like you to take another look at it, jimm.

Comment 11

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

Attachment: Testing Helpers v1

This patch contains a bunch of helper functions that I needed for writing the plugin input tests.  It also removes helpers that aren't being used anywhere in our mochitests.

Comment 12

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

Attachment: Plugin Input tests v1

I'm not sure which of you is a more appropriate reviewer for the plugin input tests.

Comment 13

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #9)
> Comment on attachment 699502 [details] [diff] [review]
> Patch v3
> 
> Review of attachment 699502 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great!
> 
> ::: widget/windows/winrt/MetroInput.cpp
> @@ +172,5 @@
> > +  //   WM_KEYDOWN
> > +  //         /en-us/library/windows/desktop/ms646280%28v=vs.85%29.aspx
> > +  //
> > +  //   WM_KEYUP
> > +  //         /en-us/library/windows/desktop/ms646281%28v=vs.85%29.aspx
> 
> nit - these uris break over time, and everybody knows how to punch something
> into google so, lets ditch the msdn links in comments headers. :)
> 
> @@ +256,5 @@
> > +  //        /en-us/library/windows/desktop/ms646242%28v=vs.85%29.aspx
> > +  //      WM_MOUSEHWHEEL
> > +  //        /en-us/library/windows/desktop/ms645614%28v=vs.85%29.aspx
> > +  //      WM_MOUSEWHEEL
> > +  //        /en-us/library/windows/desktop/ms645617(v=vs.85).aspx
> 
> nit - same here.

did you miss addressing these nits?

Comment 14

[Matt Brubeck (:mbrubeck)]

Comment on attachment 709181 [details] [diff] [review]
Testing Helpers v1

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

Awesome work!

I'm starting to want to use some of these (like waitForEvent) in chrome also; we should think about putting the most useful ones into a JS module in toolkit.

::: browser/metro/base/tests/head.js
@@ +98,5 @@
> + *
> + * @param aMs the number of miliseconds to wait for
> + * @returns a Promise that resolves to true after the time has elapsed
> + */
> +function waitForMs(aMs) {

I notice there's an equivalent task function here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/tests/xpcshell/test_sqlite.js#18

Another reason to create a shared task library...

Comment 15

[Matt Brubeck (:mbrubeck)]

Comment on attachment 709182 [details] [diff] [review]
Plugin Input tests v1

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

Looks good to me.  Some minor style hints below, from the unwritten Fennec style guide which mfinkle has passed down to me as oral tradition.

::: browser/metro/base/tests/browser_plugin_input_keyboard.js
@@ +10,5 @@
> +  desc: "Plugin keyboard input",
> +  run: function() {
> +    let oldPrefPluginDisable = Services.prefs.getBoolPref("plugin.disable");
> +    let oldPrefPluginsClickToPlay =
> +                        Services.prefs.getBoolPref("plugins.click_to_play");

No need for wrapping here.  (We have no strict line-length limits in /browser/metro, though wrapping somewhere around 100-120 columns is appreciated.)

You can similarly un-wrap several other lines in this file.

@@ +16,5 @@
> +    Services.prefs.setBoolPref("plugins.click_to_play", false);
> +    registerCleanupFunction(
> +                  Services.prefs.setBoolPref.bind(null,
> +                                                  "plugin.disable",
> +                                                  oldPrefPluginDisable));

You can use Services.prefs.clearUserPref here, so you don't need to store the old value.

@@ +46,5 @@
> +                  keyCode: 65,
> +                  modifiers: rightAlt,
> +                  expectedChar: "á" }];
> +
> +    while(keys.length > 0) {

Nit: space after keywords like "while" or "if" (purely for consistency).

::: browser/metro/base/tests/browser_plugin_input_mouse.js
@@ +16,5 @@
> +    Services.prefs.setBoolPref("plugins.click_to_play", false);
> +    registerCleanupFunction(
> +                  Services.prefs.setBoolPref.bind(null,
> +                                                  "plugin.disable",
> +                                                  oldPrefPluginDisable));

See above.

@@ +63,5 @@
> +      synthesizeNativeMouseLDown(plugin, click.x, click.y);
> +      synthesizeNativeMouseLUp(plugin, click.x, click.y);
> +      let success = yield waitForCondition(testMouseUpCount.bind(null,
> +                                                                 plugin,
> +                                                                 curClicks));

I think this would be clearer to me if the condition function were just placed inline here.  Up to you if you want to make this change.

Comment 16

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

Attachment: Patch v5 (review comments addressed)

(In reply to Jim Mathies [:jimm] from comment #13)
> (In reply to Jim Mathies [:jimm] from comment #9)
> > Comment on attachment 699502 [details] [diff] [review]
> > Patch v3
> > 
> > Review of attachment 699502 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks great!
> > 
> > ::: widget/windows/winrt/MetroInput.cpp
> > @@ +172,5 @@
> > > +  //   WM_KEYDOWN
> > > +  //         /en-us/library/windows/desktop/ms646280%28v=vs.85%29.aspx
> > > +  //
> > > +  //   WM_KEYUP
> > > +  //         /en-us/library/windows/desktop/ms646281%28v=vs.85%29.aspx
> > 
> > nit - these uris break over time, and everybody knows how to punch something
> > into google so, lets ditch the msdn links in comments headers. :)
> > 
> > @@ +256,5 @@
> > > +  //        /en-us/library/windows/desktop/ms646242%28v=vs.85%29.aspx
> > > +  //      WM_MOUSEHWHEEL
> > > +  //        /en-us/library/windows/desktop/ms645614%28v=vs.85%29.aspx
> > > +  //      WM_MOUSEWHEEL
> > > +  //        /en-us/library/windows/desktop/ms645617(v=vs.85).aspx
> > 
> > nit - same here.
> 
> did you miss addressing these nits?

Oops!  I certainly did!  Attaching new patch

Comment 17

[Jim Mathies [:jimm]]

Comment on attachment 709182 [details] [diff] [review]
Plugin Input tests v1

no need for a review from me, mbrubeck is far more qualified to review front end work.

Comment 18

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

Attachment: Plugin Input tests v2 (review comments addressed)

Comment 19

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

Landed on elm:
  Test helpers - https://hg.mozilla.org/projects/elm/rev/f07961894b62
  Patch - https://hg.mozilla.org/projects/elm/rev/9a89257abb4e
  Tests - https://hg.mozilla.org/projects/elm/rev/bd3a90f08a72

First build with these patches:
  https://tbpl.mozilla.org/?tree=Elm&rev=bd3a90f08a72