Bug 156328

Summary:

AX: [ATK] Crash getting text under element in CSS table

Product:

WebKit

Reporter:

Joanmarie Diggs <jdiggs>

Component:

Accessibility

Assignee:

Joanmarie Diggs <jdiggs>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aboxhall, apinheiro, cfleizach, commit-queue, dmazzoni, jcraig, mario, mcatanzaro, samuel_white, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

See Also:

https://bugzilla.redhat.com/show_bug.cgi?id=1324625

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

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	Details Formatted Diff Diff
Patch (7.99 KB, patch) 2016-04-07 05:43 PDT, Joanmarie Diggs	no flags	Details Formatted Diff Diff
Patch (9.25 KB, patch) 2016-04-08 07:29 PDT, Joanmarie Diggs	no flags	Details Formatted Diff Diff
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.