The default bug view has changed. See this FAQ.

invalidate access node cache

RESOLVED FIXED in mozilla1.9.1a1

Status

()

Core
Disability Access APIs
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

({access, verified1.9.0.2})

unspecified
mozilla1.9.1a1
x86
Windows XP
access, verified1.9.0.2
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: post1.9[has-patch,has-review])

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

9 years ago
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

9 years ago
Created attachment 321597 [details] [diff] [review]
patch
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #321597 - Flags: review?(aaronleventhal)

Comment 2

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

Comment 3

9 years ago
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.
Attachment #321645 - Flags: review?(surkov.alexander)

Comment 4

9 years ago
Created attachment 321647 [details] [diff] [review]
Same fix as above without extra files
Attachment #321645 - Attachment is obsolete: true
Attachment #321647 - Flags: review?(surkov.alexander)
Attachment #321645 - Flags: review?(surkov.alexander)

Comment 5

9 years ago
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.
Attachment #321648 - Flags: review?(surkov.alexander)

Comment 6

9 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

9 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

9 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

9 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

9 years ago
Created attachment 321733 [details] [diff] [review]
Don't require accessible for RefreshNodes() to happen
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

9 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

9 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)

Updated

9 years ago
Blocks: 391535
Whiteboard: post1.9[has-patch,has-review]
(Assignee)

Comment 13

9 years ago
try server build https://build.mozilla.org/tryserver-builds/2008-05-20_16:03-surkov.alexander@gmail.com-1211324552/
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

9 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.
Pushed to mozilla-central in changeset:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/85d738b45221
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Target Milestone: --- → mozilla1.9.1a1
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

9 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?
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?
(Assignee)

Comment 21

9 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.
Let's try to get a test for this before taking in 1.9.0.2 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?
(Assignee)

Comment 25

9 years ago
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 1.9.0.2

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

9 years ago
Attachment #335375 - Flags: review?(surkov.alexander) → review+
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 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+
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
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:1.9.0.2) Gecko/2008082911 Firefox/3.0.2
Keywords: fixed1.9.0.2 → verified1.9.0.2
(Assignee)

Updated

9 years ago
Blocks: 457367
You need to log in before you can comment on or make changes to this bug.