RESOLVED FIXED 156328
AX: [ATK] Crash getting text under element in CSS table 
https://bugs.webkit.org/show_bug.cgi?id=156328
Summary AX: [ATK] Crash getting text under element in CSS table
Joanmarie Diggs
Reported 2016-04-06 19:12:45 PDT
If you view the following content and try to read the text with Orca, the web process crashes. <!DOCTYPE html> <html lang="en-US"> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> <style> [class*="site"]:after { content: ""; display: table; } </style> </head> <body> <div class="site-info">foo</div> </body> </html>
Attachments
Patch (6.93 KB, patch)
2016-04-06 19:33 PDT, Joanmarie Diggs 		no flags
DetailsFormatted DiffDiff
Patch (7.99 KB, patch)
2016-04-07 05:43 PDT, Joanmarie Diggs 		no flags
DetailsFormatted DiffDiff
Patch (9.25 KB, patch)
2016-04-08 07:29 PDT, Joanmarie Diggs 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2016-04-06 19:13:04 PDT
<rdar://problem/25592627>
Joanmarie Diggs
Comment 2 2016-04-06 19:33:11 PDT
Created attachment 275849 [details] Patch
Joanmarie Diggs
Comment 3 2016-04-06 20:02:15 PDT
Comment on attachment 275849 [details] Patch Chris: This is another small patch which should be a quick review. Thanks!
chris fleizach
Comment 4 2016-04-06 20:53:51 PDT
Comment on attachment 275849 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275849&action=review > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:655 > + // The work around below for anonymous blocks assumes and asserts that the first and last child Work around -> workaround > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:658 > + if (shouldIncludeAllChildren && m_renderer->isAnonymous() && m_renderer->isTablePart()) Let's put this logic in a helper function. I can see us needing to expand this for other cases in the future
Joanmarie Diggs
Comment 5 2016-04-07 05:43:41 PDT
Created attachment 275886 [details] Patch
Joanmarie Diggs
Comment 6 2016-04-07 06:07:39 PDT
Comment on attachment 275886 [details] Patch (In reply to comment #4) > Work around -> workaround Fixed. > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:658 > > + if (shouldIncludeAllChildren && m_renderer->isAnonymous() && m_renderer->isTablePart()) > > Let's put this logic in a helper function. I can see us needing to expand > this for other cases in the future Done. Re-asking for review so we don't wind up with a function name we don't like. (I went with isAnonymousTablePart().)
chris fleizach
Comment 7 2016-04-07 07:08:15 PDT
Comment on attachment 275886 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275886&action=review > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:730 > +bool AccessibilityRenderObject::isAnonymousTablePart() const I would actually go with a generic helper name function Like ignoresRendererWhenRetrievingTexr And then the table part is one case that is in that method (the only one for now)
Joanmarie Diggs
Comment 8 2016-04-07 07:31:43 PDT
(In reply to comment #7) > Comment on attachment 275886 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=275886&action=review > > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:730 > > +bool AccessibilityRenderObject::isAnonymousTablePart() const > > I would actually go with a generic helper name function > Like > ignoresRendererWhenRetrievingTexr Ohhhhhh. I'm glad I asked. :) > And then the table part is one case that is in that method (the only one for > now) Follow-up question: If ignoresRendererWhenRetrievingText() returns true, do you envision that the next step will always be to descend the render tree to locate the first RenderObject we do not want to ignore when retrieving text? If so, then perhaps the helper function should do that instead. Please let me know what you think. Thanks!
chris fleizach
Comment 9 2016-04-07 08:45:58 PDT
(In reply to comment #8) > (In reply to comment #7) > > Comment on attachment 275886 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=275886&action=review > > > > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:730 > > > +bool AccessibilityRenderObject::isAnonymousTablePart() const > > > > I would actually go with a generic helper name function > > Like > > ignoresRendererWhenRetrievingTexr > > Ohhhhhh. I'm glad I asked. :) > > > And then the table part is one case that is in that method (the only one for > > now) > > Follow-up question: If ignoresRendererWhenRetrievingText() returns true, do > you envision that the next step will always be to descend the render tree to > locate the first RenderObject we do not want to ignore when retrieving text? > If so, then perhaps the helper function should do that instead. Based on your current patch, I don't think we would want that. We want to rely on Node traversal to get this text in this case (and maybe other others). So maybe the name should be something else that's more appropriate to what you want to accomplish > > Please let me know what you think. Thanks!
Joanmarie Diggs
Comment 10 2016-04-08 07:29:03 PDT
Created attachment 276005 [details] Patch
Joanmarie Diggs
Comment 11 2016-04-08 08:10:20 PDT
Comment on attachment 276005 [details] Patch (In reply to comment #9) > Based on your current patch, I don't think we would want that. We want to > rely on Node traversal to get this text in this case (and maybe other > others). So maybe the name should be something else that's more appropriate > to what you want to accomplish Regarding others: Yes, CSS-generated text. I moved those checks to the new function. How does shouldGetTextFromNode() grab you?
WebKit Commit Bot
Comment 12 2016-04-08 09:36:01 PDT
Comment on attachment 276005 [details] Patch Clearing flags on attachment: 276005 Committed r199229: <http://trac.webkit.org/changeset/199229>
WebKit Commit Bot
Comment 13 2016-04-08 09:36:05 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.