Closed
Bug 280498
Opened 20 years ago
Closed 19 years ago
Invalidate accessibility subtree for frame visibility/display style changes
Categories
(Core :: Disability Access APIs, defect, P1)
Tracking
()
RESOLVED
FIXED
People
(Reporter: aaronlev, Assigned: aaronlev)
References
Details
(Keywords: access, crash)
Attachments
(3 files, 4 obsolete files)
1.54 KB,
text/html
|
Details | |
25.63 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
asa
:
approval1.8b2+
|
Details | Diff | Splinter Review |
3.92 KB,
patch
|
Details | Diff | Splinter Review |
A frame can be destroyed without even a DOM mutation event. We have to be careful because: 1) We cache the frame in nsHTMLTextAccessible 2) It indicates that something important has changed about the node -- very likely display:none was set on it. The accessible tree now needs to reflect this. Caching the frame is an optimization for text accessibles, because GetPrimaryFrameFor() does a lot more calculation for text frames than what the accessible tree needs. Doing GetPFF on text frames would increase the size of the primary frame map since those are normally not stored there. We already have the text frames as the accessible tree walker iterates through, and cache those on the accessibles. I'd like to keep the optimzation if possible. Boris says "You could set the NS_FRAME_EXTERNAL_REFERENCE bit on the framestate and add a hook to PresShell::ClearFrameRefs that's #ifdef ACCESSIBILITY."
Assignee | ||
Updated•20 years ago
|
Priority: -- → P1
Assignee | ||
Comment 1•19 years ago
|
||
We should also invalidate based on some attribute changes, but I'm not sure what HTML or XUL attribute changes to ignore for performance reasons (or if we should attempt that).
Assignee | ||
Comment 2•19 years ago
|
||
Actually for the important attribute changes that would really affect the accessibility tree, we should see frames being destroyed or created. For example, if a script changes the type attribute on an input element.
Comment 3•19 years ago
|
||
Sounds like you just need a list of attributes that accessibility actually depends on and deal with them changing, basically... I suspect doing a grep for GetAttr in /accessibility will give you such a list, no?
Assignee | ||
Comment 4•19 years ago
|
||
It's not that simple, because the type of accessible that's created could depend on the frame itself, which could depend on an attribute. For example, the type attribute of an input tag. Or, the computed display: property for an element could change because an attribute changed. The XBL binding used for a XUL element can also be affected by an attribute. The XBL binding is responsible for deciding what kind of accessible to create. Also, some attributes used in the accessibility code only affect the state -- for example, the checked is may be used to determine if something is checked. We don't want to destroy the current accessible in that case, just throw a state change event. Yes, we can take all of these things into account, and might have to, but we need to be careful. Future coders modifying Mozilla may not remember to add the special casing in the right place. Maybe we can get around having to special case all of these attributes by just looking for frame changes?
Comment 5•19 years ago
|
||
It sounds like you need to look for frame changes anyway... But don't you need to change accesibles when, say, the src of an image changes? That doesn't cause a frame change.
Assignee | ||
Comment 6•19 years ago
|
||
(In reply to comment #5) > It sounds like you need to look for frame changes anyway... But don't you need > to change accesibles when, say, the src of an image changes? That doesn't cause > a frame change. If the src changes we don't need to destroy and create a new accessible. We just need to fire a LOCATIONCHANGE if it's bounds change, a NAMECHANGE if it's ALT text changes, a STATECHANGE if one of its states changes, and a VALUECHANGE since it has a new URL. Then the screen reader or other assistive technology can call back in to get the new data if it wishes.
Comment 7•19 years ago
|
||
OK. So then you just need to audit accessible creation, I guess, and see what attributes the type of accessible depends on. Then decide whether those attribute changes will always cause frame reconstructs...
Assignee | ||
Comment 8•19 years ago
|
||
Boris, I looked into NS_FRAME_EXTERNAL_REFERENCE, which causes a call into ClearFrameRefs(). It's not actually being set anywhere in the codebase. Maybe we should use that free bit for something else. Once accesssibility is active I need to find out about all frame changes, so I'd rather hook in without setting the state bit on each and every frame. I can just use a static bool in layout like gIsAccessibilityOn.
Assignee | ||
Comment 9•19 years ago
|
||
Duh, I take it back. We do use NS_FRAME_EXTERNAL_REFERENCE in esm::SetFrameExternalReference() http://lxr.mozilla.org/seamonkey/source/content/events/src/nsEventStateManager.cpp#396
Comment 10•19 years ago
|
||
Yeah, if you need to know about every single frame change then you want to just hook in directly into frame destruction....
Assignee | ||
Comment 11•19 years ago
|
||
I'm running into problems because ClearFrameRefs() isn't really a good place to hook in. It doesn't catch visibility style changes, or when display: none is cleared and the frame is created.
Comment 12•19 years ago
|
||
Visibility style changes only affect painting -- they don't affect the frame tree at all. And frame creation doesn't go through ClearFrameRefs of course (note summary of this bug; I thought that's what we were talking about).
Assignee | ||
Comment 13•19 years ago
|
||
Resummarizing to reflect what this bug should really be for. How about a new style hint like NS_STYLE_HINT_ACCESSIBILITY_CHANGE. Then we would do something like: #ifdef ACCESSIBILITY if (gIsAccessibilityActive && (frameHint & NS_STYLE_HINT_ACCESSIBILITY_CHANGE)) { mAccService->InvalidateSubteeForFrame(frame); }
Summary: Invalidate accessibility subtree when frame destroyed → Invalidate accessibility subtree for frame visibility/display style changes
Comment 14•19 years ago
|
||
Frame changes can happen for reasons other than a style change (content removal/insertion, possibly non-style content state changes (eg images being replaced by their replaced content)). But yes, if all you care about is style changes, we could probably add a new change hint for this. Note that it'd only be guaranteed to happen for the "rootmost" frame that had the relevant style change.
Assignee | ||
Comment 15•19 years ago
|
||
(In reply to comment #14) > Frame changes can happen for reasons other than a style change (content > removal/insertion, possibly non-style content state changes (eg images being > replaced by their replaced content)). I'm not sure if we care about those -- we'll still have an image accessible for that anyway. Will have to test. > But yes, if all you care about is style changes, we could probably add a new > change hint for this. Note that it'd only be guaranteed to happen for the > "rootmost" frame that had the relevant style change. Yes that's actually what I want.
Assignee | ||
Comment 16•19 years ago
|
||
Assignee | ||
Comment 17•19 years ago
|
||
Attachment #177772 -
Flags: review?(bzbarsky)
Comment 18•19 years ago
|
||
I'm not going to be able to do a thorough review for probably about two weeks, but that patch doesn't look like it should compile, if nothing else...
Assignee | ||
Comment 19•19 years ago
|
||
(In reply to comment #18) > I'm not going to be able to do a thorough review for probably about two weeks, > but that patch doesn't look like it should compile, if nothing else... It will unless I forgot something in the patch. I built and ran through the tests for me. What looks like it won't compile?
Comment 20•19 years ago
|
||
NS_STYLE_HINT_ACCESSIBLECHANGE is never defined, if nothing else. Also, the XXX comment there is wrong. The old NS_STYLE_HINT constants are just that -- old and kept just to avoid having to change lots of code that uses them. New code should be using nsChangeHint values as needed, and we shouldn't be adding more NS_STYLE_HINT constants.
Assignee | ||
Comment 21•19 years ago
|
||
The previous one didn't compile, just like bz said.
Attachment #177772 -
Attachment is obsolete: true
Attachment #177830 -
Flags: superreview?(roc)
Attachment #177830 -
Flags: review?(bzbarsky)
Attachment #177772 -
Flags: review?(bzbarsky)
Comment on attachment 177830 [details] [diff] [review] Fix compile errors Boris can do the sr on this
Attachment #177830 -
Flags: superreview?(roc) → superreview?(bzbarsky)
Assignee | ||
Comment 23•19 years ago
|
||
I don't have a testcase right now, but this almost certainly fixes potential crashes in the accessibility module. We need to make sure our accessible object tree remains valid.
Severity: normal → critical
Keywords: crash
Comment 24•19 years ago
|
||
Comment on attachment 177830 [details] [diff] [review] Fix compile errors >Index: layout/base/nsCSSFrameConstructor.cpp >+#ifdef ACCESSIBILITY >+ if ((frameChange & nsChangeHint_AccessibleChange) && >+ mPresShell->IsAccessibilityActive()) { That only handles the case when the actual node that had an attribute change or whatever it was that triggered the style change has the nsChangeHint_AccessibleChange hint. What if it doesn't but one of its descendants does? Seems to me like you want this code to be in ProcessRestyledFrames (which is called from a lot more places than just right here, by the way), not here. >Index: layout/base/nsChangeHint.h > #define NS_STYLE_HINT_FRAMECHANGE \ > nsChangeHint(NS_STYLE_HINT_REFLOW | nsChangeHint_ReconstructFrame) > >- Random whitespace change? Please take that out. >Index: layout/base/nsIPresShell.h >+#ifdef ACCESSIBILITY >+ NS_IMETHOD_(PRBool) IsAccessibilityActive(); >+#endif Why NS_IMETHOD? Why not just virtual PRBool? >Index: layout/base/nsPresShell.cpp So accessibility never becomes inactive? >Index: layout/style/nsStyleStruct.cpp > if ((mVisible != aOther.mVisible) && Want to remove this check while you're here? Given the previous |if|, it's redundant... You need to fix the MaxDifference() method for this struct; otherwise style reresolution will assert. >Index: accessible/public/nsIAccessibilityService.idl >+ [noscript] void invalidateSubtreeFor(in nsIPresShell aPresShell, in nsIContent aContent); Document what this method should do, please. >Index: accessible/public/nsPIAccessibleDocument.idl >+ void invalidateCacheSubtree(in nsIDOMNode aStartNode, in PRUint32 aChangeEvent); Same here. What is aChangeEvent, exactly? >Index: accessible/src/base/nsAccessibilityService.cpp > nsDocAccessible::GetAccessibleInParentChain(nsIDOMNode *aNode, The changes to this method look wrong. What if aNode is the document node? >-#ifdef XP_WIN >- // Windows MSAA clients crash if they listen to create or destroy events >- aAccessibleEventType = nsIAccessibleEvent::EVENT_REORDER; >-#endif Why is this code not needed at the spot to which you moved this chunk of code? >Index: accessible/src/base/nsDocAccessible.h >+ void nsDocAccessible::ShutdownNodes(nsIDOMNode I'm pretty sure the redundant class name here will make this not compile on at least some of our platforms.
Attachment #177830 -
Flags: superreview?(bzbarsky)
Attachment #177830 -
Flags: superreview-
Attachment #177830 -
Flags: review?(bzbarsky)
Attachment #177830 -
Flags: review-
Assignee | ||
Comment 25•19 years ago
|
||
> >-#ifdef XP_WIN
> >- // Windows MSAA clients crash if they listen to create or destroy events
> >- aAccessibleEventType = nsIAccessibleEvent::EVENT_REORDER;
> >-#endif
>
> Why is this code not needed at the spot to which you moved this chunk of code?
We're going to use EVENT_HIDE and EVENT_SHOW now, because they are safe. From
the AT or user's point of view it doesn't matter whether something was created
vs. shown or destroyed vs. hidden -- that's an implementation detail in our app.
We don't fire EVENT_CREATE and EVENT_DESTROY on Windows because they are unsafe
so screen readers won't listen for them. Apparently listening for them can can
cause crashes due to race conditions, in some applications. We don't need to
force everything into EVENT_REORDER anymore.
Assignee | ||
Comment 26•19 years ago
|
||
Bz, I decided to leave the differentiation of show/hide/reorder events coming from layout for another bug. I'm not sure if we'll need to know the difference yet. The invalidation of parts of the accessibility cache happens either way, and the AT can look at the changes after they've happened.
Attachment #177830 -
Attachment is obsolete: true
Attachment #179976 -
Flags: superreview?(bzbarsky)
Attachment #179976 -
Flags: review?(bzbarsky)
Comment 27•19 years ago
|
||
So you don't need to deal with ReframeContainingBlock being called from RecreateFramesForContent? Why not? In ProcessRestyledFrames, why is your code running if we had a frame reconstruct hint? That seems wrong. Why is IsAccessibilityActive virtual, if the member is on nsIPresShell? Should be one or the other, but not both, no? The style struct changes will miss returning an accessible change if both the visibility and the lang group or direction have changed.
Comment 28•19 years ago
|
||
Comment on attachment 179976 [details] [diff] [review] Address bz's comments >Index: accessible/public/nsPIAccessible.idl >+ /** >+ * Set the child count to -1 (unknown) and cached pointers to children Is the second half of that sentence missing? If not, what is this supposed to mean?
Attachment #179976 -
Flags: superreview?(bzbarsky)
Attachment #179976 -
Flags: superreview-
Attachment #179976 -
Flags: review?(bzbarsky)
Attachment #179976 -
Flags: review-
Assignee | ||
Comment 29•19 years ago
|
||
Attachment #179976 -
Attachment is obsolete: true
Assignee | ||
Updated•19 years ago
|
Attachment #180341 -
Flags: superreview?(bzbarsky)
Attachment #180341 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 30•19 years ago
|
||
The stuff from bug 280154 accidentally leaked into the last patch. Please ignore it.
Assignee | ||
Updated•19 years ago
|
Attachment #180341 -
Flags: superreview?(bzbarsky)
Attachment #180341 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 31•19 years ago
|
||
Attachment #180363 -
Flags: superreview?(bzbarsky)
Attachment #180363 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•19 years ago
|
Attachment #180341 -
Attachment is obsolete: true
Comment 32•19 years ago
|
||
Comment on attachment 180363 [details] [diff] [review] Remove extra presshell code from other patch >Index: layout/base/nsCSSFrameConstructor.cpp >@@ -10156,12 +10159,25 @@ nsCSSFrameConstructor::ProcessRestyledFr > } >- >+ > #ifdef DEBUG Nix that whitespace change. >@@ -11593,10 +11609,22 @@ nsCSSFrameConstructor::RecreateFramesFor >+#ifdef ACCESSIBILITY >+ if (mPresShell->IsAccessibilityActive()) { So... If we're actually blowing away all frames to the containing block, don't we need to invalidate the subtree for the containing block's content, not aContent? That is, are those accessibles possibly holding refs to frames that are between the frame of aContent and its containing block? If so, we need to clear those refs, right? >Index: layout/style/nsStyleContext.cpp > nsChangeHint maxHint = nsChangeHint(NS_STYLE_HINT_FRAMECHANGE | >- nsChangeHint_UpdateCursor); >+ nsChangeHint_UpdateCursor | nsChangeHint_AccessibleChange); Hmm... I guess this is OK for now, but the Visibility struct is actually misplaced in the style context code. Could you file a followup bug on me for that? >Index: accessible/public/nsIAccessibilityService.idl >+ [noscript] void invalidateSubtreeFor(in nsIPresShell aPresShell, in nsIContent aContent); This needs a uuid change. >Index: accessible/public/nsPIAccessible.idl >-[scriptable, uuid(52F1BE88-84F7-4f7f-B31C-062AFE7DF15D)] >+[scriptable, uuid(EC88FE71-8065-4310-9189-9E3B7A3D74E4)] Whereas here you just added comments; not sure why the uuid got changed here. >Index: accessible/public/nsPIAccessibleDocument.idl >- void invalidateCacheSubtree(in nsIDOMNode aStartNode); Change the uuid. With those comments addressed, r+sr=bzbarsky
Attachment #180363 -
Flags: superreview?(bzbarsky)
Attachment #180363 -
Flags: superreview+
Attachment #180363 -
Flags: review?(bzbarsky)
Attachment #180363 -
Flags: review+
Assignee | ||
Updated•19 years ago
|
Attachment #180363 -
Flags: approval1.8b2?
Comment 33•19 years ago
|
||
Comment on attachment 180363 [details] [diff] [review] Remove extra presshell code from other patch a=asa
Attachment #180363 -
Flags: approval1.8b2? → approval1.8b2+
Assignee | ||
Comment 34•19 years ago
|
||
> >@@ -11593,10 +11609,22 @@ nsCSSFrameConstructor::RecreateFramesFor
>
> >+#ifdef ACCESSIBILITY
> >+ if (mPresShell->IsAccessibilityActive()) {
>
> So... If we're actually blowing away all frames to the containing block, don't
> we need to invalidate the subtree for the containing block's content, not
> aContent? That is, are those accessibles possibly holding refs to frames that
> are between the frame of aContent and its containing block? If so, we need to
> clear those refs, right?
We actually do deal with the parent aContent -- but that's done in the
implementation of nsAccessibilityService::InvalidateSubtreeFor(presshell,
content); We conservatively assume that the changing of a node could affect
the accessibles based on their siblings. The impl goes up to the parent.
However, I suddenly noticed that RecreateFrames has a bad error. The |if
(container)| block appears to have confusion between the variables parent and
frame. Sent you email about that.
Comment 35•19 years ago
|
||
> We actually do deal with the parent aContent The containing block doesn't have to be the parent; it could be arbitrarily far up the parent chain. We could be blowing away the whole frame tree here, in fact. > However, I suddenly noticed that RecreateFrames has a bad error. Bug 289377. The error doesn't really affect anything, just disables a questionable performance optimization.
Assignee | ||
Comment 36•19 years ago
|
||
Bz, you want something like this? if (accService) { nsIFrame* containingBlock = GetIBContainingBlockFor(frame); nsCOMPtr<nsIContent> parentContent = frame->GetContent(); accService->InvalidateSubtreeFor(mPresShell, parentContent); } Just want to be clear before I check in.
Comment 37•19 years ago
|
||
> Bz, you want something like this?
Yes, if it's done _before_ the frame is recreated. Doing it after will crash,
since |frame| will have been destroyed. Oh, and you want to GetContent on the
containing block, not on |frame|.
Assignee | ||
Comment 38•19 years ago
|
||
Bz, I remembered that I actually want to call InvalidateSubtreeFor() after the new frames have been created. The reason is, it will fire an event that may result in the external assistive technology to ask for the new objects immediately -- so the new frames need to be ready at that point.
Comment 39•19 years ago
|
||
> I actually want to call InvalidateSubtreeFor() after
Make sense. That diff looks good to me.
Assignee | ||
Comment 40•19 years ago
|
||
Checking in layout/base/nsCSSFrameConstructor.cpp; /cvsroot/mozilla/layout/base/nsCSSFrameConstructor.cpp,v <-- nsCSSFrameConstructor.cpp new revision: 1.1094; previous revision: 1.1093 done Checking in layout/base/nsChangeHint.h; /cvsroot/mozilla/layout/base/nsChangeHint.h,v <-- nsChangeHint.h new revision: 3.9; previous revision: 3.8 done Checking in layout/base/nsIPresShell.h; /cvsroot/mozilla/layout/base/nsIPresShell.h,v <-- nsIPresShell.h new revision: 3.154; previous revision: 3.153 done Checking in layout/base/nsPresShell.cpp; /cvsroot/mozilla/layout/base/nsPresShell.cpp,v <-- nsPresShell.cpp new revision: 3.836; previous revision: 3.835 done Checking in layout/style/nsStyleContext.cpp; /cvsroot/mozilla/layout/style/nsStyleContext.cpp,v <-- nsStyleContext.cpp new revision: 3.258; previous revision: 3.257 done Checking in layout/style/nsStyleStruct.cpp; /cvsroot/mozilla/layout/style/nsStyleStruct.cpp,v <-- nsStyleStruct.cpp new revision: 3.127; previous revision: 3.126 done Checking in accessible/public/nsIAccessibilityService.idl; /cvsroot/mozilla/accessible/public/nsIAccessibilityService.idl,v <-- nsIAccessibilityService.idl new revision: 1.46; previous revision: 1.45 done Checking in accessible/public/nsPIAccessible.idl; /cvsroot/mozilla/accessible/public/nsPIAccessible.idl,v <-- nsPIAccessible.idl new revision: 1.3; previous revision: 1.2 done Checking in accessible/public/nsPIAccessibleDocument.idl; /cvsroot/mozilla/accessible/public/nsPIAccessibleDocument.idl,v <-- nsPIAccessibleDocument.idl new revision: 1.4; previous revision: 1.3 done Checking in accessible/src/base/nsAccessibilityService.cpp; /cvsroot/mozilla/accessible/src/base/nsAccessibilityService.cpp,v <-- nsAccessibilityService.cpp new revision: 1.138; previous revision: 1.137 done Checking in accessible/src/base/nsDocAccessible.cpp; /cvsroot/mozilla/accessible/src/base/nsDocAccessible.cpp,v <-- nsDocAccessible.cpp new revision: 1.59; previous revision: 1.58 done Checking in accessible/src/base/nsDocAccessible.h; /cvsroot/mozilla/accessible/src/base/nsDocAccessible.h,v <-- nsDocAccessible.h new revision: 1.25; previous revision: 1.24 done
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•