Handle aria-owns and presentational children in AX name calc.

Handle aria-owns and presentational children in AX name calc.

Also renames rawFirstSibling to rawNextSibling, I think this was
an accidental typo in a previous patch.

BUG=561766
Committed: https://crrev.com/be57268fb5b41f8f0c20a9d06f7725a16f7c356e
Cr-Commit-Position: refs/heads/master@{#365215}

[dmazzoni, 2015-11-25 23:49:15]

dmazzoni@chromium.org changed reviewers:
+ aboxhall@chromium.org

[dmazzoni, 2015-11-25 23:49:15]

I think we should do this, but the const_cast is unfortunate.

I spent way too long trying to figure out ways around it. We need to figure out
exactly what, if anything, is mutable and what const means.

Virtually anything you query on an AXObject might walk parents and children. It
might create parents and children.

By that argument, maybe name() and friends really shouldn't be const. If the
right way to compute the name of an element involves walking its parents and
children, we should make that more explicit.

On the other hand, there are a small number of cases where we get callbacks into
AX code in the middle of something dangerous, like layout. In those cases we
need to be careful not to create any new objects. It might be nice for those to
be const. There wouldn't be very many const functions but they'd be the few that
are safe to call from Source/core.

Thoughts?

[aboxhall, 2015-11-30 19:58:38]

On 2015/11/25 23:49:15, dmazzoni wrote:
> I think we should do this, but the const_cast is unfortunate.
> 
> I spent way too long trying to figure out ways around it. We need to figure
out
> exactly what, if anything, is mutable and what const means.
> 
> Virtually anything you query on an AXObject might walk parents and children.
It
> might create parents and children.
> 
> By that argument, maybe name() and friends really shouldn't be const. If the
> right way to compute the name of an element involves walking its parents and
> children, we should make that more explicit.
> 
> On the other hand, there are a small number of cases where we get callbacks
into
> AX code in the middle of something dangerous, like layout. In those cases we
> need to be careful not to create any new objects. It might be nice for those
to
> be const. There wouldn't be very many const functions but they'd be the few
that
> are safe to call from Source/core.
> 
> Thoughts?

Yeah, it seems like the most sensible solution is to make things non-const which
aren't truly const. It looks like these things are mostly called from non-const
contexts anyway. I'd be interested to see what breaks if we cascade
non-constness out from this point.

[dmazzoni, 2015-12-14 09:01:45]

Description was changed from

==========
Consider all children in AX name calc.

BUG=561766
==========

to

==========
Handle aria-owns and presentational children in AX name calc.

BUG=561766
==========

[dmazzoni, 2015-12-14 09:07:41]

My original patch introduced other test failures because textFromDescendants
often needs to descend into children that would normally be excluded from the
accessibility tree, so using children() doesn't make sense.

I fixed the patch to just address the two issues directly instead - now it
handles role=presentation and aria-owns when computing textFromDescendants.

[dmazzoni, 2015-12-14 09:08:13]

Description was changed from

==========
Handle aria-owns and presentational children in AX name calc.

BUG=561766
==========

to

==========
Handle aria-owns and presentational children in AX name calc.

Also renames rawFirstSibling to rawNextSibling, I think this was
an accidental typo in a previous patch.

BUG=561766
==========

[aboxhall, 2015-12-14 22:40:37]

lgtm with suggestions

https://codereview.chromium.org/1477043003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/1477043003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:1592: String
AXNodeObject::textFromDescendants(AXObjectSet& visited, bool recursive) const
Could 'recursive' be renamed to something like 'inRecursiveCall'? 'recursive'
seems to have too many potential meanings...

https://codereview.chromium.org/1477043003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:1604: for
(AXObject* obj = rawFirstChild(); obj; obj = obj->rawNextSibling()) {
Where else do we conflate "native" and aria-owned children? Do we need to add a
note to keep them in sync; or, alternatively, should we make a method to iterate
over the combined set of children?

[dmazzoni, 2015-12-14 22:51:06]

https://codereview.chromium.org/1477043003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/1477043003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:1592: String
AXNodeObject::textFromDescendants(AXObjectSet& visited, bool recursive) const
On 2015/12/14 22:40:37, aboxhall wrote:
> Could 'recursive' be renamed to something like 'inRecursiveCall'? 'recursive'
> seems to have too many potential meanings...

This is the same as textAlternative so based on our chat let's leave it for now
or change both later.

https://codereview.chromium.org/1477043003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:1604: for
(AXObject* obj = rawFirstChild(); obj; obj = obj->rawNextSibling()) {
On 2015/12/14 22:40:37, aboxhall wrote:
> Where else do we conflate "native" and aria-owned children? Do we need to add
a
> note to keep them in sync; or, alternatively, should we make a method to
iterate
> over the combined set of children?

Great question, I just searched the code and there's no other use of
rawFirstChild that doesn't handle aria-owns already if it should. The one
exception is dead code which I'm fixing in another change I just mailed out.

Basically 90% of the time we should just call children(), which does the right
thing.

In the few cases where we need to explore the tree with fewer side effects, like
here, we need to take aria-owns into account. It's only a couple of extra lines
of code.

[dmazzoni, 2015-12-14 22:51:17]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2015-12-14 22:51:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1477043003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1477043003/20001

[commit-bot: I haz the power, 2015-12-15 04:26:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-15 04:26:40]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)

[dmazzoni, 2015-12-15 06:30:08]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2015-12-15 06:30:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1477043003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1477043003/20001

[commit-bot: I haz the power, 2015-12-15 06:43:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-15 06:43:48]

Failed to apply patch for
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:
While running git apply --index -3 -p1;
  error: patch failed:
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:1474
  Falling back to three-way merge...
  Applied patch to
'third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp' with
conflicts.
  U third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp

Patch:       third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp
Index: third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp
diff --git a/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp
b/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp
index
96bdd2579a833fc65c58ac72478832d680bde08a..36f47d6b5b19b3972c90f28efe693be5ea44ff9c
100644
--- a/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp
+++ b/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp
@@ -1474,7 +1474,7 @@ static bool isInSameNonInlineBlockFlow(LayoutObject* r1,
LayoutObject* r2)
 
 AXObject* AXNodeObject::findChildWithTagName(const HTMLQualifiedName& tagName)
const
 {
-    for (AXObject* child = rawFirstChild(); child; child =
child->rawFirstSibling()) {
+    for (AXObject* child = rawFirstChild(); child; child =
child->rawNextSibling()) {
         Node* childNode = child->node();
         if (childNode && childNode->hasTagName(tagName))
             return child;
@@ -1543,7 +1543,7 @@ String AXNodeObject::textAlternative(bool recursive, bool
inAriaLabelledByTraver
         else if (isHTMLBRElement(node))
             textAlternative = String("\n");
         else
-            textAlternative = textFromDescendants(visited);
+            textAlternative = textFromDescendants(visited, false);
 
         if (!textAlternative.isEmpty()) {
             if (nameSources) {
@@ -1589,11 +1589,26 @@ String AXNodeObject::textAlternative(bool recursive,
bool inAriaLabelledByTraver
     return String();
 }
 
-String AXNodeObject::textFromDescendants(AXObjectSet& visited) const
+String AXNodeObject::textFromDescendants(AXObjectSet& visited, bool recursive)
const
 {
+    if (!canHaveChildren() && recursive)
+        return String();
+
     StringBuilder accumulatedText;
     AXObject* previous = nullptr;
-    for (AXObject* child = rawFirstChild(); child; child =
child->rawFirstSibling()) {
+
+    AXObjectVector children;
+
+    HeapVector<Member<AXObject>> ownedChildren;
+    computeAriaOwnsChildren(ownedChildren);
+    for (AXObject* obj = rawFirstChild(); obj; obj = obj->rawNextSibling()) {
+        if (!axObjectCache().isAriaOwned(obj))
+            children.append(obj);
+    }
+    for (const auto& ownedChild : ownedChildren)
+        children.append(ownedChild);
+
+    for (AXObject* child : children) {
         // Skip hidden children
         if (child->isInertOrAriaHidden())
             continue;
@@ -1608,7 +1623,11 @@ String AXNodeObject::textFromDescendants(AXObjectSet&
visited) const
                 accumulatedText.append(' ');
         }
 
-        String result = recursiveTextAlternative(*child, false, visited);
+        String result;
+        if (child->isPresentational())
+            result = child->textFromDescendants(visited, true);
+        else
+            result = recursiveTextAlternative(*child, false, visited);
         accumulatedText.append(result);
         previous = child;
     }
@@ -1745,7 +1764,7 @@ AXObject* AXNodeObject::rawFirstChild() const
     return axObjectCache().getOrCreate(firstChild);
 }
 
-AXObject* AXNodeObject::rawFirstSibling() const
+AXObject* AXNodeObject::rawNextSibling() const
 {
     if (!node())
         return 0;
@@ -2081,7 +2100,7 @@ void AXNodeObject::updateAccessibilityRole()
         childrenChanged();
 }
 
-void AXNodeObject::computeAriaOwnsChildren(HeapVector<Member<AXObject>>&
ownedChildren)
+void AXNodeObject::computeAriaOwnsChildren(HeapVector<Member<AXObject>>&
ownedChildren) const
 {
     if (!hasAttribute(aria_ownsAttr))
         return;
@@ -2606,7 +2625,7 @@ String AXNodeObject::description(AXNameFrom nameFrom,
AXDescriptionFrom& descrip
         }
 
         AXObjectSet visited;
-        description = textFromDescendants(visited);
+        description = textFromDescendants(visited, false);
 
         if (!description.isEmpty()) {
             if (descriptionSources) {

[commit-bot: I haz the power, 2015-12-15 06:44:32]

The CQ bit was unchecked by commit-bot@chromium.org

[dmazzoni, 2015-12-15 07:30:54]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-12-15 07:30:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
aboxhall@chromium.org
Link to the patchset: https://codereview.chromium.org/1477043003/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2015-12-15 07:33:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1477043003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1477043003/40001

[commit-bot: I haz the power, 2015-12-15 09:43:38]

Description was changed from

==========
Handle aria-owns and presentational children in AX name calc.

Also renames rawFirstSibling to rawNextSibling, I think this was
an accidental typo in a previous patch.

BUG=561766
==========

to

==========
Handle aria-owns and presentational children in AX name calc.

Also renames rawFirstSibling to rawNextSibling, I think this was
an accidental typo in a previous patch.

BUG=561766
==========

[commit-bot: I haz the power, 2015-12-15 09:43:39]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-15 09:44:44]

Description was changed from

==========
Handle aria-owns and presentational children in AX name calc.

Also renames rawFirstSibling to rawNextSibling, I think this was
an accidental typo in a previous patch.

BUG=561766
==========

to

==========
Handle aria-owns and presentational children in AX name calc.

Also renames rawFirstSibling to rawNextSibling, I think this was
an accidental typo in a previous patch.

BUG=561766

Committed: https://crrev.com/be57268fb5b41f8f0c20a9d06f7725a16f7c356e
Cr-Commit-Position: refs/heads/master@{#365215}
==========

[commit-bot: I haz the power, 2015-12-15 09:44:45]

Patchset 3 (id:??) landed as
https://crrev.com/be57268fb5b41f8f0c20a9d06f7725a16f7c356e
Cr-Commit-Position: refs/heads/master@{#365215}