1304290 - [Pointer Event] Add a helper function to run test in a new window

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Ming-Chou Shih [:stone]]

The content of some pointer events web platform tests may be larger than the visible area of mochitest test runner. It induces the synthesized input events can't be dispatched to the element correctly and cause those tests failed. Add a helper function to run tests in new window to solve it.

Comment 1

[Ming-Chou Shih [:stone]]

Attachment: [Pointer Event] Add a helper function to run test in a new window (V1)

Comment 2

[Olli Pettay [:smaug]]

Is this similar issues for which sendMouseEventToWindow was added?
But yeah, running in a new window should be fine too.

Comment 3

[Ming-Chou Shih [:stone]]

Yes. it's similar issues. Good to know that sendMouseEventToWindow can dispatch to specified window. Considering about we may change the synthesized input events to native events in future to increase test coverage, I would like to run tests in a new window.

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8793206 [details] [diff] [review]
[Pointer Event] Add a helper function to run test in a new window (V1)

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

There are more test cases running in a iframe,
Will all these test cases be applied to this new rule?

::: dom/events/test/pointerevents/mochitest_support_external.js
@@ +81,5 @@
>      is(!!elem, true, "Document should have element with id: " + elemId);
>    }
>  }
> +
> +function runTestInFreshWindow(aFile) {

Let's rename to runTestInNewWindow(aTest)

Comment 5

[Ming-Chou Shih [:stone]]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #4)
> Comment on attachment 8793206 [details] [diff] [review]
> There are more test cases running in a iframe,
> Will all these test cases be applied to this new rule?
I'm ok with that.

Hi Ben, are you ok if I apply it to all pointer events test cases in dom/events/test/pointerevents?

Comment 6

[Ben Hsu [:HoPang]]

Sure, since I am also planning to align the test automation of Pointer Events and touch-actions, I am happy to see it happen. By the way, is the summary the reason that Kats wrapping testcases for touch-actions into a new window?

Comment 7

[Ben Hsu [:HoPang]]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #4)
> > +function runTestInFreshWindow(aFile) {
> 
> Let's rename to runTestInNewWindow(aTest)

Well, though it's not a big deal, IMHO, keeping the "fresh" here to match the naming used in APZ test automation[0] is a good idea :P

[0] http://searchfox.org/mozilla-central/source/gfx/layers/apz/test/mochitest/apz_test_utils.js#184

Comment 8

[Ming-Chou Shih [:stone]]

Attachment: Part1: Add a helper function to run test in a new window (V2)

Add helper function to run test in new window and apply it to existed pointer events mochitests.

Comment 9

[Ming-Chou Shih [:stone]]

Attachment: Part2: Add protection and check before accessing some local variables which may be destroyed

Comment 10

[Ming-Chou Shih [:stone]]

(In reply to Ben Hsu [:HoPang] from comment #6)
> Sure, since I am also planning to align the test automation of Pointer
> Events and touch-actions, I am happy to see it happen. By the way, is the
> summary the reason that Kats wrapping testcases for touch-actions into a new
> window?
Sorry for the late reply. Actually I have no idea if it's the reason why wrapping touch-actions testcases into new window.

Comment 11

[Ming-Chou Shih [:stone]]

Comment on attachment 8797331 [details] [diff] [review]
Part2: Add protection and check before accessing some local variables which may be destroyed

Cancel f? because there are some javascript errors in the log. Would like to fix them first.

Comment 12

[Ming-Chou Shih [:stone]]

Attachment: Bug 1304290 Part1: Add a helper function to run test in a new window (V3)

Add helper function to run test in a new window. Finish test when the test case is finished and all synthesized events dispatched.

Comment 13

[Ming-Chou Shih [:stone]]

Attachment: Bug 1304290 Part2: Keep nsPresShell alive to prevent access invalid address of shell->mPointerEventTarget (V3)

When running test cases in new windows, I found shell may be destroyed after shell->HandlePositionedEvent. It induces invalid access to shell->mPointerEventTarget. We need to keep shell alive until we access to shell->mPointerEventTarget

Comment 14

[Ming-Chou Shih [:stone]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dda4adaba967d8441a31010b15f36fdf0d362f9c

Comment 15

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Refactor Pointer Event Test Harness.

A sample code to simplify the helper provided in patch part1.

Comment 16

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8798281 [details] [diff] [review]
Bug 1304290 Part1: Add a helper function to run test in a new window (V3)

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

I believe this can be simplified by the WIP in comment 15.

Comment 17

[Ming-Chou Shih [:stone]]

Attachment: Bug 1304290 Part1: Add a helper function to run test in a new window (V4)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f738d18f219da0de3c3705cf1348192e8eeaf58d

Comment 18

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8799624 [details] [diff] [review]
Bug 1304290 Part1: Add a helper function to run test in a new window (V4)

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

f=m after refining as inlined.

::: dom/events/test/pointerevents/mochitest_support_external.js
@@ +87,5 @@
> +        // We need to wait tests done and execute finished then we can close the window
> +        w.testContext.executionPromise.then(() => {
> +          w.close();
> +          SimpleTest.finish();
> +        });        

remove trailed spaces.

@@ +90,5 @@
> +          SimpleTest.finish();
> +        });        
> +      } else {
> +        // execute may synchronous trigger tests done. In that case executionPromise
> +        // is not yet assigned 

trailed space.

@@ +93,5 @@
> +        // execute may synchronous trigger tests done. In that case executionPromise
> +        // is not yet assigned 
> +        w.close();
> +        SimpleTest.finish();
> +      }

We can refine this as followed:
      let finishTest = () => {
        w.close();
        SimpleTest.finish();
      }

      if (!!w.testContext.executionPromise) {
        // Test was done asynchronously after executeTest().
        w.testContext.executionPromise.then(finishTest);
      } else {
        // Test was done synchronously after executeTest().
        finishTest();
      }

Comment 19

[Ming-Chou Shih [:stone]]

Attachment: Bug 1304290 Part1: Add a helper function to run test in a new window. f=bevistseng. r=smaug

Comment 20

[Ming-Chou Shih [:stone]]

Attachment: Bug 1304290 Part2: Keep nsPresShell alive to prevent access invalid address of shell->mPointerEventTarget. f=bevistseng. r=smaug

Updated the patch summary

Comment 21

[Ming-Chou Shih [:stone]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f738d18f219da0de3c3705cf1348192e8eeaf58d
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f3601d96d2cea27d51512abf478f33ee22218f4

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9ab8fa47b68
Part 1: Add a helper function to run test in a new window. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e44d66f5e61
Part 2: Keep nsPresShell alive to prevent access invalid address of shell->mPointerEventTarget. r=smaug

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b9ab8fa47b68
https://hg.mozilla.org/mozilla-central/rev/0e44d66f5e61