Bug 49106 – Include the FrameView widget of a RenderWidget in the accessibility tree.

• Status: RESOLVED
• Resolution: WORKSFORME
• Component: Accessibility

[Chris Guillory]

During investigation of a Chromium bug I noticed that the RenderView of an iFrame is not included in the top document AX tree hierarchy.
http://code.google.com/p/chromium/issues/detail?id=57727

Perhaps we can include the RenderView of the iFrame in the main AX tree similar to Firefox.

[Chris Guillory]

Created attachment 73135 [details]
Proposed patch.

[WebKit Review Bot]

Attachment 73135 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/accessibility/iframe-has-document-expected.txt', u'LayoutTests/accessibility/iframe-has-document.html', u'LayoutTests/platform/win/Skipped', u'WebCore/ChangeLog', u'WebCore/accessibility/AccessibilityRenderObject.cpp', u'WebKitTools/ChangeLog', u'WebKitTools/DumpRenderTree/AccessibilityUIElement.h', u'WebKitTools/DumpRenderTree/win/AccessibilityUIElementWin.cpp']" exit_code: 1
WebCore/accessibility/AccessibilityRenderObject.cpp:3375:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebKitTools/DumpRenderTree/win/AccessibilityUIElementWin.cpp:62:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

what did the AX hiearchy look like, and what does it look like after this patch

[Chris Guillory]

Created attachment 73138 [details]
Proposed patch.

Before this patch the AX tree of the top document would not include any children for a RenderIFrame render object (firstChild() doesn't return any renderer children).

After this patch the AX tree of the top document will include the RenderView of a RenderIFrame as a child. The RenderView document of the iFrame already includes all of that document's children. Also calling AccessibilityRenderObject::parentObject() on the RenderView document of an iFrame will return the RenderIFrame of the parent document; previously calling parentObject() repeatedly on a sub-document wouldn't walk all the way up to the top document.

[chris fleizach]

(In reply to comment #4)
> Created an attachment (id=73138) [details]
> Proposed patch.
> 
> Before this patch the AX tree of the top document would not include any children for a RenderIFrame render object (firstChild() doesn't return any renderer children).
> 
> After this patch the AX tree of the top document will include the RenderView of a RenderIFrame as a child. The RenderView document of the iFrame already includes all of that document's children. Also calling AccessibilityRenderObject::parentObject() on the RenderView document of an iFrame will return the RenderIFrame of the parent document; previously calling parentObject() repeatedly on a sub-document wouldn't walk all the way up to the top document.

I'm worried that this will affect the AX hierarchy in undesired ways. Right now on the mac side, we see if the object has an attachment, then pass the call onto the attachment (you'll see that in AXObjectWrapperMac.mm). I suspect you'd want to do similar things on windows.

[Chris Guillory]

Interesting, I'll look into doing something similar.

[Chris Guillory]

Created attachment 73803 [details]
Proposed patch.

Seems like the mac logic should be extended for all the ports. In this patch I'm specifically excluding the mac port in the added logic. Perhaps that mac attachment forwarding logic could be removed in favor of something like this?

[Chris Guillory]

Hey Dimitri, could you take a peek at WebKitTools/DumpRenderTree/chromium.

[Dimitri Glazkov (Google)]

Comment on attachment 73803 [details]
Proposed patch.

DRT changes are good.

[chris fleizach]

Comment on attachment 73803 [details]
Proposed patch.

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

> WebCore/accessibility/AccessibilityRenderObject.cpp:416
> +    // On the mac port this parent logic is in AccessibleObjectWrapper.mm.

not the right name of the file.

> WebCore/accessibility/AccessibilityRenderObject.cpp:419
>      }

one line else if shouldn't have { }

> WebCore/accessibility/AccessibilityRenderObject.cpp:1879
> +#endif

shouldn't this go into accessibilityIgnoreAttachment() which can be customized per platform?

> WebCore/accessibility/AccessibilityRenderObject.cpp:3382
> +    // for RenderWidget, add frame's content renderer if the widget is a FramwView.

spelling

> WebCore/accessibility/AccessibilityRenderObject.cpp:3383
> +    // on the mac port this children logic is in AccessibleObjectWrapper.mm.

the file name is AccessibilityObjectWrapper.mm.
All comments should be full sentences

[Chris Guillory]

(In reply to comment #10)
> (From update of attachment 73803 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73803&action=review
> 
> > WebCore/accessibility/AccessibilityRenderObject.cpp:416
> > +    // On the mac port this parent logic is in AccessibleObjectWrapper.mm.
> 
> not the right name of the file.
Fixed.

> 
> > WebCore/accessibility/AccessibilityRenderObject.cpp:419
> >      }
> 
> one line else if shouldn't have { }
> 
> > WebCore/accessibility/AccessibilityRenderObject.cpp:1879
> > +#endif
Fixed.

> shouldn't this go into accessibilityIgnoreAttachment() which can be customized per platform?
Seems like the iFrame renderer could always be ignored. It has an unknown role. For now I've moved this logic into AccessibilityObjectChromium.cpp.

> > WebCore/accessibility/AccessibilityRenderObject.cpp:3382
> > +    // for RenderWidget, add frame's content renderer if the widget is a FramwView.
> 
> spelling
Fixed.

> > WebCore/accessibility/AccessibilityRenderObject.cpp:3383
> > +    // on the mac port this children logic is in AccessibleObjectWrapper.mm.
> 
> the file name is AccessibilityObjectWrapper.mm.
> All comments should be full sentences
Fixed capitalization. Is that what you were referring to?

[Chris Guillory]

Created attachment 73921 [details]
Proposed patch.

[Chris Guillory]

WEBKIT API addition.

[chris fleizach]

Comment on attachment 73921 [details]
Proposed patch.

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

some small changes

can you file a Mac bug asking to revisit why Mac can't use this code (and assign to me, and relate to this)

> WebCore/accessibility/AccessibilityRenderObject.cpp:3383
> +#endif

can you put this in a method like,

appendWidgetChildren or something

> WebCore/accessibility/chromium/AccessibilityObjectChromium.cpp:37
> +    // Ignore iframe renderers that have a content renderer.

can you explain why you're ignoring content renderers here. this comment states what the code does, but not why

[Darin Fisher (:fishd, Google)]

Comment on attachment 73921 [details]
Proposed patch.

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

> WebKit/chromium/public/WebAccessibilityObject.h:66
> +    WEBKIT_API bool equals(const WebAccessibilityObject&) const;

This API change looks good to me.  Thanks!

[Chris Guillory]

> can you file a Mac bug asking to revisit why Mac can't use this code (and assign to me, and relate to this)
https://bugs.webkit.org/show_bug.cgi?id=49760
 
> > WebCore/accessibility/AccessibilityRenderObject.cpp:3383
> > +#endif
> 
> can you put this in a method like,
Added addRenderWidgetChildren

> 
> appendWidgetChildren or something
> 
> > WebCore/accessibility/chromium/AccessibilityObjectChromium.cpp:37
> > +    // Ignore iframe renderers that have a content renderer.
> 
> can you explain why you're ignoring content renderers here. this comment states what the code does, but not why
Updated comment.

[Chris Guillory]

Created attachment 74289 [details]
Proposed patch.

[chris fleizach]

Comment on attachment 74289 [details]
Proposed patch.

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

sorry, chris. one more time

> WebCore/accessibility/AccessibilityRenderObject.cpp:3387
> +    }

now that we have our own method, we should use early return in the 3 levels of if here.

[Chris Guillory]

Created attachment 74427 [details]
Added early return statements in AccessibilityRenderObject::addRenderWidgetChildren

[chris fleizach]

Comment on attachment 74427 [details]
Added early return statements in AccessibilityRenderObject::addRenderWidgetChildren

r=me

[WebKit Commit Bot]

Comment on attachment 74427 [details]
Added early return statements in AccessibilityRenderObject::addRenderWidgetChildren

Rejecting patch 74427 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 2
Last 500 characters of output:
ild/Script-5DF50887116F3077005202AB.sh

** BUILD FAILED **


The following build commands failed:
WebCore:
	CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/AccessibilityRenderObject.o /Projects/CommitQueue/WebCore/accessibility/AccessibilityRenderObject.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
(1 failure)


Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
l x86_64 c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://queues.webkit.org/results/6277014

[Chris Guillory]

Created attachment 74586 [details]
Fix Mac Build

Using early return in AccessibilityRenderObject::renderParentObject.

[Chris Guillory]

Hey Chris, if you're in can you take a look at this one. The previous change failed to commit because of a build error on mac.

[WebKit Commit Bot]

Comment on attachment 74586 [details]
Fix Mac Build

Clearing flags on attachment: 74586

Committed r72628: <http://trac.webkit.org/changeset/72628>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/72628 might have broken GTK Linux 64-bit Debug

[Chris Guillory]

http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/16298
GTK Linux 64-bit Debug: layout-test
editing/execCommand/4128080-1.html -> crashed
.................
editing/execCommand/4976800.html -> crashed
........................
editing/execCommand/5543472-1.html -> crashed
.....
editing/execCommand/5575101-1.html -> crashed
....................
editing/execCommand/break-out-of-empty-list-item.html -> crashed
......................
editing/execCommand/indent-node-to-split-to-crash.html -> crashed
................................................
editing/input ..
editing/inserting ..........................................
editing/inserting/insert-3907422-fix.html -> crashed
...........................
editing/pasteboard ............
editing/pasteboard/5032095.html -> crashed
...............................................................
editing/pasteboard/paste-text-003.html -> crashed
.........
editing/pasteboard/paste-text-events.html -> crashed
....
editing/selection ........................................................................
editing/selection/extend-selection-enclosing-block.html -> crashed
.............
editing/selection/home-end.html -> crashed
.................................................
editing/spelling ..
editing/style ......................
editing/style/inline-style-extend-run.html -> crashed
...
editing/style/push-down-font-styles.html -> crashed
......................
editing/text-iterator .
editing/undo .
editing/undo/4059423-1.html -> crashed
...........
editing/undo/redo-split-text-node.html -> crashed

Look like this change needs to be reverted. I'll have to investigate this failure.

[Chris Guillory]

Patch was reverted due to build failure.
http://trac.webkit.org/changeset/72628

[Chris Guillory]

Created attachment 75393 [details]
Patch

I ran the layout tests locally and didn't hit the crash issues. Not sure what the issue is.

[chris fleizach]

can you get a crash log from the crashing server

[Chris Guillory]

How would I do that? I can see the stdout which shows test cases crashing.
http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/16603/steps/layout-test/logs/stdio

[chris fleizach]

yea it looks like if the tests aren't run to completion it doesn't collect crash logs unfortunately

[chris fleizach]

Comment on attachment 75393 [details]
Patch

yea it seems unlikely that this change caused tons of crashes. lets try again

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 75393 [details]:

http/tests/loading/load-javascript-after-many-xhrs.html
http/tests/security/xss-DENIED-mime-type-execute-as-html.html

Please file bugs against the tests.  These tests were authored by [e-mail], [e-mail], [e-mail], [e-mail], and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 75393 [details]
Patch

Clearing flags on attachment: 75393

Committed r73302: <http://trac.webkit.org/changeset/73302>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[chris fleizach]

chris, does this fix also address
https://bugs.webkit.org/show_bug.cgi?id=49379

[Chris Guillory]

Yes.

[Xan Lopez]

I think this is causing crashes in the debug layout tests in GTK+, hitting an ASSERT. One trace: http://webkit-bots.igalia.com/amd64/svn_73312.core-when_1291419012-_-who_DumpRenderTree-_-why_11.trace.html

[chris fleizach]

(In reply to comment #39)
> I think this is causing crashes in the debug layout tests in GTK+, hitting an ASSERT. One trace: http://webkit-bots.igalia.com/amd64/svn_73312.core-when_1291419012-_-who_DumpRenderTree-_-why_11.trace.html

(In reply to comment #39)
> I think this is causing crashes in the debug layout tests in GTK+, hitting an ASSERT. One trace: http://webkit-bots.igalia.com/amd64/svn_73312.core-when_1291419012-_-who_DumpRenderTree-_-why_11.trace.html

So it looks like you can't pass in 0 to 
RenderWidget::find(frameViewIfRenderView())

and the other thing is that accessibility is always enabled on the GTK test bot, interestingly

[Xan Lopez]

(In reply to comment #40)
> and the other thing is that accessibility is always enabled on the GTK test bot, interestingly

a11y is enabled by default in the GTK+ port.

[chris fleizach]

(In reply to comment #41)
> (In reply to comment #40)
> > and the other thing is that accessibility is always enabled on the GTK test bot, interestingly
> 
> a11y is enabled by default in the GTK+ port.

good idea i think, helps catch more bugs

[chris fleizach]

changing resolved status back to unconfirmed

[Chris Guillory]

Created attachment 75746 [details]
Not passing 0 to RenderWidget::find

[WebKit Review Bot]

Attachment 75746 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'HEAD']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'HEAD'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Comment on attachment 75746 [details]
Not passing 0 to RenderWidget::find

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

> WebCore/accessibility/AccessibilityRenderObject.cpp:420
> +            return renderWidget;

since parent already is nil in this case, it probably doesn't matter if renderWidget is nil, so you could probably just do
if (frameView)
   return RenderWidget::find(frameView);

[Chris Guillory]

Created attachment 76485 [details]
Patch

- Using return RenderWidget::find(frameView) now.
- Added the iframe-has-document.html layout test back to the change.

[Eric Seidel (no email)]

Comment on attachment 74427 [details]
Added early return statements in AccessibilityRenderObject::addRenderWidgetChildren

Cleared Chris Fleizach's review+ from obsolete attachment 74427 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Chris Guillory]

Chris can you look this over when you get a chance?

[chris fleizach]

(In reply to comment #49)
> Chris can you look this over when you get a chance?

I made a bunch of changes to support WebKit2, one of which inserts the ScrollView widget into the AX tree. do you still need this patch in light of that?

[Chris Guillory]

That seems to resolve the original issue in Chromium. Thanks!