Closed Bug 1304290 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: DOM: Events, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: stone, Assigned: stone)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 6 obsolete files)

4.55 KB, patch
Details | Diff | Splinter Review
90.14 KB, patch
stone
: review+
stone
: feedback+
Details | Diff | Splinter Review
1.79 KB, patch
stone
: review+
stone
: feedback+
Details | Diff | Splinter Review
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.
Assignee: nobody → sshih
Is this similar issues for which sendMouseEventToWindow was added?
But yeah, running in a new window should be fine too.
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.
Blocks: 1296492
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)
Attachment #8793206 - Flags: feedback?(btseng)
(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?
Flags: needinfo?(bhsu)
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?
Flags: needinfo?(bhsu)
(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
Add helper function to run test in new window and apply it to existed pointer events mochitests.
Attachment #8793206 - Attachment is obsolete: true
Attachment #8797330 - Flags: feedback?(btseng)
(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.
Attachment #8797330 - Flags: feedback?(btseng)
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.
Attachment #8797331 - Flags: feedback?(btseng)
Add helper function to run test in a new window. Finish test when the test case is finished and all synthesized events dispatched.
Attachment #8797330 - Attachment is obsolete: true
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
Attachment #8797331 - Attachment is obsolete: true
Attachment #8798281 - Flags: feedback?(btseng)
Attachment #8798286 - Flags: feedback?(btseng)
Attachment #8798286 - Flags: feedback?(btseng) → feedback+
A sample code to simplify the helper provided in patch part1.
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.
Attachment #8798281 - Flags: feedback?(btseng)
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();
      }
Attachment #8799624 - Flags: feedback?(btseng) → feedback+
Attachment #8799624 - Flags: review?(bugs)
Attachment #8798286 - Flags: review?(bugs)
Attachment #8798286 - Flags: review?(bugs) → review+
Attachment #8799624 - Flags: review?(bugs) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b9ab8fa47b68
https://hg.mozilla.org/mozilla-central/rev/0e44d66f5e61
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.