Bug 72100 – Accessibility: new iframe content may not be reflected in ax tree

• Component: Accessibility
• Status: RESOLVED FIXED

[Dominic Mazzoni]

When an iframe loads new content after it already has an AccessibilityObject, the child of the iframe will still point to the old scroll area's AccessibilityObject, which will be invalid, rather than the new scroll area's AccessibilityObject.

This is a regression caused by https://bugs.webkit.org/show_bug.cgi?id=68636. Prior to 68636, the whole ax tree was cleared when any iframe loaded new content. After 68636, the tree isn't cleared when only an iframe as opposed to the top document reloads. However, if the user has already explored the iframe and then it loads new content, there's now an invalid tree. I'll send a patch that correctly updates an iframe's AccessibilityObject when new content is loaded.

[Dominic Mazzoni]

Created attachment 114645 [details]
Patch

[chris fleizach]

Comment on attachment 114645 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114645&action=review

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:441
> +

i'm not sure this will be true on webkit1. on wk1 the web area's parent should be an attachment

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3422
>          

I think we could remove this isAccessibilityRenderObject restriction and postNotification can take an axObject instead. that will reduce the code in this method and you won't need to add your comment

> Source/WebCore/accessibility/AccessibilityRenderObject.h:207
>      

since this method is in AXObject.h now as public, it does not need to be public in AXRenderObject

> Source/WebCore/accessibility/AccessibilityScrollView.cpp:183
> +

what's up with this change?

> Source/WebCore/dom/Document.cpp:2232
>      }

this change is not reflected in the layout test. it should be in a different bug

[Dominic Mazzoni]

Comment on attachment 114645 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114645&action=review

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:441
>> +
> 
> i'm not sure this will be true on webkit1. on wk1 the web area's parent should be an attachment

This is copied directly from parentObject except with get() instead of getOrCreate(), so I'd be really surprised if it breaks anything.

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3422
>>          
> 
> I think we could remove this isAccessibilityRenderObject restriction and postNotification can take an axObject instead. that will reduce the code in this method and you won't need to add your comment

Good idea! That does simplify the code a lot, I even pulled the postNotification out of the loop.

>> Source/WebCore/accessibility/AccessibilityRenderObject.h:207
>>      
> 
> since this method is in AXObject.h now as public, it does not need to be public in AXRenderObject

Done.

>> Source/WebCore/accessibility/AccessibilityScrollView.cpp:183
>> +
> 
> what's up with this change?

It was returning the grandparent, not the parent. An iframe in a document might look something like this:

GroupRole id=10
  UnknownRole id=11  <-- iframe tag
    ScrollAreaRole id=12
      WebAreaRole id=13

Calling parent() on the AccessibilityScrollView (id=12) was returning the parent of the iframe, in this case id=10. My change modifies it to return the renderer of the owner element itself (id=11 in this example). This is necessary for this patch because otherwise setNeedsToUpdateChildren won't get called on the iframe when childrenChanged is walking up the parent chain from the web area.

>> Source/WebCore/dom/Document.cpp:2232
>>      }
> 
> this change is not reflected in the layout test. it should be in a different bug

You're right, I'll pull it into a separate patch.

[Dominic Mazzoni]

Created attachment 114726 [details]
Patch

[chris fleizach]

Comment on attachment 114726 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114726&action=review

> Source/WebCore/accessibility/AccessibilityScrollView.h:73
> +    mutable bool m_childrenDirty;

this shouldn't have to be mutable anymore  i think because setNeedsToUpdateChildren() is not const. same probably goes for the ivar in the AX render object as well

> Source/WebCore/accessibility/AccessibilityScrollView.h:-85
> -

is this removing the newline at the end of the file?

[Dominic Mazzoni]

Comment on attachment 114726 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114726&action=review

>> Source/WebCore/accessibility/AccessibilityScrollView.h:73
>> +    mutable bool m_childrenDirty;
> 
> this shouldn't have to be mutable anymore  i think because setNeedsToUpdateChildren() is not const. same probably goes for the ivar in the AX render object as well

Done.

>> Source/WebCore/accessibility/AccessibilityScrollView.h:-85
>> -
> 
> is this removing the newline at the end of the file?

Sorry - my editor did that automatically. I fixed it manually, will try to avoid that.

[Dominic Mazzoni]

Created attachment 114739 [details]
Patch

[Dominic Mazzoni]

Created attachment 114747 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 114747 [details]
Patch for landing

Clearing flags on attachment: 114747

Committed r100057: <http://trac.webkit.org/changeset/100057>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.