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...
<rdar://problem/15287471>
*** Bug 118577 has been marked as a duplicate of this bug. ***
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.
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?
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())
Created attachment 215500 [details] Patch proposal
Created attachment 215501 [details] Patch proposal Sorry. Wrong patch attached before.
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
Committed r158428: <http://trac.webkit.org/changeset/158428>