Closed Bug 1290217 Opened 4 years ago Closed 4 years ago

Ensure input is removed after test before next one starts in browser_treeupdate_doc.js

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

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
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: nobody → mili
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 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)
Attachment #8775699 - Attachment is obsolete: true
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 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+
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/
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+
Keywords: checkin-needed
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);
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+
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+
Michael, also, since the try looks good I think we should enable the test in browser.ini?
Flags: needinfo?(mili)
Keywords: checkin-needed
Flags: needinfo?(mili)
Attachment #8777106 - Flags: review?(mili) → review+
Keywords: checkin-needed
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
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+
Attachment #8777106 - Attachment is obsolete: true
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/f0e0cd618dde
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.