Summary: | [ATK] Avoid explicit traversal of text controls and the render tree in AtkText implementation | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mario Sanchez Prada <mario> | ||||||||
Component: | Accessibility | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | apinheiro, enrica, jdiggs, k.czech, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Mario Sanchez Prada
2013-10-22 05:42:27 PDT
*** 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> |