RESOLVED FIXED 123153
[ATK] Avoid explicit traversal of text controls and the render tree in AtkText implementation 
https://bugs.webkit.org/show_bug.cgi?id=123153
Summary [ATK] Avoid explicit traversal of text controls and the render tree in AtkTex...
Mario Sanchez Prada
Reported 2013-10-22 05:42:27 PDT
At the moment, the implementation of atk_text_get_text() relies sometimes on code that do things such as manually traversing the render tree, or manually checking line by line for text controls, to compose the text for a given AtkObject. Fortunately, this is only done when certain conditions apply, such as exposing an anonymous block or multiline text controls, but to me is an error-prone approach and most likely not the way to go, since by doing things like this we are not properly considering all the possible scenarios that might happen when traversing text, which is a task better suited for the TextIterator, for instance. Another reason why that code is placed there (and the textForRenderer() function specially) is to deal with the case of exposing replaced objects to AT-SPI/ATK ATs, which again is something that the TextIterator should probably do instead, IMHO. Furthermore, after some conversations with Joanie on IRC, I learned that the way we are currently exposing those replaced objects is not the right one, even if some test expectations seem to say so. For instance, consider the following code: <html><body><p>This is a text with a <button type="button">nice button</button> in the middle</p></body></html> At the moment, the code that we have in WebKitGTK+ exposes the text for that paragraph as this: "This is a text with a <obj>nice button in the middle" ...and this is coherent with what the test expectations for layout tests checking the exposure of replaced objects claim to expect (e.g. deleting-iframe-destroys-axcache). However, the expected output for Orca would better be something like this: "This is a text with a <obj> in the middle" --> Orca can query on that <obj> to know what's in, if she wants So, this means that the current code, besides being error-prone, is wrong anyway. Great :) Fortunately, after doing some experiments locally, I believe there's a better way to do all this, and it is by removing all that code (textForObject() and textForRenderer() mainly) from WebKitAccessibleInterfaceText.cpp and make sure the same functionality is achieved by using the AccessibilityObject API (which we are already using anyway) + slightly modifying the TextIterator to allow it not walk the subtree under a replaced object when asked for it (presumably, only EFL and GTK will want that). I plan to upload such a patch soon to this bug. Hope it will make WebKitGTK/EFL a11y support of AtkText better, without disturbing the Mac port at all. Let's see...
Attachments
Patch proposal (26.40 KB, patch)
2013-10-22 06:23 PDT, Mario Sanchez Prada 		no flags
DetailsFormatted DiffDiff
Patch proposal (20.95 KB, patch)
2013-10-30 07:28 PDT, Mario Sanchez Prada 		no flags
DetailsFormatted DiffDiff
Patch proposal (20.95 KB, patch)
2013-10-30 07:29 PDT, Mario Sanchez Prada 		cfleizach: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2013-10-22 05:43:12 PDT
<rdar://problem/15287471>
Mario Sanchez Prada
Comment 2 2013-10-22 06:21:28 PDT
*** Bug 118577 has been marked as a duplicate of this bug. ***
Mario Sanchez Prada
Comment 3 2013-10-22 06:23:05 PDT
Created attachment 214850 [details] Patch proposal Here is the patch. I'll be adding Enrica to CC for an extra pair of eyes on the changes proposed for TextIterator.
chris fleizach
Comment 4 2013-10-29 09:50:40 PDT
Comment on attachment 214850 [details] Patch proposal View in context: https://bugs.webkit.org/attachment.cgi?id=214850&action=review > Source/WebCore/ChangeLog:8 > + Remove functions from the AtkText implementation that manually walk the can you add why you want to do this. its not clear what the purpose is > Source/WebCore/accessibility/AccessibilityObject.cpp:1976 > + | TextIteratorIgnoresChildrenForReplacedObjects); is there ever a case where you don't want to do these two things together? > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:659 > + if (!m_renderer->isAnonymous()) { can this check be done by just checking if this->node() == 0?
Mario Sanchez Prada
Comment 5 2013-10-30 04:03:24 PDT
Comment on attachment 214850 [details] Patch proposal View in context: https://bugs.webkit.org/attachment.cgi?id=214850&action=review >> Source/WebCore/ChangeLog:8 >> + Remove functions from the AtkText implementation that manually walk the > > can you add why you want to do this. its not clear what the purpose is Sure. I will expand the description in the ChangeLog with more detail, more in the line of this bug's description. >> Source/WebCore/accessibility/AccessibilityObject.cpp:1976 >> + | TextIteratorIgnoresChildrenForReplacedObjects); > > is there ever a case where you don't want to do these two things together? Not for ATK. However, I added this extra option because I assume other port might be using the TextIteratorEmitsObjectReplacementCharacters value, since it was there already before I added this bit some time ago. In any case, that does not seem to keep being the case, since a quick grep tells me nobody other than this function is actually requiring that behaviour: $ git grep TextIteratorEmitsObjectReplacementCharacters WebCore/ChangeLog-2011-06-04: enumeration (TextIteratorEmitsObjectReplacementCharacters) and new WebCore/accessibility/AccessibilityObject.cpp: behavior = static_cast<TextIteratorBehavior>(behavior | TextIteratorEmitsObjectReplacementCharacters); WebCore/editing/TextIterator.cpp: , m_emitsObjectReplacementCharacters(behavior & TextIteratorEmitsObjectReplacementCharacters) WebCore/editing/TextIterator.h: TextIteratorEmitsObjectReplacementCharacters = 1 << 4, Thus, I think it makes sense to merge them in two, probably by keeping the old name and just expanding its effect so it ignores the children when enabled. I don't know... maybe this was used in the past by ports such as chromium or Qt? >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:659 >> + if (!m_renderer->isAnonymous()) { > > can this check be done by just checking if this->node() == 0? Probably, actually node() includes the isAnonymous() check anyway: Node* RenderObject::node() const { return isAnonymous() ? 0 : &m_node; } I'll change that, if you think it's better. > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:671 > + ASSERT(!firstChildRenderer->isAnonymous()); > + ASSERT(!lastChildRenderer->isAnonymous()); I'll change this asserts too to check that ASSERT({first|last}ChildRenderer->node())
Mario Sanchez Prada
Comment 6 2013-10-30 07:28:20 PDT
Created attachment 215500 [details] Patch proposal
Mario Sanchez Prada
Comment 7 2013-10-30 07:29:48 PDT
Created attachment 215501 [details] Patch proposal Sorry. Wrong patch attached before.
chris fleizach
Comment 8 2013-10-30 09:13:40 PDT
Comment on attachment 215501 [details] Patch proposal View in context: https://bugs.webkit.org/attachment.cgi?id=215501&action=review minor comment. I think it's OK to collapse those for now. Mac may eventually want to use the emitsObjectReplacement and I think we'd also want the same children behavior GTK wants > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:657 > + if (m_renderer->node()) { you can probably do a if (Node* node = m_renderer->node()) and then get rid of the next if check. I know sometimes those results might be different but I'm pretty sure AXrenderObject's node() method just does m_renderer->node() anyway
Mario Sanchez Prada
Comment 9 2013-11-01 03:57:51 PDT
Committed r158428: <http://trac.webkit.org/changeset/158428>
Note You need to log in before you can comment on or make changes to this bug.