| Summary: | Include the FrameView widget of a RenderWidget in the accessibility tree. | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Guillory <ctguil> | ||||||||||||||||||||||
| Component: | Accessibility | Assignee: | Chris Guillory <ctguil> | ||||||||||||||||||||||
| Status: | RESOLVED WORKSFORME | ||||||||||||||||||||||||
| Severity: | Normal | CC: | abarth, cfleizach, commit-queue, dglazkov, eric, fishd, webkit.review.bot, xan.lopez | ||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||
| Bug Depends on: | 49994, 50499 | ||||||||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
|
Description
Chris Guillory
2010-11-05 15:52:33 PDT
Created attachment 73135 [details]
Proposed patch.
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.
what did the AX hiearchy look like, and what does it look like after this patch 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.
(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. Interesting, I'll look into doing something similar. 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?
Hey Dimitri, could you take a peek at WebKitTools/DumpRenderTree/chromium. Comment on attachment 73803 [details]
Proposed patch.
DRT changes are good.
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 (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? Created attachment 73921 [details]
Proposed patch.
WEBKIT API addition. 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 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! > 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. Created attachment 74289 [details]
Proposed patch.
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. Created attachment 74427 [details]
Added early return statements in AccessibilityRenderObject::addRenderWidgetChildren
Comment on attachment 74427 [details]
Added early return statements in AccessibilityRenderObject::addRenderWidgetChildren
r=me
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 Created attachment 74586 [details]
Fix Mac Build
Using early return in AccessibilityRenderObject::renderParentObject.
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. Comment on attachment 74586 [details] Fix Mac Build Clearing flags on attachment: 74586 Committed r72628: <http://trac.webkit.org/changeset/72628> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/72628 might have broken GTK Linux 64-bit Debug 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. Patch was reverted due to build failure. http://trac.webkit.org/changeset/72628 Created attachment 75393 [details]
Patch
I ran the layout tests locally and didn't hit the crash issues. Not sure what the issue is.
can you get a crash log from the crashing server 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 yea it looks like if the tests aren't run to completion it doesn't collect crash logs unfortunately Comment on attachment 75393 [details]
Patch
yea it seems unlikely that this change caused tons of crashes. lets try again
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 abarth@webkit.org, ddkilzer@webkit.org, hayato@chromium.org, inferno@chromium.org, and svillar@igalia.com. The commit-queue is continuing to process your patch. Comment on attachment 75393 [details] Patch Clearing flags on attachment: 75393 Committed r73302: <http://trac.webkit.org/changeset/73302> All reviewed patches have been landed. Closing bug. chris, does this fix also address https://bugs.webkit.org/show_bug.cgi?id=49379 Yes. 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 (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 (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. (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 changing resolved status back to unconfirmed Created attachment 75746 [details]
Not passing 0 to RenderWidget::find
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.
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); Created attachment 76485 [details]
Patch
- Using return RenderWidget::find(frameView) now.
- Added the iframe-has-document.html layout test back to the change.
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 can you look this over when you get a chance? (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? That seems to resolve the original issue in Chromium. Thanks! |