Closed
Bug 1304290
Opened 6 years ago
Closed 6 years ago
[Pointer Event] Add a helper function to run test in a new window
Categories
(Core :: DOM: Events, defect)
Core
DOM: Events
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 | ||
Updated•6 years ago
|
Assignee: nobody → sshih
Assignee | ||
Comment 1•6 years ago
|
||
Attachment #8793206 -
Flags: feedback?(btseng)
Comment 2•6 years ago
|
||
Is this similar issues for which sendMouseEventToWindow was added? But yeah, running in a new window should be fine too.
Assignee | ||
Comment 3•6 years ago
|
||
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•6 years ago
|
||
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)
Assignee | ||
Comment 5•6 years ago
|
||
(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)
Comment 6•6 years ago
|
||
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)
Comment 7•6 years ago
|
||
(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
Assignee | ||
Comment 8•6 years ago
|
||
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)
Assignee | ||
Comment 9•6 years ago
|
||
Attachment #8797331 -
Flags: feedback?(btseng)
Assignee | ||
Comment 10•6 years ago
|
||
(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.
Assignee | ||
Updated•6 years ago
|
Attachment #8797330 -
Flags: feedback?(btseng)
Assignee | ||
Comment 11•6 years ago
|
||
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)
Assignee | ||
Comment 12•6 years ago
|
||
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
Assignee | ||
Comment 13•6 years ago
|
||
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
Assignee | ||
Comment 14•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dda4adaba967d8441a31010b15f36fdf0d362f9c
Assignee | ||
Updated•6 years ago
|
Attachment #8798281 -
Flags: feedback?(btseng)
Assignee | ||
Updated•6 years ago
|
Attachment #8798286 -
Flags: feedback?(btseng)
Updated•6 years ago
|
Attachment #8798286 -
Flags: feedback?(btseng) → feedback+
Comment 15•6 years ago
|
||
A sample code to simplify the helper provided in patch part1.
Comment 16•6 years ago
|
||
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)
Assignee | ||
Comment 17•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f738d18f219da0de3c3705cf1348192e8eeaf58d
Attachment #8798281 -
Attachment is obsolete: true
Attachment #8799624 -
Flags: feedback?(btseng)
Comment 18•6 years ago
|
||
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+
Assignee | ||
Updated•6 years ago
|
Attachment #8799624 -
Flags: review?(bugs)
Assignee | ||
Updated•6 years ago
|
Attachment #8798286 -
Flags: review?(bugs)
Updated•6 years ago
|
Attachment #8798286 -
Flags: review?(bugs) → review+
Updated•6 years ago
|
Attachment #8799624 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 19•6 years ago
|
||
Attachment #8799624 -
Attachment is obsolete: true
Attachment #8800583 -
Flags: review+
Attachment #8800583 -
Flags: feedback+
Assignee | ||
Comment 20•6 years ago
|
||
Updated the patch summary
Attachment #8798286 -
Attachment is obsolete: true
Attachment #8800584 -
Flags: review+
Attachment #8800584 -
Flags: feedback+
Assignee | ||
Comment 21•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f738d18f219da0de3c3705cf1348192e8eeaf58d https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f3601d96d2cea27d51512abf478f33ee22218f4
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 22•6 years ago
|
||
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
Comment 23•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b9ab8fa47b68 https://hg.mozilla.org/mozilla-central/rev/0e44d66f5e61
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•