1422201 - NVDA takes on the dev tools document as the content of certain tabs

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[derek.riemer]

1. Run the NVDA screen reader.
2. open a new tab, type random text in the address bar, and perform a search.
3. Now, press f12. Now, press shift+tab to focus the document.
4. control+t time!
5. Type random text into the address bar again, and search for it.
6. Try to read the resulting document with NVDA.

Expected: NVDA reads the search engines document.
Actual: NVDA reads the dev tools document as if it were actually the page.

Comment 1

[James Teh [:Jamie]]

Confirmed. Once you focus the Dev Tools console output area, browse mode for all documents in the same content process reflects the content of the console output, not the content document.

This occurs because IAccessible::accChild for the Dev Tools output document answers for child ids in all content documents in that process. NVDA browse mode uses accChild to test whether an accessible is part of a document, so in this case, Firefox is essentially telling NVDA that everything belongs to the console output document.

I don't quite understand why this is happening yet. This Dev Tools document seems to be special somehow; it doesn't happen anywhere else.

Comment 2

[Brian Grinstead [:bgrins]]

Could the fact that the webconsole is an HTML document and not XUL be an issue? Does this happen on any other devtools like the Inspector or Style Editor?

Comment 3

[James Teh [:Jamie]]

The fact that it's an HTML document is certainly relevant (because NVDA browse mode gets used for it). NVDA browse mode being used for the document in question is a condition needed to trigger the behaviour.

What process does the console run in? Does it run in the parent process, the content process for the document it is inspecting or some other process?

Comment 4

[Marco Zehe (:MarcoZ)]

Could the fix for bug 1414451 be at fault here? An undesired side effect?

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to James Teh [:Jamie] from comment #3)
> What process does the console run in? Does it run in the parent process, the
> content process for the document it is inspecting or some other process?

The devtools UI runs in the parent process, loaded in an iframe in browser.xul (and sub iframes for each tool). So something like:

browser.xul:
-- toolbox.xul:
---- webconsole.html
---- inspector.xhtml
---- debugger.html
---- styleeditor.xul

Comment 6

[Marco Zehe (:MarcoZ)]

Jamie confirmed by backing out bug 1414451 from a local build that this is indeed the culprit. Setting flags accordingly. Note that 58 is affected because bug 1414451 was uplifted there to fix dev tools extensions accessibility and responsive design view.

Comment 7

[James Teh [:Jamie]]

The point about it being related to the same content process was a red herring. The confusion was because whether we expose this bug depends on the order of the documents in NVDA's browse mode documents set. In reality, accChild on all documents in the parent process responds to ids for all accessibles in remote processes.

Here's a reliable way to reproduce this with the NVDA python console:

1. Open mozilla.org
2. Open a new tab and load about:support
3. Focused on the about:support document, press NVDA+control+z.
4. Save off the about:support document by entering: doc = focus
5. Switch back to Firefox.
6. Switch to the mozilla.org tab.
7. Focused on the mozilla.org document, press NVDA+control+z.
8. Enter: nav in doc
Expected: False
Actual: True

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

CC: eeejay

Comment 9

[James Teh [:Jamie]]

The change made in bug 1414451 was that when getting remote accessibles, we compare the root of the accessible being called against the root of the remote document. Previously, we compared the *documents* of the two accessibles.

In terms of this bug, this means that previously, we compared the local document (since the document of a document is the document itself) against the root (since the document of an outer doc is the root). These are not equal and thus we skip the remote document. Now, we compare the root of the local document and the root of the outer doc, which are the same. Thus, we do query the remote document and end up getting an accessible.

While the previous behaviour was better for this bug, it was still somewhat broken. accChild on most chrome accessibles that weren't inside another document would return remote documents that weren't descendants. For example, if you called accChild on the Navigation toolbar and passed the id of a remote document, it would happily return an accessible, which is still wrong.

There are only two cases in which accChild is used with a negative id:
1. To retrieve a specific accessible from an event or a cached id (always called on the root); or
2. To test whether something is a descendant of a document (always called on the document and should fail for anything which isn't in that document).
For case 2, accChild is called on the remote document and it already won't answer for an id which isn't a descendant.
The simplest way to support case 1 without breaking case 2 is to only support querying remote ids on the root accessible.

Comment 10

[James Teh [:Jamie]]

Attachment: Bug 1422201: Only handle remote ids passed to IAccessible::accChild on the root accessible.

Previously, we could return remote accessibles which weren't actually descendants of the accessible on which accChild was called.
For example, calling accChild on a local document with the id of a remote document would happily return the remote accessible.
This confused clients such as NVDA which use accChild to check whether something is a descendant of a document.

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

Comment 11

[Marco Zehe (:MarcoZ)]

Comment on attachment 8934059 [details]
Bug 1422201: Only handle remote ids passed to IAccessible::accChild on the root accessible.

https://reviewboard.mozilla.org/r/205002/#review210448

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/81958c54d557
Only handle remote ids passed to IAccessible::accChild on the root accessible. r=MarcoZ

Comment 13

[James Teh [:Jamie]]

Testing performed:
1. Confirmed (expected) failure when calling accChild on a local document, passing a remote id. Previously, this succeeded when it shouldn't.
2. Confirmed (expected) failure when calling accChild on the Navigation toolbar , passing a remote id. Previously, this succeeded when it shouldn't.
3. Confirmed success when calling accChild on the root accessible, passing a remote id. No change.
4. Confirmed success when calling accChild on the root accessible, passing a local id. No change.
5. Confirmed success when calling accChild on the root accessible, passing an id within a document beneath the responsive design view. No change, but this was what bug 1414451 was fixing.

I'm seeing inconsistent results (both before and after the patch) regarding accChild with positive child ids. I need to investigate that further.

Comment 14

[James Teh [:Jamie]]

(In reply to James Teh [:Jamie] from comment #13)
> I'm seeing inconsistent results (both before and after the patch) regarding
> accChild with positive child ids. I need to investigate that further.

Filed bug 1422674.

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/81958c54d557

Comment 16

[Marco Zehe (:MarcoZ)]

Verified fixed in Nightly 59.0a1 20171204234137.

Comment 17

[Julien Cristau [:jcristau]]

Please request uplift to beta when you get a chance.

Comment 18

[James Teh [:Jamie]]

Comment on attachment 8934059 [details]
Bug 1422201: Only handle remote ids passed to IAccessible::accChild on the root accessible.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1414451.
[User impact if declined]: Screen reader users will be unable to access web content properly when a chrome document (web console, about:support, etc.) is open.
[Is this code covered by automated tests?]: No, because we don't have a mechanism for platform accessibility testing.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Trivial patch which only affects accessibility. Easy to explain, justify and verify the change in behaviour.
[String changes made/needed]: None.

Comment 19

[Gerry Chang [:gchang]]

Comment on attachment 8934059 [details]
Bug 1422201: Only handle remote ids passed to IAccessible::accChild on the root accessible.

Fix an accessibility regression issue. Beta58+.

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/143546f96bc4

Comment 21

[derek.riemer]

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=3176ff271bd7ee01b48faa09
d93590e902245975&tochange=81958c54d5571136c5cb5eb0df71bb5c9748a6fa

1. Try to open grease monkey, or lastpass from the bar of addons.
Expected: a browse mode document with NVDA.
Actual: an unknown.

Comment 22

[derek.riemer]

Output from mozregression:

23:56.09 INFO: First bad revision: 81958c54d5571136c5cb5eb0df71bb5c9748a6fa
23:56.09 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=3176ff271bd7ee01b48faa09
d93590e902245975&tochange=81958c54d5571136c5cb5eb0df71bb5c9748a6fa