Right now with e10s, if AT is started when there are pre-existing documents, it will not be notified about them because DOCUMENT_LOAD_COMPLETE is never fired for them. We need to make sure that that happens.

Are you thinking we need to fire a fake event?

Is this bug related to the WIPs in bug 1269369?

yes, adding that wip here will take a look at it now.

Getting doc accessible for all windows, not just top level ones.

just to be clear I'll get back to this when we've delt with crashes and other stability bugs.

ping

This is a use case of screen reader starting while Firefox is already running, not sure if it's high priority.

Jamie are you also seeing this issue happen in the case where the screen reader is launched before the browser?

No, not with a screen reader. However, this is a regression and was determined to be one cause of bug 1403180 (see bug 1403180 comment 9). I also very much suspect this is the reason for the failures on Windows 10 even after bug 1405065 and bug 1407475 were fixed; see bug 1407475 comment 9. I've verified locally that this patch fixes this problem. There's a test case attached to bug 1403180 comment 7 which helps with this, though you need to make sure you start Firefox without another a11y client running to test this. In summary, this doesn't affect screen readers, but there are reports that this is affecting other clients. At the very least, we should try to get this into Nightly sooner rather than later. I'd also suggest an uplift to 57 (because the less regressions we can have with e10s the better; we're regressing things enough as it is).

Was talking to Jamie and we're thinking we really need this in 57.

(In reply to James Teh [:Jamie] from comment #9) > No, not with a screen reader. However, this is a regression and was > determined to be one cause of bug 1403180 (see bug 1403180 comment 9). I > also very much suspect this is the reason for the failures on Windows 10 > even after bug 1405065 and bug 1407475 were fixed; see bug 1407475 comment 9. > > I've verified locally that this patch fixes this problem. There's a test > case attached to bug 1403180 comment 7 which helps with this, though you > need to make sure you start Firefox without another a11y client running to > test this. > > In summary, this doesn't affect screen readers, but there are reports that > this is affecting other clients. At the very least, we should try to get > this into Nightly sooner rather than later. I'd also suggest an uplift to 57 > (because the less regressions we can have with e10s the better; we're > regressing things enough as it is). just to confirm, Jamie, which patch have you tested? I guess the one in Comment 4?

(In reply to Yura Zenevich [:yzen] from comment #11) > just to confirm, Jamie, which patch have you tested? I guess the one in > Comment 4? Correct; the patch in comment 4. Note that the document load complete events are not the important part here; I'm not sure if they get fired. (I guess not looking at earlier comments?) The important thing is that the documents get created so they're available when a client walks the tree. It's worth noting that even with this patch, the documents are not immediately available after accessibility instantiation, I guess because they are created async. However, I think clients that don't use events can learn to live with this; a simple delay and/or retry should suffice. NI Alex for his thoughts on whether his client can deal with the async nature of this. Alex, in bug 1403180 comment 9, we discussed the child count being 0 for the initial tab. Can you deal with needing to delay and/or retry after making the first accessibility call if the child count is 0?

That's not a problem for me at all. Chrome needed delay/retry behaviour under certain circumstances when first initialising accessibility too, so I already have one code path that needs to deal with this sort of thing. I'm assuming that the behaviour would be that the child count is 0 until the child is available, after which it is 1 and at that point the child can immediately be accessed - i.e. at no point is the child count inaccurate, or throwing errors.

Fixed linting errors.

Sorry, arriving on the party late. What clients are broken by this behavior? The approach moves us further from a11y lazy instantiation ideas, and has perf impact for sure. I suppose that if a client attempts to grab a document, then a document accessible has to be created asynchronously. Is this async behavior confusing for those clients or document accessible doesn't get created for some reason?

(In reply to alexander :surkov from comment #16) > Sorry, arriving on the party late. What clients are broken by this behavior? See comment 9. I don't have specific names (Alex V might be able to help there), but comment 9 lists several references concerning people hitting this problem. > The approach moves us further from a11y lazy instantiation ideas I guess, but laziness is only good if it's relatively transparent. See below. > I suppose that if a client attempts to grab a > document, then a document accessible has to be created asynchronously. Is > this async behavior confusing for those clients or document accessible > doesn't get created for some reason? What happens is that a client instantiates accessibility by retrieving the root accessible. They then try to locate the document. At present, unless an event (e.g. focus) causes the document to be created, you cannot access the remote document accessibles at all; the "browser" accessible for the tab has 0 children (where it would normally have a single document child). The practical upshot is that the only way to get access to a remote document right now is for an event to be fired for it. This patch changes things such that when the client instantiates accessibility, they also get access to remote docs. There is an async part to this, but I think clients can deal with that; see comment 12, comment 13.

I assume those clients want to access to a content of the background tabs? Otherwise a tab document would had the focus, and thus a document accessible was created. If it's not a background content use case, then what is it? I skimmed over comment #9 and bug 1403180 in particular, but not sure I found the answers. I buy the argument that regressions should be fixed, and we probably should fix it now. But ideally I would avoid to create or update a11y trees in the background tabs at all, so one day when we get closer to these ideas, we may found ourself on same topic again. On implementation note: if async behavior is perfectly fine here, then wouldn't it make sense to instantiate a document accessible only if AT asked for children of its tab's browser accessible? That'd be cheaper than instantiating all of them unconditionally. Btw, ApplicationAccessible::Init() already traverses all windows via nsIWindowMediator and creates document accessibles for their documents, I'm curious why that approach doesn't longer work.

(In reply to alexander :surkov from comment #18) > I assume those clients want to access to a content of the background tabs? > Otherwise a tab document would had the focus, and thus a document accessible > was created. Not necessarily. Right now, this happens for the active tab if a document was loaded before accessibility was instantiated. So, it goes something like this: 1. User opens browser. 2. Browser loads a page. 3. Accessibility client makes a query. 4. Accessibility gets instantiated. At this point, you can't get access to the remote document until you move the focus in some way. Still, perhaps Alex V can provide more details concerning a real world use case. > On implementation note: if async behavior is perfectly fine here, then > wouldn't it make sense to instantiate a document accessible only if AT asked > for children of its tab's browser accessible? Well, async behaviour is far from ideal, but that's not a problem we can realistically solve. :) What you're suggesting makes sense, but it's tricky to get right. You'd have to catch quite a lot of calls. For example, if the client asks for the child count, you should probably return 0, but that in turn should start the async doc creation. Similarly, if you ask for the first child without checking the child count, that should probably fail, but it too must trigger the creation. Same for IEnumVARIANT, NAVRELATION_EMBEDS, etc.

> Still, perhaps Alex V can provide more details concerning a real world use > case. The bug that impacts my use case is 1403180, I only encountered this one while diagnosing the other. That said, working backwards, I can provide a real use case that would be affected. One of the user-initiated functions of my client produces a list of open browser tabs by URL. This will initially fail due to this issue, as the document is not accessible from the browser.

Updated to the same code path both for parent and child processes.

Comment on attachment 8922476 [details] [diff] [review] 1329977 patch v3 Review of attachment 8922476 [details] [diff] [review]: ----------------------------------------------------------------- the code looks nice, the logic is something to avoid :) let's get it as it's a regression, and let's file a follow up but to keep docs in a lazy way. ::: accessible/generic/ApplicationAccessible.cpp @@ +174,5 @@ > > + for (auto iter = windowsById->Iter(); !iter.Done(); iter.Next()) { > + nsGlobalWindow* window = iter.Data(); > + if (window->GetDocShell() && window->IsOuterWindow()) { > + if (window->IsRootOuterWindow()) { a whole bunch of ifs, I assume that Olli thumb up on these, but could you please put all of them under same if statement? @@ +177,5 @@ > + if (window->GetDocShell() && window->IsOuterWindow()) { > + if (window->IsRootOuterWindow()) { > + nsCOMPtr<nsIDocument> docNode = window->GetExtantDoc(); > + if (docNode) { > + GetAccService()->GetDocAccessible(docNode); // ensure creation Curious if there's no any problems with underlying iframes and such. If their documents creation is not triggered by a host document, and if if AT doesn't move the focus into iframes during doc traversal, then there's a problem. There's too many ifs there :) but it's something to consider for sure. ::: accessible/tests/browser/general/browser_test_doc_creation.js @@ +16,5 @@ > + return BrowserTestUtils.openNewForegroundTab( > + { gBrowser, url, forceNewProcess }); > +} > + > +add_task(async function testDocumentCreation() { it'd be nice to have a small comment what this test is about @@ +18,5 @@ > +} > + > +add_task(async function testDocumentCreation() { > + let tab1 = await openNewTab("doc_test_doc_creation_tab1.html"); > + let tab2 = await openNewTab("doc_test_doc_creation_tab2.html"); those files are pretty trivial, so they can be nicely replaced on data: protocol ::: accessible/tests/browser/general/head.js @@ +13,5 @@ > + this); > + > +async function initAccessibilityService() { > + info("Create accessibility service."); > + let initPromise = new Promise(resolve => { maybe |await new Promise()| to get rid of the variable @@ +41,5 @@ > + Services.obs.addObserver(observe, "a11y-init-or-shutdown"); > + }); > + > + forceGC(); > + await shutdownPromise; you don't have to keep this function async, just return a promise, it's same, but less lines

I filed bug 1412946 for a followup.

(In reply to alexander :surkov from comment #22) > Comment on attachment 8922476 [details] [diff] [review] > 1329977 patch v3 > > Review of attachment 8922476 [details] [diff] [review]: > ----------------------------------------------------------------- > @@ +177,5 @@ > > + if (window->GetDocShell() && window->IsOuterWindow()) { > > + if (window->IsRootOuterWindow()) { > > + nsCOMPtr<nsIDocument> docNode = window->GetExtantDoc(); > > + if (docNode) { > > + GetAccService()->GetDocAccessible(docNode); // ensure creation > > Curious if there's no any problems with underlying iframes and such. If > their documents creation is not triggered by a host document, and if if AT > doesn't move the focus into iframes during doc traversal, then there's a > problem. There's too many ifs there :) but it's something to consider for > sure. If there is an issue there, it is probably a separate bug. For the purposes here we only care about topmost documents (in parent and content).

Pushed by yura.zenevich@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0fbf251a6244 creating doc accessibles for existing content documents. r=surkov

I suggest we do not uplift this to 57 since there's not enough time in Beta left to be tested more thoroughly. The issue does not seem to affect screen reader users, so riding the 58 train seems appropriate. Jamie, let me know if you still think strongly about 57 uplift.

Given that 57 already unacceptably regresses accessibility performance, it seems sad to let another accessibility regression slip through when we have a patch. However, I agree there just isn't time left to test this properly, and I can't make a strong argument on behalf of another client. Also, there is a workaround: focus the document by some means before trying to access content.