434464 - invalidate access node cache

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

When nsISimpleDOMNode interface is used then we create access nodes for every DOM node. If the DOM node is accessible but doesn't have accessible object (for example, the DOM node is hidden) then we create access node object for it only. When the node is appeared then we won't invalidate the cache (because we invalidate the tree of accessible object only).

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Aaron Leventhal]

Why isn't this invalidation occuring in InvalidateCacheSubtree() when the nodes are made visible?

Comment 3

[Aaron Leventhal]

Attachment: Untested -- but I believe this is the correct fix.

Originally I never invalidated accessible subtrees when something was being shown, because I didn't see any reason to. After all, if the stuff didn't exist before why would there be any nodes to invalidate? Now I see that those nodes exist as nsAccessNodes even if they are hidden.

Comment 4

[Aaron Leventhal]

Attachment: Same fix as above without extra files

Comment 5

[Aaron Leventhal]

Attachment: Alternative patch -- only change for EVENT_SHOW. Still no need to RefreshNodes() for EVENT_DOM_CREATE, because there is no chance of having previous nsAccessNode's in subtree.

Comment 6

[Aaron Leventhal]

Try server builds from my first patch (I trust that one more):
https://build.mozilla.org/tryserver-builds/2008-05-19_15:[e-mail]-RemoveNoShowSubtreeInvalidationOptimization/

Comment 7

[alexander :surkov (:asurkov)]

Your patches won't work due to if (accessible) condition above.

Originally I thought to invalidate accessible tree like you suggested in patches. Actually we are interested i show events only (because we don't care about that is hiding). But our code is targeted to accessibile nodes (excepting RefreshNode() method that is called when accessible is hidden). Therefore I thought that's better way to make accessible our access node. That allows to keep our current code unchanged and we will fire events for those nodes. If we'll follow your way then I think we'll miss show events for access nodes and it doesn't seems to be correct.

Comment 8

[Aaron Leventhal]

I think we should try to fix the root of the problem, which is that invalidations are not working correctly.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> I think we should try to fix the root of the problem, which is that
> invalidations are not working correctly.
> 

That's I thought I tried to do :). What does invalidation mean here? We have access node for DOM node which is going to be visible. What should we do? I suspect we should remove access node and create accessible. That's I tried to do, though probably I chosen too common place for this?

Comment 10

[Aaron Leventhal]

Attachment: Don't require accessible for RefreshNodes() to happen

Comment 11

[Aaron Leventhal]

Comment on attachment 321733 [details] [diff] [review]
Don't require accessible for RefreshNodes() to happen

Surkov convinced me his approach is actually more performant.

Comment 12

[Aaron Leventhal]

Comment on attachment 321597 [details] [diff] [review]
patch

We need to do extensive testing with this.

I don't recommend it for Firefox 3.

Comment 13

[alexander :surkov (:asurkov)]

try server build https://build.mozilla.org/tryserver-builds/2008-05-20_16:[e-mail]-1211324552/

Comment 14

[Marco Zehe (:MarcoZ)]

So far nothing unusual with that try-server build. Surkov, did you have any specific testcases that I could run for this one?

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #14)
> So far nothing unusual with that try-server build. Surkov, did you have any
> specific testcases that I could run for this one?
> 

No, I haven't actually. But we need to test screen readers who uses ISimpleDOMNode interface to navigate through the a11y tree. Mainly this patch affects them.

Comment 16

[Marco Zehe (:MarcoZ)]

Pushed to mozilla-central in changeset:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/85d738b45221

Comment 17

[Marco Zehe (:MarcoZ)]

Attachment: Patch updated to CVS trunk.

This fix may be needed for JAWS 10. A try server build which will help us confirm this will appear here:
https://build.mozilla.org/tryserver-builds/2008-08-22_03:[e-mail]-bug434464/

Comment 18

[Aaron Leventhal]

Comment on attachment 335029 [details] [diff] [review]
Patch updated to CVS trunk.

This is crucial for compatibility with JAWS 10 (a beta is coming out for next week).

Without this fix, dynamic pages will be broken in JAWS. Web 2.0 a11y is our big push, and JAWS is the most popular screen reader, so this has a big impact.

Marco has been running this on trunk for a while with no problems.

Comment 19

[Samuel Sidler (old account; do not CC)]

Aaron, others, is there any way to create automated tests for this?

Comment 20

[Marco Zehe (:MarcoZ)]

Alexander, do you know if we have access to the nsAccessNode elements from JS?

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #20)
> Alexander, do you know if we have access to the nsAccessNode elements from JS?
> 

Yes, we always can query nsIAccessNode from nsIAccessible, so we could try to write mochitest I guess.

Comment 22

[Samuel Sidler (old account; do not CC)]

Let's try to get a test for this before taking in 1.9.0.2 then.

Comment 23

[Marco Zehe (:MarcoZ)]

I'm on a mochitest for this one. Will be posting a patch shortly.

Comment 24

[Marco Zehe (:MarcoZ)]

Attachment: WIP mochitest

Alex, this shows the steps that, according to Aaron, would normally trigger this bug in Firefox 3.0.1. However, I don't get a test failure right now, even without this patch.

Am I missing any steps?

Comment 25

[alexander :surkov (:asurkov)]

Marco, your test works fine if you put SimpleTest.finish(); into correct place - window.setTimeout. Thank you for working on this.

Comment 26

[Marco Zehe (:MarcoZ)]

Attachment: Patch2 to go into 1.9.0.2

Working Mochitest, plus the original patch updated to CVS trunk. Requesting review for test part.

Comment 27

[Marco Zehe (:MarcoZ)]

Comment on attachment 335375 [details] [diff] [review]
Patch2 to go into 1.9.0.2

Has Mochitest. has been baking on mozilla-central for over 1 month, is needed for JAWS 10 compatibility, which releases a public beta this week.

Comment 28

[Samuel Sidler (old account; do not CC)]

Comment on attachment 335375 [details] [diff] [review]
Patch2 to go into 1.9.0.2

Alright, let's get this landed. Approved for 1.9.0.2. Please land in CVS. a=ss

Comment 29

[Marco Zehe (:MarcoZ)]

Checking in accessible/src/base/nsAccessibilityService.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessibilityService.cpp,v  <--  nsAccessibilityService.cpp
new revision: 1.282; previous revision: 1.281
done
Checking in accessible/src/base/nsDocAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsDocAccessible.cpp,v  <--  nsDocAccessible.cpp
new revision: 1.247; previous revision: 1.246
done
Checking in accessible/tests/mochitest/Makefile.in;
/cvsroot/mozilla/accessible/tests/mochitest/Makefile.in,v  <--  Makefile.in
new revision: 1.20; previous revision: 1.19
done
RCS file: /cvsroot/mozilla/accessible/tests/mochitest/test_bug434464.html,v
done
Checking in accessible/tests/mochitest/test_bug434464.html;
/cvsroot/mozilla/accessible/tests/mochitest/test_bug434464.html,v  <--  test_bug434464.html
initial revision: 1.1
done

Comment 30

[Marco Zehe (:MarcoZ)]

Mochitest is now also pushed to mozilla-central in changeset:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/a491cb768bf4

Comment 31

[Marco Zehe (:MarcoZ)]

Verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.0.2) Gecko/2008082911 Firefox/3.0.2