1954448 - Accessibility tests use inline scripts in a browser context

Status: RESOLVED FIXED

(Core :: Disability Access APIs, task)

Description

[Tom Schuster]

Unfortunately in bug 1937540 I changed this test to use inline scripts instead of inline event handlers, but now we want to block both.

I have an almost working patch for this, however we don't get the correct "click" event for the very first test run, when clicking on the first element li_clickable1.

 0:07.52 PASS Received document load complete event - 
 0:07.52 GECKO(62186) must wait for focus
 0:07.53 GECKO(62186) must wait for focus in content
 0:07.63 PASS Actually remote browser - 
 0:07.67 PASS Doc should not have any actions - 
 0:07.67 GECKO(62186) console.log: "READY STATE" "complete"
 0:07.68 INFO Testing element li_clickable1
 0:07.68 FAIL Correct action count - Got +0, expected 1
Stack trace:
chrome://mochikit/content/browser-test.js:test_is:1654
chrome://mochitests/content/browser/accessible/tests/browser/e10s/browser_caching_actions.js:testActions:29
chrome://mochitests/content/browser/accessible/tests/browser/e10s/browser_caching_actions.js:_testActions:140
chrome://mochitests/content/browser/accessible/tests/browser/e10s/browser_caching_actions.js:null:143
chrome://mochitests/content/browser/accessible/tests/browser/shared-head.js:accessibleTask/wrapped/<:603
resource://testing-common/BrowserTestUtils.sys.mjs:withNewTab:121
chrome://mochitests/content/browser/accessible/tests/browser/shared-head.js:wrapped:521
chrome://mochikit/content/browser-test.js:handleTask:1170
chrome://mochikit/content/browser-test.js:_runTaskBasedTest:1242
chrome://mochikit/content/browser-test.js:Tester_execTest:1383
chrome://mochikit/content/browser-test.js:nextTest/<:1159
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/<:1058
 0:07.68 FAIL Correct action names - Got "", expected "click"

Comment 1

[Tom Schuster]

Attachment: WIP: Bug 1954448 - Remove inline script from browser_caching_actions.js test

Comment 2

[Tom Schuster]

Attachment: Bug 1954448 - Remove inline script from browser_aria_activedescendant.js test. r?Jamie

Comment 3

[Tom Schuster]

Attachment: Bug 1954448 - Remove inline script from browser_caching_relations_002.js test. r?Jamie

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b0063b4e98f4
Remove inline script from browser_aria_activedescendant.js test. r=Jamie

Comment 5

[Cosmin Sabou [:CosminS]]

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

Comment 6

[Tom Schuster]

For https://searchfox.org/mozilla-central/source/accessible/tests/browser/text/browser_highlights.js#175 would it be okay to use a content task + waitForTextAttrRanges, or does that also invalidate the testing of the initial range?

Comment 7

[James Teh [:Jamie]]

This would change the test so that it's testing mutations, not initial state. In theory, those should have the same result, but they need to be tested separately because obscure bugs can mean that initial state vs mutations behave differently.

It seems to me that we're going to need a generic solution for this problem, as I suspect we're going to keep hitting it. I can think of ways to solve this in individual tests, but it's going to get very messy.

Is there some internal pref (or other magic) we can set for our test harness to allow inline script or script from a data: URI? Failing that, how can I test your proposed change in bug 1953866 so I can try to come up with a solution for our harness?

Comment 8

[Tom Schuster]

(In reply to James Teh [:Jamie] from comment #7)

This would change the test so that it's testing mutations, not initial state. In theory, those should have the same result, but they need to be tested separately because obscure bugs can mean that initial state vs mutations behave differently.

Thanks, I had kind of expected something like this.

It seems to me that we're going to need a generic solution for this problem, as I suspect we're going to keep hitting it. I can think of ways to solve this in individual tests, but it's going to get very messy.

Could we move the inline scripts to a standalone JS file and include them via <script src="chrome://mochitests/xxxx"> instead? That approach should keep working basically indefinitely.

Is there some internal pref (or other magic) we can set for our test harness to allow inline script or script from a data: URI? Failing that, how can I test your proposed change in bug 1953866 so I can try to come up with a solution for our harness?

Introducing a pref for this would be a bit messy, because the CSP is set before the test is even running. And I would like to avoid these kind of opt-out prefs if possible, because we have seen in other instances like security.allow_eval_in_parent_process that this just means delaying the work of doing it properly.
I just attached a patch to bug 1953866 that you can use for testing. The tl;dr is: changing the browser.xhtml CSP to script-src chrome: resource:.

Comment 9

[James Teh [:Jamie]]

(In reply to Tom Schuster from comment #8)

Could we move the inline scripts to a standalone JS file and include them via <script src="chrome://mochitests/xxxx"> instead? That approach should keep working basically indefinitely.

There are two challenges there:

1. We'd need a separate tiny .js file for each test where we need an init script.
2. We need to reference the script with a different URI schema for each variant (chrome:// vs https:// for content process tests). I guess the harness could search and replace some special string in the snippet, though.

Introducing a pref for this would be a bit messy, because the CSP is set before the test is even running. And I would like to avoid these kind of opt-out prefs if possible, because we have seen in other instances like security.allow_eval_in_parent_process that this just means delaying the work of doing it properly.

Fair enough. There's tension here between doing the right thing for security and making tests easy to write and maintain. Part of the reason we came up with the approach we have now is that we previously had entirely separate test suites for chrome context and content process tests. We regularly ran into cases where something was tested well in one context, but not the other. Ideally, in most cases, the accessibility engine should behave exactly the same in both contexts, so we figured having common tests that could run in both contexts without modification was the best way to ensure good coverage (and best use of our limited team resources). Anyway, this is just some background on why we have it set up this way. I'll see if I can come up with some way to deal with this.

Comment 10

[Tom Schuster]

2. We need to reference the script with a different URI schema for each variant (chrome:// vs https:// for content process tests). I guess the harness could search and replace some special string in the snippet, though.

I think we actually have content-accessible chrome: (or maybe it's resource:) URLs, mochitest files might even be accessible by default. I am not sure, but maybe this is something we could use the new moz-src: for as well.

Comment 11

[James Teh [:Jamie]]

What is moz-src:? Can you explain or point me somewhere? Thanks.

Comment 12

[James Teh [:Jamie]]

Attachment: Bug 1954448: Add a contentSetup argument to addAccessibleTask to facilitate content setup before a test starts.

After bug 1953866, it will no longer be possible to have inline <script> elements in any document which runs in a chrome context.
Several accessibility tests currently rely on this to set things up which can't be done in pure HTML/CSS, but which need to be done before the content is initially added to the accessibility tree.
To work around this, a content task can be specified using the contentSetup argument to addAccessibleTask.
This causes the following to happen:

1. aria-hidden="true" is set on the body, preventing the initial accessibility tree update from including any content.
2. contentTask is executed in the context of the document.
3. aria-hidden is removed from the body, so that all of the content (including any changes made by contentSetup) is processed by accessibility.
4. A reorder event is awaited on the body, so that we can guarantee that (3) is complete.

Subsequent patches will adjust existing tests as needed.

Comment 13

[James Teh [:Jamie]]

Attachment: Bug 1954448: Adjust several accessibility tests which specify chrome: true to use contentSetup instead of inline script.

Comment 14

[Tom Schuster]

Thank you for completing this work!

(In reply to James Teh [:Jamie] from comment #11)

What is moz-src:? Can you explain or point me somewhere? Thanks.

Bug 1950723 has link.

Comment 15

[Tom Schuster]

Attachment: Bug 1954448: Adjust several accessibility tests which specify chrome: true to use contentSetup instead of inline script (Part 2). r?Jamie

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1acfd3210860
Add a contentSetup argument to addAccessibleTask to facilitate content setup before a test starts. r=tschuster,eeejay
https://hg.mozilla.org/integration/autoland/rev/093397f55597
Adjust several accessibility tests which specify chrome: true to use contentSetup instead of inline script. r=tschuster,eeejay

Comment 17

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/1acfd3210860
https://hg.mozilla.org/mozilla-central/rev/093397f55597

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/75ceb46f6227
Adjust several accessibility tests which specify chrome: true to use contentSetup instead of inline script (Part 2). r=Jamie

Comment 19

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/75ceb46f6227