Last Comment Bug 434464 - invalidate access node cache
: invalidate access node cache
Status: RESOLVED FIXED
post1.9[has-patch,has-review]
: access, verified1.9.0.2
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86 Windows XP
: -- normal (vote)
: mozilla1.9.1a1
Assigned To: alexander :surkov
:
:
Mentors:
Depends on:
Blocks: 191a11y 457367
  Show dependency treegraph
 
Reported: 2008-05-19 02:47 PDT by alexander :surkov
Modified: 2008-09-26 19:54 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (2.99 KB, patch)
2008-05-19 06:40 PDT, alexander :surkov
aaronlev: review+
Details | Diff | Splinter Review
Untested -- but I believe this is the correct fix. (5.12 KB, patch)
2008-05-19 13:31 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Same fix as above without extra files (1.51 KB, patch)
2008-05-19 13:35 PDT, Aaron Leventhal
no flags Details | Diff | Splinter 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. (1.44 KB, patch)
2008-05-19 13:40 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Don't require accessible for RefreshNodes() to happen (1.79 KB, patch)
2008-05-20 01:56 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Patch updated to CVS trunk. (2.63 KB, patch)
2008-08-22 04:22 PDT, Marco Zehe (:MarcoZ)
no flags Details | Diff | Splinter Review
WIP mochitest (3.21 KB, patch)
2008-08-25 06:43 PDT, Marco Zehe (:MarcoZ)
no flags Details | Diff | Splinter Review
Patch2 to go into 1.9.0.2 (6.56 KB, patch)
2008-08-25 09:55 PDT, Marco Zehe (:MarcoZ)
aaronlev: review+
samuel.sidler+old: approval1.9.0.2+
Details | Diff | Splinter Review

Description alexander :surkov 2008-05-19 02:47:53 PDT
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 2008-05-19 06:40:32 PDT
Created attachment 321597 [details] [diff] [review]
patch
Comment 2 Aaron Leventhal 2008-05-19 11:53:15 PDT
Why isn't this invalidation occuring in InvalidateCacheSubtree() when the nodes are made visible?
Comment 3 Aaron Leventhal 2008-05-19 13:31:38 PDT
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.
Comment 4 Aaron Leventhal 2008-05-19 13:35:44 PDT
Created attachment 321647 [details] [diff] [review]
Same fix as above without extra files
Comment 5 Aaron Leventhal 2008-05-19 13:40:00 PDT
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.
Comment 6 Aaron Leventhal 2008-05-19 15:52:23 PDT
Try server builds from my first patch (I trust that one more):
https://build.mozilla.org/tryserver-builds/2008-05-19_15:07-aaronleventhal@moonset.net-RemoveNoShowSubtreeInvalidationOptimization/
Comment 7 alexander :surkov 2008-05-19 17:50:38 PDT
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 2008-05-20 00:40:53 PDT
I think we should try to fix the root of the problem, which is that invalidations are not working correctly.
Comment 9 alexander :surkov 2008-05-20 00:51:47 PDT
(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 2008-05-20 01:56:06 PDT
Created attachment 321733 [details] [diff] [review]
Don't require accessible for RefreshNodes() to happen
Comment 11 Aaron Leventhal 2008-05-20 02:16:57 PDT
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 2008-05-20 02:17:33 PDT
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 14 Marco Zehe (:MarcoZ) 2008-05-21 08:17:22 PDT
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 2008-05-25 19:17:08 PDT
(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) 2008-06-11 02:19:57 PDT
Pushed to mozilla-central in changeset:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/85d738b45221
Comment 17 Marco Zehe (:MarcoZ) 2008-08-22 04:22:16 PDT
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:36-mzehe@mozilla.com-bug434464/
Comment 18 Aaron Leventhal 2008-08-22 04:59:13 PDT
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) 2008-08-22 15:56:20 PDT
Aaron, others, is there any way to create automated tests for this?
Comment 20 Marco Zehe (:MarcoZ) 2008-08-22 23:01:20 PDT
Alexander, do you know if we have access to the nsAccessNode elements from JS?
Comment 21 alexander :surkov 2008-08-23 00:05:15 PDT
(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) 2008-08-23 19:59:43 PDT
Let's try to get a test for this before taking in 1.9.0.2 then.
Comment 23 Marco Zehe (:MarcoZ) 2008-08-25 05:58:47 PDT
I'm on a mochitest for this one. Will be posting a patch shortly.
Comment 24 Marco Zehe (:MarcoZ) 2008-08-25 06:43:45 PDT
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?
Comment 25 alexander :surkov 2008-08-25 09:29:33 PDT
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) 2008-08-25 09:55:51 PDT
Created attachment 335375 [details] [diff] [review]
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) 2008-08-25 11:29:16 PDT
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) 2008-08-25 11:31:13 PDT
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) 2008-08-25 12:11:42 PDT
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) 2008-08-25 12:26:15 PDT
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) 2008-09-02 08:58:57 PDT
Verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.0.2) Gecko/2008082911 Firefox/3.0.2

Note You need to log in before you can comment on or make changes to this bug.