Attachment 340380 Details for Bug 185521 – patch for landing

[patch] patch for landing

bug-185521-20180514200428.patch (text/plain), 16.73 KB, created by Joanmarie Diggs on 2018-05-14 17:04:29 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Joanmarie Diggs

Created: 2018-05-14 17:04:29 PDT

Size: 16.73 KB

patch

obsolete

>Subversion Revision: 231777
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 01d58a32f829b8c00667e125ab0846e7cd30aace..a388e842101abd42b3973f99d99c74169f6dbe99 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,33 @@
>+2018-05-14  Joanmarie Diggs  <jdiggs@igalia.com>
>+
>+        AX: Listbox and Combobox roles embedded in labels should participate in name calculation
>+        https://bugs.webkit.org/show_bug.cgi?id=185521
>+
>+        Reviewed by Chris Fleizach.
>+
>+        Take selected children into account when computing the name in accessibleNameForNode.
>+        Add ListBox to the roles for which accessibleNameDerivesFromContent returns false so
>+        that native select elements with size > 1 are treated the same way as ARIA listbox.
>+        Also add ListBox to the roles which are treated as controls when used in ARIA. Finally,
>+        prevent labels which contain unrelated controls from being used as an AXTitleUIElement.
>+        This causes us to build a string from the label and its descendants, ensuring the latter
>+        participate in the name calculation.
>+
>+        Test: accessibility/text-alternative-calculation-from-listbox.html
>+
>+        * accessibility/AccessibilityLabel.cpp:
>+        (WebCore::childrenContainUnrelatedControls):
>+        (WebCore::AccessibilityLabel::containsUnrelatedControls const):
>+        * accessibility/AccessibilityLabel.h:
>+        * accessibility/AccessibilityNodeObject.cpp:
>+        (WebCore::accessibleNameForNode):
>+        * accessibility/AccessibilityObject.cpp:
>+        (WebCore::AccessibilityObject::accessibleNameDerivesFromContent const):
>+        (WebCore::AccessibilityObject::isARIAControl):
>+        * accessibility/AccessibilityRenderObject.cpp:
>+        (WebCore::AccessibilityRenderObject::exposesTitleUIElement const):
>+        (WebCore::AccessibilityRenderObject::computeAccessibilityIsIgnored const):
>+
> 2018-05-14  Antoine Quint  <graouts@apple.com>
> 
>         [Web Animations] Tests using the new animation engine may crash under WebCore::FrameView::didDestroyRenderTree when using internals methods
>diff --git a/Source/WebCore/accessibility/AccessibilityLabel.cpp b/Source/WebCore/accessibility/AccessibilityLabel.cpp
>index 7f548d14026dba8b710fb83060c19c80e7dd2abf..b45fc60024a4f30ab007556eb56b349f23422f58 100644
>--- a/Source/WebCore/accessibility/AccessibilityLabel.cpp
>+++ b/Source/WebCore/accessibility/AccessibilityLabel.cpp
>@@ -83,6 +83,33 @@ bool AccessibilityLabel::containsOnlyStaticText() const
>     return m_containsOnlyStaticText;
> }
> 
>+static bool childrenContainUnrelatedControls(const AccessibilityObject::AccessibilityChildrenVector& children, AccessibilityObject* controlForLabel)
>+{
>+    if (!children.size())
>+        return false;
>+
>+    for (const auto& child : children) {
>+        if (child->isControl()) {
>+            if (child == controlForLabel)
>+                continue;
>+            return true;
>+        }
>+
>+        if (childrenContainUnrelatedControls(child->children(), controlForLabel))
>+            return true;
>+    }
>+
>+    return false;
>+}
>+
>+bool AccessibilityLabel::containsUnrelatedControls() const
>+{
>+    if (containsOnlyStaticText())
>+        return false;
>+
>+    return childrenContainUnrelatedControls(m_children, correspondingControlForLabelElement());
>+}
>+
> void AccessibilityLabel::updateChildrenIfNecessary()
> {
>     AccessibilityRenderObject::updateChildrenIfNecessary();
>diff --git a/Source/WebCore/accessibility/AccessibilityLabel.h b/Source/WebCore/accessibility/AccessibilityLabel.h
>index 5056b335722665803bed92afa3a9e7786530ea08..b13af2d7d92dbb5280f4dccd821400d466c394e0 100644
>--- a/Source/WebCore/accessibility/AccessibilityLabel.h
>+++ b/Source/WebCore/accessibility/AccessibilityLabel.h
>@@ -38,6 +38,7 @@ public:
>     virtual ~AccessibilityLabel();
>     
>     bool containsOnlyStaticText() const;
>+    bool containsUnrelatedControls() const;
> 
> private:
>     explicit AccessibilityLabel(RenderObject*);
>diff --git a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp b/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
>index 4ad50050362fb38ddc1e4bf54c258af8bc302697..12dce85fd7e6d69a7be745cd51aa5b03c89f390c 100644
>--- a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
>+++ b/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
>@@ -1952,6 +1952,29 @@ static String accessibleNameForNode(Node* node, Node* labelledbyNode)
>         String valueDescription = axObject->valueDescription();
>         if (!valueDescription.isEmpty())
>             return valueDescription;
>+
>+        // The Accname specification states that if the name is being calculated for a combobox
>+        // or listbox inside a labeling element, return the text alternative of the chosen option.
>+        AccessibilityObject::AccessibilityChildrenVector children;
>+        if (axObject->isListBox())
>+            axObject->selectedChildren(children);
>+        else if (axObject->isComboBox()) {
>+            for (const auto& child : axObject->children()) {
>+                if (child->isListBox()) {
>+                    child->selectedChildren(children);
>+                    break;
>+                }
>+            }
>+        }
>+
>+        StringBuilder builder;
>+        String childText;
>+        for (const auto& child : children)
>+            appendNameToStringBuilder(builder, accessibleNameForNode(child->node()));
>+
>+        childText = builder.toString();
>+        if (!childText.isEmpty())
>+            return childText;
>     }
>     
>     if (is<HTMLInputElement>(*node))
>diff --git a/Source/WebCore/accessibility/AccessibilityObject.cpp b/Source/WebCore/accessibility/AccessibilityObject.cpp
>index f29c88696941d97964351516930da4def4edfff0..6f2e1983192a92971b36388a701add3a5eacd224 100644
>--- a/Source/WebCore/accessibility/AccessibilityObject.cpp
>+++ b/Source/WebCore/accessibility/AccessibilityObject.cpp
>@@ -354,6 +354,7 @@ bool AccessibilityObject::accessibleNameDerivesFromContent() const
>     // Now check for generically derived elements now that we know the element does not match a specific ARIA role.
>     switch (roleValue()) {
>     case AccessibilityRole::Slider:
>+    case AccessibilityRole::ListBox:
>         return false;
>     default:
>         break;
>@@ -905,7 +906,7 @@ bool AccessibilityObject::isARIAInput(AccessibilityRole ariaRole)
>     
> bool AccessibilityObject::isARIAControl(AccessibilityRole ariaRole)
> {
>-    return isARIAInput(ariaRole) || ariaRole == AccessibilityRole::TextArea || ariaRole == AccessibilityRole::Button || ariaRole == AccessibilityRole::ComboBox || ariaRole == AccessibilityRole::Slider;
>+    return isARIAInput(ariaRole) || ariaRole == AccessibilityRole::TextArea || ariaRole == AccessibilityRole::Button || ariaRole == AccessibilityRole::ComboBox || ariaRole == AccessibilityRole::Slider || ariaRole == AccessibilityRole::ListBox;
> }
>     
> bool AccessibilityObject::isRangeControl() const
>diff --git a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp b/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
>index e6049eec6aaf67f6c5b31e8350538a6e888c6c4e..fabd616c903eef74c6b0b2838a8af059e4867e4d 100644
>--- a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
>+++ b/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
>@@ -31,6 +31,7 @@
> 
> #include "AXObjectCache.h"
> #include "AccessibilityImageMapLink.h"
>+#include "AccessibilityLabel.h"
> #include "AccessibilityListBox.h"
> #include "AccessibilitySVGRoot.h"
> #include "AccessibilitySpinButton.h"
>@@ -1064,6 +1065,11 @@ bool AccessibilityRenderObject::exposesTitleUIElement() const
>             if (AccessibilityObject* labelObject = axObjectCache()->getOrCreate(label)) {
>                 if (!labelObject->ariaLabeledByAttribute().isEmpty())
>                     return false;
>+                // To simplify instances where the labeling element includes widget descendants
>+                // which it does not label.
>+                if (is<AccessibilityLabel>(*labelObject)
>+                    && downcast<AccessibilityLabel>(*labelObject).containsUnrelatedControls())
>+                    return false;
>             }
>         }
>     }
>@@ -1204,12 +1210,6 @@ bool AccessibilityRenderObject::computeAccessibilityIsIgnored() const
>     if (m_renderer && ancestorsOfType<RenderMenuList>(*m_renderer).first())
>         return true;
> 
>-    // find out if this element is inside of a label element.
>-    // if so, it may be ignored because it's the label for a checkbox or radio button
>-    AccessibilityObject* controlObject = correspondingControlForLabelElement();
>-    if (controlObject && !controlObject->exposesTitleUIElement() && controlObject->isCheckboxOrRadio())
>-        return true;
>-    
>     // https://webkit.org/b/161276 Getting the controlObject might cause the m_renderer to be nullptr.
>     if (!m_renderer)
>         return true;
>@@ -1416,6 +1416,12 @@ bool AccessibilityRenderObject::computeAccessibilityIsIgnored() const
>     if (m_renderer->isRubyRun() || m_renderer->isRubyBlock() || m_renderer->isRubyInline())
>         return false;
> 
>+    // Find out if this element is inside of a label element.
>+    // If so, it may be ignored because it's the label for a checkbox or radio button.
>+    AccessibilityObject* controlObject = correspondingControlForLabelElement();
>+    if (controlObject && !controlObject->exposesTitleUIElement() && controlObject->isCheckboxOrRadio())
>+        return true;
>+
>     // By default, objects should be ignored so that the AX hierarchy is not
>     // filled with unnecessary items.
>     return true;
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index f70692b46f8ddae024ceaf8ace3257af3097852c..0007cbaa7977e8d73070dc8c99fabfdbcbb36d6f 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,16 @@
>+2018-05-14  Joanmarie Diggs  <jdiggs@igalia.com>
>+
>+        AX: Listbox and Combobox roles embedded in labels should participate in name calculation
>+        https://bugs.webkit.org/show_bug.cgi?id=185521
>+
>+        Reviewed by Chris Fleizach.
>+
>+        * accessibility/label-with-pseudo-elements-expected.txt: Updated for new behavior.
>+        * accessibility/text-alternative-calculation-from-listbox-expected.txt: Added.
>+        * accessibility/text-alternative-calculation-from-listbox.html: Added.
>+        * platform/mac/accessibility/label-with-pseudo-elements-expected.txt: Updated for new behavior.
>+        * platform/win/accessibility/label-with-pseudo-elements-expected.txt: Updated for new behavior.
>+
> 2018-05-14  Antoine Quint  <graouts@apple.com>
> 
>         [Web Animations] Tests using the new animation engine may crash under WebCore::FrameView::didDestroyRenderTree when using internals methods
>diff --git a/LayoutTests/accessibility/label-with-pseudo-elements-expected.txt b/LayoutTests/accessibility/label-with-pseudo-elements-expected.txt
>index b938f3ddcb279d295ee1f6edfcbf82c099a05a4d..3ca445b4b781f48df8bf304e62506d696c0e60e4 100644
>--- a/LayoutTests/accessibility/label-with-pseudo-elements-expected.txt
>+++ b/LayoutTests/accessibility/label-with-pseudo-elements-expected.txt
>@@ -24,13 +24,13 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE
> 	AXDescription: 
> 	AXTitleUIElement: null
> 
>-	AXTitle: 
>+	AXTitle: test 5 input value
> 	AXDescription: 
>-	AXTitleUIElement: non-null
>+	AXTitleUIElement: null
> 
>-	AXTitle: 
>+	AXTitle: before test 6 input value after
> 	AXDescription: 
>-	AXTitleUIElement: non-null
>+	AXTitleUIElement: null
> 
> PASS successfullyParsed is true
> 
>diff --git a/LayoutTests/accessibility/text-alternative-calculation-from-listbox-expected.txt b/LayoutTests/accessibility/text-alternative-calculation-from-listbox-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..2ee3fbe05886e8ce6c04fb2068e8c4ccb92419ec
>--- /dev/null
>+++ b/LayoutTests/accessibility/text-alternative-calculation-from-listbox-expected.txt
>@@ -0,0 +1,13 @@
>+This tests text alternative calculation from label with an embedded listbox.
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+
>+Test 1: AXTitle: Flash the screen 2 times.
>+Test 2: AXTitle: Flash the screen 2 times.
>+Test 3: AXTitle: Flash the screen 2 times.
>+Test 4: AXTitle: Flash the screen 2 times.
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>diff --git a/LayoutTests/accessibility/text-alternative-calculation-from-listbox.html b/LayoutTests/accessibility/text-alternative-calculation-from-listbox.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..1a9d72bf6e7807cad08935acbbc1bf8392785927
>--- /dev/null
>+++ b/LayoutTests/accessibility/text-alternative-calculation-from-listbox.html
>@@ -0,0 +1,81 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<script src="../resources/js-test-pre.js"></script>
>+<script src="../resources/accessibility-helper.js"></script>
>+</head>
>+<body>
>+<div id="content">
>+  <div>
>+    <input type="checkbox" id="test1" />
>+    <label for="test1" id="label1">Flash the screen
>+      <ul role="listbox" style="list-style-type: none;">
>+	<li role="option" aria-selected="false">1</li>
>+	<li role="option" aria-selected="true">2</li>
>+	<li role="option">3</li>
>+      </ul>
>+      times.
>+    </label>
>+  </div>
>+  <div>
>+    <input type="checkbox" id="test2" />
>+    <label for="test2" id="label2">Flash the screen
>+      <div role="combobox" id="combobox">
>+	<div role="textbox" id="textbox"></div>
>+	<ul role="listbox" id="listbox" style="list-style-type: none;">
>+	  <li role="option" id="option1" aria-selected="false">1</li>
>+	  <li role="option" id="option2" aria-selected="true">2</li>
>+	  <li role="option" id="option3">3</li>
>+	</ul>
>+      </div>
>+      times.
>+    </label>
>+  </div>
>+  <div>
>+    <input type="checkbox" id="test3" />
>+    <label for="test3" id="label3">Flash the screen
>+      <select size="1">
>+	<option>1</option>
>+	<option selected>2</option>
>+	<option>3</option>
>+      </select>
>+      times.
>+    </label>
>+  </div>
>+  <div>
>+    <input type="checkbox" id="test4" />
>+    <label for="test4" id="label4">Flash the screen
>+      <select size="3">
>+	<option>1</option>
>+	<option selected>2</option>
>+	<option>3</option>
>+      </select>
>+      times.
>+    </label>
>+  </div>
>+</div>
>+<p id="description"></p>
>+<div id="console"></div>
>+
>+<script>
>+    description("This tests text alternative calculation from label with an embedded listbox.");
>+    if (window.accessibilityController) {
>+        for (var i = 1; i <= 4; i++) {
>+            // Touching the accessibility tree prevents test flakiness in macOS.
>+            // In particular, when this file is viewed with Accessibility Inspector,
>+            // the AXTitle is displayed as expected for all four tests. But when we
>+            // run this test as a layout test without first touching the accessibility
>+            // tree, the AXTitle is often the empty string for most if not all tests.
>+            var axLabel = accessibilityController.accessibleElementById("label" + i);
>+            touchAccessibilityTree(axLabel);
>+
>+            var axElement = accessibilityController.accessibleElementById("test" + i);
>+            debug("Test " + i + ": " +  axElement.title);
>+        }
>+        document.getElementById("content").style.visibility = "hidden";
>+    }
>+</script>
>+
>+<script src="../resources/js-test-post.js"></script>
>+</body>
>+</html>
>diff --git a/LayoutTests/platform/mac/accessibility/label-with-pseudo-elements-expected.txt b/LayoutTests/platform/mac/accessibility/label-with-pseudo-elements-expected.txt
>index 119f25e2718b42ead63616d54548da1953eec4a6..f2ac0e5596664c5ca8f8458f85ca7c2a4c645314 100644
>--- a/LayoutTests/platform/mac/accessibility/label-with-pseudo-elements-expected.txt
>+++ b/LayoutTests/platform/mac/accessibility/label-with-pseudo-elements-expected.txt
>@@ -28,15 +28,15 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE
> 	AXHelp: 
> 	AXTitleUIElement: null
> 
>-	AXTitle: 
>+	AXTitle: test 5 input value
> 	AXDescription: 
> 	AXHelp: 
>-	AXTitleUIElement: non-null
>+	AXTitleUIElement: null
> 
>-	AXTitle: 
>+	AXTitle: before test 6 input value after
> 	AXDescription: 
> 	AXHelp: 
>-	AXTitleUIElement: non-null
>+	AXTitleUIElement: null
> 
> PASS successfullyParsed is true
> 
>diff --git a/LayoutTests/platform/win/accessibility/label-with-pseudo-elements-expected.txt b/LayoutTests/platform/win/accessibility/label-with-pseudo-elements-expected.txt
>index b938f3ddcb279d295ee1f6edfcbf82c099a05a4d..3ca445b4b781f48df8bf304e62506d696c0e60e4 100644
>--- a/LayoutTests/platform/win/accessibility/label-with-pseudo-elements-expected.txt
>+++ b/LayoutTests/platform/win/accessibility/label-with-pseudo-elements-expected.txt
>@@ -24,13 +24,13 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE
> 	AXDescription: 
> 	AXTitleUIElement: null
> 
>-	AXTitle: 
>+	AXTitle: test 5 input value
> 	AXDescription: 
>-	AXTitleUIElement: non-null
>+	AXTitleUIElement: null
> 
>-	AXTitle: 
>+	AXTitle: before test 6 input value after
> 	AXDescription: 
>-	AXTitleUIElement: non-null
>+	AXTitleUIElement: null
> 
> PASS successfullyParsed is true
>

Actions: View | Formatted Diff | Diff

Attachments on bug 185521: 340115 | 340125 | 340192 | 340193 | 340196 | 340211 | 340212 | 340368 | 340380