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

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Michael Li]

MozReview-Commit-ID: 6mwFAK7CH8u

Comment 1

[Michael Li]

Attachment: Wait for text node to load when inserting body to iframe document before testing accessible tree in browser_treeupdate_doc.js

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.

Comment 2

[Michael Li]

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

[Yura Zenevich [:yzen]]

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?

Comment 4

[Michael Li]

Attachment: Bug 1290217: Ensure input is removed after test before next one starts in browser_treeupdate_doc.js

Review commit: https://reviewboard.mozilla.org/r/68580/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68580/

Comment 5

[Michael Li]

Try server results look good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=81e29df80682

Comment 6

[Yura Zenevich [:yzen]]

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.

Comment 7

[Michael Li]

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 8

[Michael Li]

https://reviewboard.mozilla.org/r/68580/#review65686

Comment 9

[Michael Li]

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

Comment 10

[Yura Zenevich [:yzen]]

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 11

[Michael Li]

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/

Comment 12

[Michael Li]

https://reviewboard.mozilla.org/r/68578/#review65708

Comment 13

[Michael Li]

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

Comment 14

[Yura Zenevich [:yzen]]

Michael, also, since the try looks good I think we should enable the test in browser.ini?

Comment 15

[Michael Li]

Attachment: Enable browser_treeupdate_doc.js now that tests work

Review commit: https://reviewboard.mozilla.org/r/68712/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68712/

Comment 16

[Yura Zenevich [:yzen]]

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

Comment 17

[Michael Li]

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/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f0e0cd618dde