Closed
Bug 434464
Opened 16 years ago
Closed 16 years ago
invalidate access node cache
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9.1a1
People
(Reporter: surkov, Assigned: surkov)
References
Details
(Keywords: access, verified1.9.0.2, Whiteboard: post1.9[has-patch,has-review])
Attachments
(2 files, 6 obsolete files)
2.99 KB,
patch
|
aaronlev
:
review+
|
Details | Diff | Splinter Review |
6.56 KB,
patch
|
aaronlev
:
review+
samuel.sidler+old
:
approval1.9.0.2+
|
Details | Diff | Splinter Review |
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).
Assignee | ||
Comment 1•16 years ago
|
||
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #321597 -
Flags: review?(aaronleventhal)
Comment 2•16 years ago
|
||
Why isn't this invalidation occuring in InvalidateCacheSubtree() when the nodes are made visible?
Comment 3•16 years ago
|
||
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.
Attachment #321645 -
Flags: review?(surkov.alexander)
Comment 4•16 years ago
|
||
Attachment #321645 -
Attachment is obsolete: true
Attachment #321647 -
Flags: review?(surkov.alexander)
Attachment #321645 -
Flags: review?(surkov.alexander)
Comment 5•16 years ago
|
||
Attachment #321648 -
Flags: review?(surkov.alexander)
Comment 6•16 years ago
|
||
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/
Assignee | ||
Comment 7•16 years ago
|
||
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•16 years ago
|
||
I think we should try to fix the root of the problem, which is that invalidations are not working correctly.
Assignee | ||
Comment 9•16 years ago
|
||
(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•16 years ago
|
||
Attachment #321647 -
Attachment is obsolete: true
Attachment #321648 -
Attachment is obsolete: true
Attachment #321733 -
Flags: review?(surkov.alexander)
Attachment #321647 -
Flags: review?(surkov.alexander)
Attachment #321648 -
Flags: review?(surkov.alexander)
Comment 11•16 years ago
|
||
Comment on attachment 321733 [details] [diff] [review] Don't require accessible for RefreshNodes() to happen Surkov convinced me his approach is actually more performant.
Attachment #321733 -
Attachment is obsolete: true
Attachment #321733 -
Flags: review?(surkov.alexander)
Comment 12•16 years ago
|
||
Comment on attachment 321597 [details] [diff] [review] patch We need to do extensive testing with this. I don't recommend it for Firefox 3.
Attachment #321597 -
Flags: review?(aaronleventhal) → review+
Assignee | ||
Comment 13•16 years ago
|
||
try server build https://build.mozilla.org/tryserver-builds/2008-05-20_16:03-surkov.alexander@gmail.com-1211324552/
Comment 14•16 years ago
|
||
So far nothing unusual with that try-server build. Surkov, did you have any specific testcases that I could run for this one?
Assignee | ||
Comment 15•16 years ago
|
||
(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•16 years ago
|
||
Pushed to mozilla-central in changeset: http://hg.mozilla.org/mozilla-central/index.cgi/rev/85d738b45221
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Target Milestone: --- → mozilla1.9.1a1
Comment 17•16 years ago
|
||
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•16 years ago
|
||
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.
Attachment #335029 -
Flags: approval1.9.0.2?
Comment 19•16 years ago
|
||
Aaron, others, is there any way to create automated tests for this?
Comment 20•16 years ago
|
||
Alexander, do you know if we have access to the nsAccessNode elements from JS?
Assignee | ||
Comment 21•16 years ago
|
||
(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•16 years ago
|
||
Let's try to get a test for this before taking in 1.9.0.2 then.
Comment 23•16 years ago
|
||
I'm on a mochitest for this one. Will be posting a patch shortly.
Comment 24•16 years ago
|
||
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?
Assignee | ||
Comment 25•16 years ago
|
||
Marco, your test works fine if you put SimpleTest.finish(); into correct place - window.setTimeout. Thank you for working on this.
Comment 26•16 years ago
|
||
Working Mochitest, plus the original patch updated to CVS trunk. Requesting review for test part.
Attachment #335029 -
Attachment is obsolete: true
Attachment #335351 -
Attachment is obsolete: true
Attachment #335375 -
Flags: review?(surkov.alexander)
Attachment #335029 -
Flags: approval1.9.0.2?
Updated•16 years ago
|
Attachment #335375 -
Flags: review?(surkov.alexander) → review+
Comment 27•16 years ago
|
||
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.
Attachment #335375 -
Flags: approval1.9.0.2?
Comment 28•16 years ago
|
||
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
Attachment #335375 -
Flags: approval1.9.0.2? → approval1.9.0.2+
Comment 29•16 years ago
|
||
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
Keywords: access,
fixed1.9.0.2
Comment 30•16 years ago
|
||
Mochitest is now also pushed to mozilla-central in changeset: http://hg.mozilla.org/mozilla-central/index.cgi/rev/a491cb768bf4
Comment 31•16 years ago
|
||
Verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.0.2) Gecko/2008082911 Firefox/3.0.2
Keywords: fixed1.9.0.2 → verified1.9.0.2
You need to log in
before you can comment on or make changes to this bug.
Description
•