When a document is loaded then each its accessbile gets a show event plus text change events, plus coalescencing and events delivering over ipc and AT. The behavior was introduced in bug 982842 for ipc implementation. It's non optimal and slow to deliver accessible content to the main process, and it can be avoided if a document itself was serialized instead. I believe this is another part of the puzzle of bug 1321960.

DocAccessibleChildBase::SerializeTree() asserts that we cannot serizalize documents [1], I wonder if there's a good reason of that. Otherwise it seems we could send a document itself. [1] https://hg.mozilla.org/mozilla-central/annotate/7e962631ba4298bcefa571008661983d77c3e652/accessible/ipc/DocAccessibleChildBase.cpp#l67

I'm curious whether Send/RecvShowEvent is expensive thing and preferably should be avoided, and whether it's cheaper to call it once with N accessbiles serialized rather than to call it N times for each accessible serialized.

Here's a try build that makes us not to fire show/hide events for a content of just loaded document. Marco, would you mind to check this one if it makes any difference for testcase of bug 1321960?

I don't see any positive impact. On the contrary, it appears that buffers from the previously open tab seem to stick around NVDA longer, giving the impression that one is still in the previous tab when actually the new one is already loading. So no positive perf impact, but a perceived regression in that the previous buffer doesn't get replaced so fast.

(In reply to Marco Zehe (:MarcoZ) from comment #5) > I don't see any positive impact. On the contrary, it appears that buffers > from the previously open tab seem to stick around NVDA longer, giving the > impression that one is still in the previous tab when actually the new one > is already loading. So no positive perf impact, but a perceived regression > in that the previous buffer doesn't get replaced so fast. That's very interesting. We're already seeing that problem a lot with JAWS.

I realized that show events are still fired through ipc layer, this version fixes it. If delivering the large number of events is an issue, even if there's no consumer on the other end, then it might help. Marco, curious, if it makes anything different for you? https://tools.taskcluster.net/index/artifacts/gecko.v2.try.revision.9e6c317fecb102d6c9d868448273aa802e025a00.firefox

This new try build is a huge magnitude faster. Like NVDA can consume the whole document from the test case in bug 1321960 comment #0 at once, and the browser becomes responsive again immediately. Good work!

Giggity giggity giggity goo!

Comment on attachment 8913730 [details] [diff] [review] patch Review of attachment 8913730 [details] [diff] [review]: ----------------------------------------------------------------- Cool! Just one nit on indentation. r=me with that fixed. ::: accessible/ipc/win/DocAccessibleChild.h @@ +111,5 @@ > SerializedShow(DocAccessibleChild* aTarget, > ShowEventData& aEventData, bool aFromUser) > : DeferredEvent(aTarget) > + , mEventData(aEventData.ID(), aEventData.Idx() > + , nsTArray<AccessibleData>(), aEventData.EventSuppressed()) This indentation confused me because it makes the second row of parameters look like additional entries in the initialization list. Can you please fix this so that it is more clear that these are additional parameters to mEventData?

(In reply to Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman) from comment #11) > > : DeferredEvent(aTarget) > > + , mEventData(aEventData.ID(), aEventData.Idx() > > + , nsTArray<AccessibleData>(), aEventData.EventSuppressed()) > > This indentation confused me because it makes the second row of parameters > look like additional entries in the initialization list. Can you please fix > this so that it is more clear that these are additional parameters to > mEventData? np, you are right. I myself was thinking these are additional entrees :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/cfc8e7a8f4a46ea09bcfbbcd5acffd627e224aed Bug 1402951 - no show events for content on document load complete, r=aklotz

Backed out for a11y test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=134133862&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/ac3b04d7c4fe42b65535bfeece101cd0c0058d65

correct version

Comment on attachment 8914390 [details] [diff] [review] patch + test fix Review of attachment 8914390 [details] [diff] [review]: ----------------------------------------------------------------- I'm assuming I needed to look at the test only, r+ for that

(In reply to Yura Zenevich [:yzen] from comment #18) > I'm assuming I needed to look at the test only, r+ for that yeah, Aaron r+ed the core changes. Thanks!

Pushed by surkov.alexander@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ffc4c6397692 no show events for content on document load complete, r=aklotz, yzen

Marco, can you please thumps up for the patch in the Nightly before I request uploading it?

Yes, this landed on Nightly 2017-10-03 2nd edition and performs as expected.

Comment on attachment 8914390 [details] [diff] [review] patch + test fix Approval Request Comment [Feature/Bug causing the regression]: bug 982842 [User impact if declined]: significant perf impact on a page loading if a11y is enabled [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no, Marco thumbed up for the changes [List of other uplifts needed for the feature/fix]:no [Is the change risky?]:low-moderate risk [Why is the change risky/not risky?]: non trivial changes, but it's replacing the complex and unnecessary logic on simpler and clearer code [String changes made/needed]: no

Hi David, Jim, I am a bit unclear on e10s + a11y plans for 57. Is this fix a must for 57? If not, I'd rather not uplift and let it ride the 58 train. Beta57 is getting an unusually large number of uplifts and I am trying to minimize risk to quality.

I think we need to take this if we are going to ship in 57. The final call for that has not been made. Jim what do you think?

(In reply to Ritu Kothari (:ritu) from comment #25) > Hi David, Jim, I am a bit unclear on e10s + a11y plans for 57. Is this fix a > must for 57? If not, I'd rather not uplift and let it ride the 58 train. > Beta57 is getting an unusually large number of uplifts and I am trying to > minimize risk to quality. Anything a11y + perf related that isn't invasive we want in 57. This looks like a safe landing to me, fixes a event bug, and apparently improves perf quite a bit.

Comment on attachment 8914390 [details] [diff] [review] patch + test fix Assuming e10s will not be enabled for a11y users and given the risk assessment, I would prefer letting this one ride the 58 train.

(In reply to Ritu Kothari (:ritu) from comment #28) > Comment on attachment 8914390 [details] [diff] [review] > patch + test fix > > Assuming e10s will not be enabled for a11y users and given the risk > assessment, I would prefer letting this one ride the 58 train. Ritu, we're shipping a11y in 57 for supported clients.

Ok, in that case let's uplift to 57. The fact that the fix has been verified is also reassuring.

(In reply to alexander :surkov from comment #24) > [Is this code covered by automated tests?]: yes > [Has the fix been verified in Nightly?]: yes > [Needs manual test from QE? If yes, steps to reproduce]: no, Marco thumbed > up for the changes Setting qe-verify- based on Alexander's assessment on manual testing needs and the fact that this fix has automated coverage.