WebKit Bugzilla
New
Browse
Search+
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
[patch]
Patch proposal
0001-ATK-Avoid-explicit-traversal-of-text-controls-and-th.patch (text/plain), 20.95 KB, created by
Mario Sanchez Prada
on 2013-10-30 07:28:20 PDT
(
hide
)
Description:
Patch proposal
Filename:
MIME Type:
Creator:
Mario Sanchez Prada
Created:
2013-10-30 07:28:20 PDT
Size:
20.95 KB
patch
obsolete
>From 849f9f1735e16dfb5d1a20e38f0bd94b2eaf282c Mon Sep 17 00:00:00 2001 >From: Mario Sanchez Prada <mario.prada@samsung.com> >Date: Wed, 30 Oct 2013 13:19:24 +0000 >Subject: [PATCH] [ATK] Avoid explicit traversal of text controls and the > render tree in AtkText implementation > https://bugs.webkit.org/show_bug.cgi?id=123153 > >--- > LayoutTests/ChangeLog | 24 +++++ > .../accessibility/table-with-rules-expected.txt | 2 +- > .../deleting-iframe-destroys-axcache-expected.txt | 2 +- > .../accessibility/table-with-rules-expected.txt | 2 +- > LayoutTests/platform/gtk/TestExpectations | 2 - > .../deleting-iframe-destroys-axcache-expected.txt | 2 +- > ...placed-objects-in-anonymous-blocks-expected.txt | 8 +- > .../accessibility/table-with-rules-expected.txt | 2 +- > Source/WebCore/ChangeLog | 51 ++++++++++ > .../accessibility/AccessibilityRenderObject.cpp | 36 ++++++- > .../atk/WebKitAccessibleInterfaceText.cpp | 105 -------------------- > Source/WebCore/editing/TextIterator.cpp | 3 + > Source/WebCore/editing/TextIterator.h | 2 +- > 13 files changed, 119 insertions(+), 122 deletions(-) > >diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog >index 5acbae4..eefa934 100644 >--- a/LayoutTests/ChangeLog >+++ b/LayoutTests/ChangeLog >@@ -1,3 +1,27 @@ >+2013-10-30 Mario Sanchez Prada <mario.prada@samsung.com> >+ >+ [ATK] Avoid explicit traversal of text controls and the render tree in AtkText implementation >+ https://bugs.webkit.org/show_bug.cgi?id=123153 >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ Updated test expectations to properly reflect the new reality when it >+ comes to exposing replaced objects and anymous blocks. >+ >+ * platform/gtk/accessibility/table-with-rules-expected.txt: Updated to >+ print <\n> explicitly for the two instances of <BR> that are present in the >+ test, that will be included as part of an anonymous block. >+ * platform/efl/accessibility/table-with-rules-expected.txt: Ditto. >+ * platform/efl-wk2/accessibility/table-with-rules-expected.txt: Ditto. >+ >+ * platform/efl/accessibility/deleting-iframe-destroys-axcache-expected.txt: Updated >+ expectations not to expect the text of a button to be shown. >+ * platform/gtk/accessibility/deleting-iframe-destroys-axcache-expected.txt: Ditto. >+ * platform/gtk/accessibility/replaced-objects-in-anonymous-blocks-expected.txt: Ditto. >+ >+ * platform/gtk/TestExpectations: Removed replaced-objects-in-anonymous-blocks.html >+ from the list of expected failures, as it's now being properly exposed. >+ > 2013-10-28 Mario Sanchez Prada <mario.prada@samsung.com> > > [GTK] Expose title and alternative text for links in image maps >diff --git a/LayoutTests/platform/efl-wk2/accessibility/table-with-rules-expected.txt b/LayoutTests/platform/efl-wk2/accessibility/table-with-rules-expected.txt >index 323865e..9e58b03 100644 >--- a/LayoutTests/platform/efl-wk2/accessibility/table-with-rules-expected.txt >+++ b/LayoutTests/platform/efl-wk2/accessibility/table-with-rules-expected.txt >@@ -84,7 +84,7 @@ AXPosition: { 0.000000, 0.000000 } > AXSize: { 769.000000, 36.000000 } > AXTitle: > AXDescription: >-AXValue: ------------------------------------ >+AXValue: <\n>------------------------------------<\n> > AXFocusable: 0 > AXFocused: 0 > AXSelectable: 0 >diff --git a/LayoutTests/platform/efl/accessibility/deleting-iframe-destroys-axcache-expected.txt b/LayoutTests/platform/efl/accessibility/deleting-iframe-destroys-axcache-expected.txt >index 0fee235..0c846d5 100644 >--- a/LayoutTests/platform/efl/accessibility/deleting-iframe-destroys-axcache-expected.txt >+++ b/LayoutTests/platform/efl/accessibility/deleting-iframe-destroys-axcache-expected.txt >@@ -17,7 +17,7 @@ AXRole: AXScrollArea > AXRole: AXGroup AXValue: <obj> > AXRole: AXScrollArea > AXRole: AXWebArea >- AXRole: AXGroup AXValue: <obj>Click me >+ AXRole: AXGroup AXValue: <obj> > AXRole: AXButton > AXRole: AXParagraph AXValue: After > AXRole: AXParagraph AXValue: End of test >diff --git a/LayoutTests/platform/efl/accessibility/table-with-rules-expected.txt b/LayoutTests/platform/efl/accessibility/table-with-rules-expected.txt >index 84c504f..196afa3 100644 >--- a/LayoutTests/platform/efl/accessibility/table-with-rules-expected.txt >+++ b/LayoutTests/platform/efl/accessibility/table-with-rules-expected.txt >@@ -84,7 +84,7 @@ AXPosition: { 8.000000, 116.000000 } > AXSize: { 765.000000, 36.000000 } > AXTitle: > AXDescription: >-AXValue: ------------------------------------ >+AXValue: <\n>------------------------------------<\n> > AXFocusable: 0 > AXFocused: 0 > AXSelectable: 0 >diff --git a/LayoutTests/platform/gtk/TestExpectations b/LayoutTests/platform/gtk/TestExpectations >index e818860..caa0cbb 100644 >--- a/LayoutTests/platform/gtk/TestExpectations >+++ b/LayoutTests/platform/gtk/TestExpectations >@@ -1321,8 +1321,6 @@ webkit.org/b/114251 accessibility/svg-group-element-with-title.html [ Failure ] > webkit.org/b/114259 platform/gtk/accessibility/aria-slider-required-attributes.html [ Failure ] > webkit.org/b/114259 platform/gtk/accessibility/combo-box-collapsed-selection-changed.html [ Failure ] > >-webkit.org/b/118577 platform/gtk/accessibility/replaced-objects-in-anonymous-blocks.html [ Failure ] >- > webkit.org/b/114612 editing/style/block-style-005.html [ Failure ] > > webkit.org/b/115025 fast/events/constructors/mouse-event-constructor.html [ Failure ] >diff --git a/LayoutTests/platform/gtk/accessibility/deleting-iframe-destroys-axcache-expected.txt b/LayoutTests/platform/gtk/accessibility/deleting-iframe-destroys-axcache-expected.txt >index 0fee235..0c846d5 100644 >--- a/LayoutTests/platform/gtk/accessibility/deleting-iframe-destroys-axcache-expected.txt >+++ b/LayoutTests/platform/gtk/accessibility/deleting-iframe-destroys-axcache-expected.txt >@@ -17,7 +17,7 @@ AXRole: AXScrollArea > AXRole: AXGroup AXValue: <obj> > AXRole: AXScrollArea > AXRole: AXWebArea >- AXRole: AXGroup AXValue: <obj>Click me >+ AXRole: AXGroup AXValue: <obj> > AXRole: AXButton > AXRole: AXParagraph AXValue: After > AXRole: AXParagraph AXValue: End of test >diff --git a/LayoutTests/platform/gtk/accessibility/replaced-objects-in-anonymous-blocks-expected.txt b/LayoutTests/platform/gtk/accessibility/replaced-objects-in-anonymous-blocks-expected.txt >index 8cae580..0eff2d3 100644 >--- a/LayoutTests/platform/gtk/accessibility/replaced-objects-in-anonymous-blocks-expected.txt >+++ b/LayoutTests/platform/gtk/accessibility/replaced-objects-in-anonymous-blocks-expected.txt >@@ -12,16 +12,16 @@ button > End of test > AXRole: AXWebArea > AXRole: AXParagraph AXValue: Paragraph >- AXRole: AXGroup AXValue: <obj>button<\n> >+ AXRole: AXGroup AXValue: <obj> > AXRole: AXButton > AXRole: AXGroup AXValue: <obj> > AXRole: AXScrollArea > AXRole: AXWebArea >- AXRole: AXGroup AXValue: <obj>text area<\n> >+ AXRole: AXGroup AXValue: <obj> > AXRole: AXTextField AXValue: text area >- AXRole: AXDiv AXValue: Div<\n><obj>button >+ AXRole: AXDiv AXValue: Div<\n><obj> > AXRole: AXButton >- AXRole: AXHeading AXValue: Heading<\n><obj>button<\n><obj> >+ AXRole: AXHeading AXValue: Heading<\n><obj><\n><obj> > AXRole: AXButton > AXRole: AXImage > AXRole: AXDiv AXValue: End of test >diff --git a/LayoutTests/platform/gtk/accessibility/table-with-rules-expected.txt b/LayoutTests/platform/gtk/accessibility/table-with-rules-expected.txt >index 1067eda..a1391fe 100644 >--- a/LayoutTests/platform/gtk/accessibility/table-with-rules-expected.txt >+++ b/LayoutTests/platform/gtk/accessibility/table-with-rules-expected.txt >@@ -84,7 +84,7 @@ AXPosition: { 8.000000, 111.000000 } > AXSize: { 769.000000, 34.000000 } > AXTitle: > AXDescription: >-AXValue: ------------------------------------ >+AXValue: <\n>------------------------------------<\n> > AXFocusable: 0 > AXFocused: 0 > AXSelectable: 0 >diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog >index 1285d1e..efd1d5b 100644 >--- a/Source/WebCore/ChangeLog >+++ b/Source/WebCore/ChangeLog >@@ -1,3 +1,54 @@ >+2013-10-30 Mario Sanchez Prada <mario.prada@samsung.com> >+ >+ [ATK] Avoid explicit traversal of text controls and the render tree in AtkText implementation >+ https://bugs.webkit.org/show_bug.cgi?id=123153 >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ Remove functions from the AtkText implementation that manually >+ walk the render tree to compose the text for a exposed objects in >+ certain cases (e.g. anonymous blocks, text controls). >+ >+ The reason for this change is that the current implementation >+ follows an error-prone approach, since by doing things like >+ manually walking the render tree from here we are not properly >+ considering all the possible scenarios that might happen when >+ traversing text. This, however, is a task that is better suited >+ for the TextIterator, which is already written to consider all >+ those cases and to emit the proper character in every single >+ situation: text nodes, replaced objects and so on. >+ >+ So, by removing all that too specific code (textForObject() and >+ textForRenderer() mainly) from WebKitAccessibleInterfaceText.cpp >+ and relying in AccessibilityObject::textUnderElement(), which it >+ ends up using the TextIterator for certain cases, we have a much >+ better and robust method of retrieving the text associated with an >+ instance of AtkObject implementing the AtkText interface. >+ >+ * accessibility/atk/WebKitAccessibleInterfaceText.cpp: >+ (webkitAccessibleTextGetText): Removed call to textForObject(), now that >+ we have just removed that function, together with textForRenderer(). >+ >+ Make AccessibilityRenderObject::textUnderElement() able to deal with >+ anonymous blocks directly, by creating a range based in the boundaries >+ defined by the first and last child renderers for that block. This will >+ make possible to treat an anonymous block as a whole instead of having >+ to rely in the concatenation of each of its children, as it does now. >+ >+ * accessibility/AccessibilityRenderObject.cpp: >+ (WebCore::AccessibilityRenderObject::textUnderElement): Added a new code >+ path to deal with anonymous blocks for text renderers, or when including >+ all the children is explicitly requested. >+ >+ Modified TextIterator so text for children of replaced objects are >+ ignored if we are emmiting the special character for those objects. >+ >+ * editing/TextIterator.cpp: >+ (WebCore::TextIterator::handleReplacedElement): Make sure no children are >+ handled a replaced object if m_emitsObjectReplacementCharacters is set. >+ * editing/TextIterator.cpp: Updated m_emitsObjectReplacementCharacters >+ description to reflect the new behavior. >+ > 2013-10-28 Mario Sanchez Prada <mario.prada@samsung.com> > > [GTK] Expose title and alternative text for links in image maps >diff --git a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp b/Source/WebCore/accessibility/AccessibilityRenderObject.cpp >index 5c044a2..ce174d8 100644 >--- a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp >+++ b/Source/WebCore/accessibility/AccessibilityRenderObject.cpp >@@ -652,13 +652,39 @@ String AccessibilityRenderObject::textUnderElement(AccessibilityTextUnderElement > if (m_renderer->isText() || mode.childrenInclusion == AccessibilityTextUnderElementMode::TextUnderElementModeIncludeAllChildren) { > // If possible, use a text iterator to get the text, so that whitespace > // is handled consistently. >- if (Node* node = this->node()) { >- if (Frame* frame = node->document().frame()) { >+ Document* nodeDocument = 0; >+ RefPtr<Range> textRange; >+ if (m_renderer->node()) { >+ if (Node* node = this->node()) { >+ nodeDocument = &node->document(); >+ textRange = rangeOfContents(*node); >+ } >+ } else { >+ // For anonymous blocks, we work around not having a direct node to create a range from >+ // defining one based in the two external positions defining the boundaries of the subtree. >+ RenderObject* firstChildRenderer = m_renderer->firstChildSlow(); >+ RenderObject* lastChildRenderer = m_renderer->lastChildSlow(); >+ if (firstChildRenderer && lastChildRenderer) { >+ ASSERT(firstChildRenderer->node()); >+ ASSERT(lastChildRenderer->node()); >+ >+ // We define the start and end positions for the range as the ones right before and after >+ // the first and the last nodes in the DOM tree that is wrapped inside the anonymous block. >+ Node* firstNodeInBlock = firstChildRenderer->node(); >+ Position startPosition = positionInParentBeforeNode(firstNodeInBlock); >+ Position endPosition = positionInParentAfterNode(lastChildRenderer->node()); >+ >+ nodeDocument = &firstNodeInBlock->document(); >+ textRange = Range::create(*nodeDocument, startPosition, endPosition); >+ } >+ } >+ >+ if (nodeDocument && textRange) { >+ if (Frame* frame = nodeDocument->frame()) { > // catch stale WebCoreAXObject (see <rdar://problem/3960196>) >- if (frame->document() != &node->document()) >+ if (frame->document() != nodeDocument) > return String(); >- >- return plainText(rangeOfContents(*node).get(), textIteratorBehaviorForTextRange()); >+ return plainText(textRange.get(), textIteratorBehaviorForTextRange()); > } > } > >diff --git a/Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp b/Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp >index a772ae0..b1439aa 100644 >--- a/Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp >+++ b/Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp >@@ -69,103 +69,6 @@ static AccessibilityObject* core(AtkText* text) > return webkitAccessibleGetAccessibilityObject(WEBKIT_ACCESSIBLE(text)); > } > >-static gchar* textForRenderer(RenderObject* renderer) >-{ >- GString* resultText = g_string_new(0); >- >- if (!renderer) >- return g_string_free(resultText, FALSE); >- >- // For RenderBlocks, piece together the text from the RenderText objects they contain. >- for (RenderObject* object = renderer->firstChildSlow(); object; object = object->nextSibling()) { >- if (object->isBR()) { >- g_string_append(resultText, "\n"); >- continue; >- } >- >- RenderText* renderText; >- if (object->isText()) >- renderText = toRenderText(object); >- else { >- // List item's markers will be treated in an special way >- // later on this function, so ignore them here. >- if (object->isReplaced() && !object->isListMarker()) >- g_string_append_unichar(resultText, objectReplacementCharacter); >- >- // We need to check children, if any, to consider when >- // current object is not a text object but some of its >- // children are, in order not to miss those portions of >- // text by not properly handling those situations >- if (object->firstChildSlow()) { >- GOwnPtr<char> objectText(textForRenderer(object)); >- g_string_append(resultText, objectText.get()); >- } >- continue; >- } >- >- InlineTextBox* box = renderText ? renderText->firstTextBox() : 0; >- while (box) { >- // WebCore introduces line breaks in the text that do not reflect >- // the layout you see on the screen, replace them with spaces. >- String text = String(renderText->characters(), renderText->textLength()).replace("\n", " "); >- g_string_append(resultText, text.substring(box->start(), box->end() - box->start() + 1).utf8().data()); >- >- // Newline chars in the source result in separate text boxes, so check >- // before adding a newline in the layout. See bug 25415 comment #78. >- // If the next sibling is a BR, we'll add the newline when we examine that child. >- if (!box->nextOnLineExists() && !(object->nextSibling() && object->nextSibling()->isBR())) { >- // If there was a '\n' in the last position of the >- // current text box, it would have been converted to a >- // space in String::replace(), so remove it first. >- if (renderText->characters()[box->end()] == '\n') >- g_string_erase(resultText, resultText->len - 1, -1); >- >- g_string_append(resultText, "\n"); >- } >- box = box->nextTextBox(); >- } >- } >- >- // Insert the text of the marker for list item in the right place, if present >- if (renderer->isListItem()) { >- String markerText = toRenderListItem(renderer)->markerTextWithSuffix(); >- if (renderer->style()->direction() == LTR) >- g_string_prepend(resultText, markerText.utf8().data()); >- else >- g_string_append(resultText, markerText.utf8().data()); >- } >- >- return g_string_free(resultText, FALSE); >-} >- >-static gchar* textForObject(const AccessibilityObject* coreObject) >-{ >- GString* str = g_string_new(0); >- >- // For text controls, we can get the text line by line. >- if (coreObject->isTextControl()) { >- unsigned textLength = coreObject->textLength(); >- int lineNumber = 0; >- PlainTextRange range = coreObject->doAXRangeForLine(lineNumber); >- while (range.length) { >- String lineText = coreObject->doAXStringForRange(range); >- g_string_append(str, lineText.utf8().data()); >- >- // When a line of text wraps in a text area, the final space >- // after each non-final line must be replaced with a line break. >- if (range.start + range.length < textLength) >- g_string_overwrite_len(str, str->len - 1, "\n", 1); >- >- range = coreObject->doAXRangeForLine(++lineNumber); >- } >- } else if (coreObject->isAccessibilityRenderObject()) { >- GOwnPtr<gchar> rendererText(textForRenderer(coreObject->renderer())); >- g_string_append(str, rendererText.get()); >- } >- >- return g_string_free(str, FALSE); >-} >- > static int baselinePositionForRenderObject(RenderObject* renderObject) > { > // FIXME: This implementation of baselinePosition originates from RenderObject.cpp and was >@@ -588,14 +491,6 @@ static gchar* webkitAccessibleTextGetText(AtkText* text, gint startOffset, gint > ret = coreObject->textUnderElement(AccessibilityTextUnderElementMode(AccessibilityTextUnderElementMode::TextUnderElementModeIncludeAllChildren)); > } > >- if (!ret.length()) { >- // This can happen at least with anonymous RenderBlocks (e.g. body text amongst paragraphs) >- // In such instances, there may also be embedded objects. The object replacement character >- // is something ATs want included and we have to account for the fact that it is multibyte. >- GOwnPtr<char> objectText(textForObject(coreObject)); >- ret = String::fromUTF8(objectText.get()); >- } >- > // Prefix a item number/bullet if needed > int actualEndOffset = endOffset == -1 ? ret.length() : endOffset; > if (coreObject->roleValue() == ListItemRole) { >diff --git a/Source/WebCore/editing/TextIterator.cpp b/Source/WebCore/editing/TextIterator.cpp >index d4d89ed..58b55dd 100644 >--- a/Source/WebCore/editing/TextIterator.cpp >+++ b/Source/WebCore/editing/TextIterator.cpp >@@ -723,6 +723,9 @@ bool TextIterator::handleReplacedElement() > > if (m_emitsObjectReplacementCharacters && renderer && renderer->isReplaced()) { > emitCharacter(objectReplacementCharacter, m_node->parentNode(), m_node, 0, 1); >+ // Don't process subtrees for embedded objects. If the text there is required, >+ // it must be explicitly asked by specifying a range falling inside its boundaries. >+ m_handledChildren = true; > return true; > } > >diff --git a/Source/WebCore/editing/TextIterator.h b/Source/WebCore/editing/TextIterator.h >index b9e8310..12b4524 100644 >--- a/Source/WebCore/editing/TextIterator.h >+++ b/Source/WebCore/editing/TextIterator.h >@@ -187,7 +187,7 @@ private: > bool m_handledFirstLetter; > // Used when the visibility of the style should not affect text gathering. > bool m_ignoresStyleVisibility; >- // Used when emitting the special 0xFFFC character is required. >+ // Used when emitting the special 0xFFFC character is required. Children for replaced objects will be ignored. > bool m_emitsObjectReplacementCharacters; > // Used when the iteration should stop if form controls are reached. > bool m_stopsOnFormControls; >-- >1.7.10.4 >
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=215500">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 123153
:
214850
|
215500
|
215501