280498 - Invalidate accessibility subtree for frame visibility/display style changes

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P1)

Description

[Aaron Leventhal]

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."

Comment 1

[Aaron Leventhal]

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

Comment 2

[Aaron Leventhal]

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

[Boris Zbarsky [:bzbarsky]]

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?

Comment 4

[Aaron Leventhal]

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

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Aaron Leventhal]

(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

[Boris Zbarsky [:bzbarsky]]

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...

Comment 8

[Aaron Leventhal]

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.

Comment 9

[Aaron Leventhal]

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

[Boris Zbarsky [:bzbarsky]]

Yeah, if you need to know about every single frame change then you want to just
hook in directly into frame destruction....

Comment 11

[Aaron Leventhal]

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

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

[Aaron Leventhal]

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);
}

Comment 14

[Boris Zbarsky [:bzbarsky]]

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.

Comment 15

[Aaron Leventhal]

(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.

Comment 16

[Aaron Leventhal]

Attachment: Testcase with some of the types of frames changes I know we care about

Comment 17

[Aaron Leventhal]

Attachment: Use RecreateFramesForContent() call or nsChangeHint_AccessibleChange of so we know when to invalidate accessible object hierarchy

Comment 18

[Boris Zbarsky [:bzbarsky]]

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...

Comment 19

[Aaron Leventhal]

(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

[Boris Zbarsky [:bzbarsky]]

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.

Comment 21

[Aaron Leventhal]

Attachment: Fix compile errors

The previous one didn't compile, just like bz said.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 177830 [details] [diff] [review]
Fix compile errors

Boris can do the sr on this

Comment 23

[Aaron Leventhal]

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.

Comment 24

[Boris Zbarsky [:bzbarsky]]

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.

Comment 25

[Aaron Leventhal]

> >-#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. 

Comment 26

[Aaron Leventhal]

Attachment: Address bz's comments

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.

Comment 27

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

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?

Comment 29

[Aaron Leventhal]

Attachment: Another round

Comment 30

[Aaron Leventhal]

The stuff from bug 280154 accidentally leaked into the last patch. Please ignore it.

Comment 31

[Aaron Leventhal]

Attachment: Remove extra presshell code from other patch

Comment 32

[Boris Zbarsky [:bzbarsky]]

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

Comment 33

[Asa Dotzler [:asa]]

Comment on attachment 180363 [details] [diff] [review]
Remove extra presshell code from other patch

a=asa

Comment 34

[Aaron Leventhal]

> >@@ -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

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 36

[Aaron Leventhal]

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

[Boris Zbarsky [:bzbarsky]]

> 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|.

Comment 38

[Aaron Leventhal]

Attachment: Diff of nsCSSFrameConstructor.cpp just to make sure we have it right.

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

[Boris Zbarsky [:bzbarsky]]

> I actually want to call InvalidateSubtreeFor() after

Make sense.  That diff looks good to me.

Comment 40

[Aaron Leventhal]

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