| 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: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| See Also: | https://bugzilla.redhat.com/show_bug.cgi?id=1324625 | ||||||||||
| Attachments: |
|
||||||||||
Created attachment 275849 [details]
Patch
Comment on attachment 275849 [details]
Patch
Chris: This is another small patch which should be a quick review. Thanks!
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 Created attachment 275886 [details]
Patch
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().) 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) (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! (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! Created attachment 276005 [details]
Patch
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? Comment on attachment 276005 [details] Patch Clearing flags on attachment: 276005 Committed r199229: <http://trac.webkit.org/changeset/199229> All reviewed patches have been landed. Closing bug. |
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>