Closed
Bug 1290217
Opened 9 years ago
Closed 9 years ago
Ensure input is removed after test before next one starts in browser_treeupdate_doc.js
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: michael.li11702, Assigned: michael.li11702)
Details
Attachments
(1 file, 2 obsolete files)
MozReview-Commit-ID: 6mwFAK7CH8u
Assignee | ||
Comment 1•9 years ago
|
||
Some notes: We're adding an id to a text node, which works for text-insert and reorder events but not show, so we wait for a text-insert and reorder as indication that the text node's loaded and we're ready to test.
The tests are working properly, but for some reason the logs for all the text-insert and reorder events are coming after the INFO TEST-PASS logs.
Attachment #8775699 -
Flags: review?(yzenevich)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → mili
Assignee | ||
Comment 2•9 years ago
|
||
Also, using waitForMultipleEvents in place of Promise.all didn't work, which also seems strange. Maybe it is due to the same event firing successively multiple times?
Comment 3•9 years ago
|
||
Comment on attachment 8775699 [details] [diff] [review]
Wait for text node to load when inserting body to iframe document before testing accessible tree in browser_treeupdate_doc.js
Alright. I looked into the issue and I think the failure is a side effect of a previous test. If you notice there, we are removing the input (only child) of the iframe's document element. That is the clean up step. I think there's actually a bug there and the input is not being removed. Also it is a bug that we do not wait until the reorder on the iframe after we perform that removal before proceeding to our test in question. So what I would suggest is that we make sure that we remove everything from the document element in the previous test by both: waiting for reorder on the iframe and then testing the accessibility tree that it matches something like this:
{
role: ROLE_DOCUMENT,
children: [ ]
}
What do you think?
Attachment #8775699 -
Flags: review?(yzenevich)
Assignee | ||
Comment 4•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/68580/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68580/
Attachment #8776966 -
Flags: review?(yzenevich)
Assignee | ||
Updated•9 years ago
|
Attachment #8775699 -
Attachment is obsolete: true
Assignee | ||
Comment 5•9 years ago
|
||
Try server results look good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=81e29df80682
Assignee | ||
Updated•9 years ago
|
Summary: Wait for text node to load when inserting body to iframe document before testing accessible tree in browser_treeupdate_doc.js → Ensure input is removed after test before next one starts in browser_treeupdate_doc.js
Comment 6•9 years ago
|
||
Comment on attachment 8776966 [details]
Bug 1290217: Ensure input is removed after test before next one starts in browser_treeupdate_doc.js
https://reviewboard.mozilla.org/r/68580/#review65678
Looks good with the comment addressed, thanks!
::: accessible/tests/browser/browser_treeupdate_doc.js:239
(Diff revision 1)
> let docNode = content.document.getElementById('iframe').contentDocument;
> // Remove aftermath of this test before next test starts.
> - docNode.documentElement.removeChild(content.window.inputNode);
> + docNode.documentElement.removeChild(docNode.documentElement.firstChild);
> });
> + // Make sure reorder event was fired and that the input was removed.
> + reorderEventPromise = waitForEvent(EVENT_REORDER, iframe);
This has to go before the ContentTask above, since we want to initialize the promise before the DOM manipulation (see similar pattern everywhere else).
Here you just need to have the yield part.
Attachment #8776966 -
Flags: review?(yzenevich) → review+
Assignee | ||
Comment 7•9 years ago
|
||
Comment on attachment 8776966 [details]
Bug 1290217: Ensure input is removed after test before next one starts in browser_treeupdate_doc.js
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68580/diff/1-2/
Assignee | ||
Comment 8•9 years ago
|
||
Assignee | ||
Comment 9•9 years ago
|
||
Comment on attachment 8776966 [details]
Bug 1290217: Ensure input is removed after test before next one starts in browser_treeupdate_doc.js
https://reviewboard.mozilla.org/r/68580/#review65688
Attachment #8776966 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 10•9 years ago
|
||
https://reviewboard.mozilla.org/r/68580/#review65698
::: accessible/tests/browser/browser_treeupdate_doc.js:237
(Diff revision 2)
>
> + reorderEventPromise = waitForEvent(EVENT_REORDER, iframe);
> yield ContentTask.spawn(browser, {}, () => {
> let docNode = content.document.getElementById('iframe').contentDocument;
> // Remove aftermath of this test before next test starts.
> - docNode.documentElement.removeChild(content.window.inputNode);
> + docNode.documentElement.removeChild(docNode.documentElement.firstChild);
ah sorry forgot to add one more things:
could you do something like this here:
let docEl = content.document.getElementById('iframe').contentDocument.documentElement;
docEl.removeChild(dodEl.firstChild);
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8776966 [details]
Bug 1290217: Ensure input is removed after test before next one starts in browser_treeupdate_doc.js
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68580/diff/2-3/
Attachment #8776966 -
Flags: review+
Assignee | ||
Comment 12•9 years ago
|
||
Assignee | ||
Comment 13•9 years ago
|
||
Comment on attachment 8776966 [details]
Bug 1290217: Ensure input is removed after test before next one starts in browser_treeupdate_doc.js
https://reviewboard.mozilla.org/r/68580/#review65712
Attachment #8776966 -
Flags: review+
Comment 14•9 years ago
|
||
Michael, also, since the try looks good I think we should enable the test in browser.ini?
Flags: needinfo?(mili)
Keywords: checkin-needed
Assignee | ||
Comment 15•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/68712/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68712/
Attachment #8777106 -
Flags: review?(mili)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(mili)
Attachment #8777106 -
Flags: review?(mili) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 16•9 years ago
|
||
Hi Michael, would you mind squashing both patches into 1, making sure that commit message is prefixed with:
Bug 1290217 - ... and ends with r=yzen?
Thanks
Keywords: checkin-needed
Assignee | ||
Comment 17•9 years ago
|
||
Comment on attachment 8776966 [details]
Bug 1290217: Ensure input is removed after test before next one starts in browser_treeupdate_doc.js
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68580/diff/3-4/
Attachment #8776966 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8777106 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 18•9 years ago
|
||
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f0e0cd618dde
Ensure input is removed after test before next one starts in browser_treeupdate_doc.js r=yzen
Keywords: checkin-needed
Comment 19•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•