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).
Created attachment 321597 [details] [diff] [review] patch
Why isn't this invalidation occuring in InvalidateCacheSubtree() when the nodes are made visible?
Created attachment 321645 [details] [diff] [review] 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.
Created attachment 321647 [details] [diff] [review] Same fix as above without extra files
Created attachment 321648 [details] [diff] [review] 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.
Try server builds from my first patch (I trust that one more): https://build.mozilla.org/tryserver-builds/2008-05-19_15:firstname.lastname@example.org-RemoveNoShowSubtreeInvalidationOptimization/
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.
I think we should try to fix the root of the problem, which is that invalidations are not working correctly.
(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?
Created attachment 321733 [details] [diff] [review] Don't require accessible for RefreshNodes() to happen
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 on attachment 321597 [details] [diff] [review] patch We need to do extensive testing with this. I don't recommend it for Firefox 3.
So far nothing unusual with that try-server build. Surkov, did you have any specific testcases that I could run for this one?
(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.
Pushed to mozilla-central in changeset: http://hg.mozilla.org/mozilla-central/index.cgi/rev/85d738b45221
Created attachment 335029 [details] [diff] [review] 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:email@example.com/
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.
Aaron, others, is there any way to create automated tests for this?
Alexander, do you know if we have access to the nsAccessNode elements from JS?
(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.
Let's try to get a test for this before taking in 184.108.40.206 then.
I'm on a mochitest for this one. Will be posting a patch shortly.
Created attachment 335351 [details] [diff] [review] 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?
Marco, your test works fine if you put SimpleTest.finish(); into correct place - window.setTimeout. Thank you for working on this.
Created attachment 335375 [details] [diff] [review] Patch2 to go into 220.127.116.11 Working Mochitest, plus the original patch updated to CVS trunk. Requesting review for test part.
Comment on attachment 335375 [details] [diff] [review] Patch2 to go into 18.104.22.168 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 on attachment 335375 [details] [diff] [review] Patch2 to go into 22.214.171.124 Alright, let's get this landed. Approved for 126.96.36.199. Please land in CVS. a=ss
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
Mochitest is now also pushed to mozilla-central in changeset: http://hg.mozilla.org/mozilla-central/index.cgi/rev/a491cb768bf4
Verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:188.8.131.52) Gecko/2008082911 Firefox/3.0.2