Ignores ARIA relations (e.g. aria-labelledby) that point to an invisible target.

Ignores ARIA relations (e.g. aria-labelledby) that point to an invisible target.

BUG=625356
R=dmazzoni@chromium.org, aboxhall@chromium.org
TESTED=layout tests, manually with Jaws

Committed: https://crrev.com/e0989c034eec8d6fcbb55f03a004fec2a32e83d7
Cr-Commit-Position: refs/heads/master@{#404696}

[dmazzoni, 2016-07-05 22:42:54]

This is incorrect in the specific case of aria-labelledby, it's allowed to point
to hidden nodes and we already have quite a few tests for this.

The change is probably correct for aria-flowto and aria-controls.

[blink-reviews, 2016-07-07 15:53:15]

I read the ARIA Implementation Guide 1.1. I changed my implementation to 
remove elements that are the target of aria-controls, aria-owns and 
aria-flowto when they are hidden either via display:none or CSS 
visibility:hidden, unless aria-hidden="false" is specified. I left 
aria-describedby and aria-labelledby alone. In other words, I reused the 
code in the |isHiddenForTextAlternativeCalculation| function.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-07-07 15:53:15]

I read the ARIA Implementation Guide 1.1. I changed my implementation to 
remove elements that are the target of aria-controls, aria-owns and 
aria-flowto when they are hidden either via display:none or CSS 
visibility:hidden, unless aria-hidden="false" is specified. I left 
aria-describedby and aria-labelledby alone. In other words, I reused the 
code in the |isHiddenForTextAlternativeCalculation| function.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[aboxhall, 2016-07-08 17:12:35]

https://codereview.chromium.org/2112243004/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/accessibility/aria-relations-should-ignore-hidden-targets.html
(right):

https://codereview.chromium.org/2112243004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/aria-relations-should-ignore-hidden-targets.html:16:
<div class="container">
Suggestion: put this in between the first and second tests, i.e.
</script>

<div class="container">
...
</div>

<script>
...

https://codereview.chromium.org/2112243004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/aria-relations-should-ignore-hidden-targets.html:31:
panel.style.display = 'none';
Test for visibility: hidden as well? Here and below. Should be trivial.

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

https://codereview.chromium.org/2112243004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:582: if
(!includeHiddenObjects && child->isHiddenForTextAlternativeCalculation())
Why not just call child->accessibilityIsIgnored() here?

https://codereview.chromium.org/2112243004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXNodeObject.h (right):

https://codereview.chromium.org/2112243004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.h:72: void
accessibilityChildrenFromAttribute(QualifiedName attr,
AXObject::AXObjectVector&, bool) const;
Perhaps instead of passing "includeHiddenObjects" in, we could simply switch on
"attr" in the method body? I don't see why the logic for determining this
shouldn't live in the method rather than in the various calling methods.

If you still want to pass it in, it should definitely have a name in the method
signature.

https://codereview.chromium.org/2112243004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/2112243004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:904: if (Element*
idElement = scope.getElementById(AtomicString(id))) {
Something weird happened here, does this file need to be in this CL?

[blink-reviews, 2016-07-08 23:25:43]

All comments addressed.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-07-08 23:25:44]

All comments addressed.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[aboxhall, 2016-07-08 23:34:44]

lgtm

[nektarios, 2016-07-11 16:11:48]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2016-07-11 16:11:48]

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/2112243004/#ps80001
(title: "Fixed test.")

[commit-bot: I haz the power, 2016-07-11 16:12:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-11 16:18:53]

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

[commit-bot: I haz the power, 2016-07-11 16:18:54]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mlamouri (slow - plz ping), 2016-07-11 16:53:00]

mlamouri@chromium.org changed reviewers:
+ mlamouri@chromium.org

[mlamouri (slow - plz ping), 2016-07-11 16:53:00]

modules/ rs-lgtm

[nektarios, 2016-07-11 16:54:00]

The CQ bit was checked by nektar@chromium.org

[commit-bot: I haz the power, 2016-07-11 16:54:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-11 18:35:22]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-11 18:35:37]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-11 18:37:21]

Description was changed from

==========
Ignores ARIA relations (e.g. aria-labelledby) that point to an invisible target.

BUG=625356
R=dmazzoni@chromium.org, aboxhall@chromium.org
TESTED=layout tests, manually with Jaws
==========

to

==========
Ignores ARIA relations (e.g. aria-labelledby) that point to an invisible target.

BUG=625356
R=dmazzoni@chromium.org, aboxhall@chromium.org
TESTED=layout tests, manually with Jaws

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

[commit-bot: I haz the power, 2016-07-11 18:37:22]

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