106710 - Active Accessibility: expose accessible anonymous content

Status: RESOLVED FIXED

(SeaMonkey :: General, defect, P1)

Description

[Aaron Leventhal]

If we don't expose anonymous content, we're missing out on the OK/Cancel/etc.
buttons in a <dialog>, and we don't expose any HTML at all in the tabbed browser.

This is because as we walk the normal content tree, we've been skipping these
computed nodes.

Comment 1

[Aaron Leventhal]

Attachment: Tested - works! (See next attachment for easy-to-read version of new nsAccessibleTreeWalker class)

Comment 2

[Aaron Leventhal]

Attachment: Providing entire file makes it easier to read the completely rewritten parts (nsDOMTreeWalker is now nsAccessibleTreeWalker)

Comment 3

[Aaron Leventhal]

Duh, forgot to mention, the second patch is nsAccessible.cpp

Comment 4

[John Gaunt (redfive)]

Comment on attachment 55077 [details] [diff] [review]
Tested - works! (See next attachment for easy-to-read version of new nsAccessibleTreeWalker class)

Looks like you moved the test in nsAccessibilityService:: GetAccessibleFor down in the order. Why? I thought we decided it was best to take care of that right away, since we don't need to do anything else.

needs a newline at the end of nsGenericAccessible.cpp

In nsIFrameRootAcc, why not just call Init() when we create the object rather than have init calls in ever method?

Why add the following:
+    nsCOMPtr<nsIDOMNode> mParentNode;
+    nsCOMPtr<nsIAccessible> mParentAccessible;

to nsIFrame when it is an nsAccessible and already has a nsCOMPtr<nsIAccessible> mParent? Perhaps the one in nsAccessible should be called mParentAcc[essible]
The declaraion of this has a different arg name:
GetSiblings(nsIDOMNode *aOneOfTheSiblings)

decl uses aParent -- confusing, use aOneOfTheSibilings everywhere, much better

pre and post conditions on the state of the mState variable might be helpful as comments for the treewalker stuff, since you set the index and list and stuff in so many different places

looks like it is possible for us to get a non-accessible node from GetSibling() -- need to talk with you about this method in general -- whoops, never mind about the non-acc, but still want to talk about this one.

Add some braces and/or newlines to this:
do ++count;  // This loop always iterates at least once
    while (NS_SUCCEEDED(GetNextSibling()));


Ok, that completes everything but the non-treewalker code in nsAccessible. I'll finish up in the am.

Comment 5

[Aaron Leventhal]

John,

> Looks like you moved the test in nsAccessibilityService:: GetAccessibleFor
down in the order. 
> Why? I thought we decided it was best to take care of that right away, 
> since we don't need to do anything else.
You're right, I must have been smoking something. I originally thought this
helped solve the problem. Excuses, excuses ... 

> Needs a newline at the end of nsGenericAccessible.cpp
Done.

> In nsIFrameRootAcc, why not just call Init() when we create the object rather
than have init calls in ever method?
I tried that, it causes an infinite loop to occur, because of the
frame->GetAccessibleFor() - the constructor keeps getting called and filling up
the stack.

> Why add the following:  + nsCOMPtr<nsIDOMNode> mParentNode; + 
nsCOMPtr<nsIAccessible> mParentAccessible;  to  ...
Okay, I've changed the name to mOuterAccessible and mOuterNode.
I'm not comfortable with reusing mParent there since it's a different concept. 
Here "outer" relates to the nsHTMLIFrameAccessible, as opposed to the
nsHTMLIFrameRootAccessible which is "inner".
The outer node is a <browser> or <iframe> tag, the inner node is the inner
document root.
I don't think mParent needs to be renamed in the context of an IAccessible, it's
clear that it points to another IAccessible.

> Add some braces and/or newlines to this:
> do ++count;  // This loop always iterates at least once
>    while (NS_SUCCEEDED(GetNextSibling()));
Done.

> decl uses aParent -- confusing, use aOneOfTheSibilings everywhere, much better
Fixed.

> Pre and post conditions on the state of the mState variable might be helpful
as comments 
> for the treewalker stuff, since you set the index and list and stuff in so
many different places
Not sure what you have in mind here, we'll have to discuss.

Comment 6

[Aaron Leventhal]

Attachment: Changes for John

Comment 7

[John Gaunt (redfive)]

Comment on attachment 55077 [details] [diff] [review]
Tested - works! (See next attachment for easy-to-read version of new nsAccessibleTreeWalker class)

thanks for taking care of those first points.

round 2

Cache optimizations, why check if there is a parent, but not the others before assigning?

are you sure this line:
*aAccParent = mParent = walker.mState.accessible;
    NS_ADDREF(*aAccParent);

does the right thing with respect to the comptr addreffing and stuff.

Other than these nits, looks good to me.

Comment 8

[John Gaunt (redfive)]

Comment on attachment 55171 [details] [diff] [review]
Changes for John

r=jgaunt

Comment 9

[Chris Waterson]

Use |struct WalkState|. This is C++, you don't have to typedef.

+typedef struct _WalkState {

nsAccessibleTreeWalker should have an MOZ_COUNT_[CTOR|DTOR] macros in its ctor &
dtor, as should WalkState (albeit, debug only ctors for this struct).

Put those in, and make sure you don't leak.

Heh: you really want to be sure that mState.prevState is null, I guess:

+  mState.prevState = nsnull;
+  mState.domNode = aNode;
+  mState.siblingIndex = aSiblingIndex;
+  mState.prevState = nsnull;


Fix those, and sr=waterson. 

Comment 10

[Aaron Leventhal]

-> yay!

Comment 11

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381