Enable accessible name of a control to include multiple <label> elements.

Enable accessible name of a control to include multiple <label> elements.

The spec allows a form control element to have multiple <label> elements,
but we only supported one in accessibility code. Fix it by switching to
using the LabelableElement interface rather than some redundant code that
kept track of label elements in TreeScope, which allows us to get rid of
that now-unused code.

BUG=573494
Committed: https://crrev.com/054bdb4ca5858d7b65188f961a52b8b04e94078d
Cr-Commit-Position: refs/heads/master@{#398005}

[dmazzoni, 2016-05-02 21:20:58]

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

[dmazzoni, 2016-05-02 21:20:59]

aboxhall: primary review of modules/accessibility/
dglazkov: owners approval to delete some code in core/

[dglazkov, 2016-05-13 18:07:47]

lgtm

[aboxhall, 2016-05-13 20:19:16]

https://codereview.chromium.org/1939303002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/accessibility/name-calc-native-markup-input-buttons.html
(right):

https://codereview.chromium.org/1939303002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/accessibility/name-calc-native-markup-input-buttons.html:82:
<!--
Commented out?

https://codereview.chromium.org/1939303002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/1939303002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:2475:
result.unite(labelRect);
This doesn't change in this change, but it never occurred to me until now that
this could be weird if there was some content between the labeled element and
its label. Not sure how likely that is to occur in practice.

https://codereview.chromium.org/1939303002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/1939303002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2166: nameFrom
= AXNameFromRelatedElement;
This needs to be set outside this if block, as before. Perhaps it'd be less
confusing if we renamed this variable "potentialNameFrom"?

Also, need to create and append the NameSource here regardless of whether a
label is found, for the same reason. Have a look at how it's done for
aria-labelledby for how to handle multiple relatedObjects.

[dmazzoni, 2016-05-13 22:22:42]

https://codereview.chromium.org/1939303002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/accessibility/name-calc-native-markup-input-buttons.html
(right):

https://codereview.chromium.org/1939303002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/accessibility/name-calc-native-markup-input-buttons.html:82:
<!--
On 2016/05/13 20:19:16, aboxhall wrote:
> Commented out?

Done.

https://codereview.chromium.org/1939303002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/1939303002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:2475:
result.unite(labelRect);
On 2016/05/13 20:19:16, aboxhall wrote:
> This doesn't change in this change, but it never occurred to me until now that
> this could be weird if there was some content between the labeled element and
> its label. Not sure how likely that is to occur in practice.

Agreed, it's a bit weird.

https://codereview.chromium.org/1939303002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/1939303002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2166: nameFrom
= AXNameFromRelatedElement;
On 2016/05/13 20:19:16, aboxhall wrote:
> This needs to be set outside this if block, as before. Perhaps it'd be less
> confusing if we renamed this variable "potentialNameFrom"?
> 
> Also, need to create and append the NameSource here regardless of whether a
> label is found, for the same reason. Have a look at how it's done for
> aria-labelledby for how to handle multiple relatedObjects.

OK, done. I was thinking it was okay to skip it if there weren't any label
elements, but we want to show the full possible name calculation either way.

I also moved *foundTextAlternative = true up so that it only sets it to true if
at least one label is actually found, just to be safe.

[dmazzoni, 2016-05-13 22:25:53]

Sorry about the delay. Moved to ToolbarProgressBar.

[aboxhall, 2016-05-16 17:19:01]

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

https://codereview.chromium.org/1939303002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2187: if
(relatedObjects) {
Now that I've had a closer look, this is weird. Setting `*relatedObjects =
localRelatedObjects` should only happen once, not once per loop.

I'm wondering if we should split this out into a separate method, and combine it
somehow with the aria-labelledby logic, since it seems like they are closely
related. The main difference seems to be that this checks
`visited.contains(axObject)` here, whereas the aria-labelledby logic checks it
during the recursive step
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).

[dmazzoni, 2016-06-01 19:53:23]

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

https://codereview.chromium.org/1939303002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2187: if
(relatedObjects) {
On 2016/05/16 17:19:01, aboxhall wrote:
> Now that I've had a closer look, this is weird. Setting `*relatedObjects =
> localRelatedObjects` should only happen once, not once per loop.
> 
> I'm wondering if we should split this out into a separate method, and combine
it
> somehow with the aria-labelledby logic, since it seems like they are closely
> related. The main difference seems to be that this checks
> `visited.contains(axObject)` here, whereas the aria-labelledby logic checks it
> during the recursive step
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).


OK, ready for another look. I had this call AXObject::textFromElements
just like textFromAriaLabelledby does, so now they share most of the
same logic.

[aboxhall, 2016-06-03 21:19:44]

https://codereview.chromium.org/1939303002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector-protocol/accessibility/accessibility-nameSources-input-expected.txt
(right):

https://codereview.chromium.org/1939303002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector-protocol/accessibility/accessibility-nameSources-input-expected.txt:788:
"nativeSource": "labelwrapped",
Hah, this is a fun edge case for listing a single "source".

https://codereview.chromium.org/1939303002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector-protocol/accessibility/accessibility-nameSources-input-expected.txt:906:
"value": "label-wrapping-text10 "
Why the extra space here, do you know?

https://codereview.chromium.org/1939303002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector-protocol/accessibility/accessibility-nameSources-labelledby-expected.txt
(right):

https://codereview.chromium.org/1939303002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector-protocol/accessibility/accessibility-nameSources-labelledby-expected.txt:212:
"type": "contents",
Hmmm something weird has happened here. Is this a consequence of the change to
recursiveTextAlternative?

Yeah, looks like it, because recursiveTextAlternative is called at
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/access...

Ugh. Seems like we need to make a copy of |visited|.

[dmazzoni, 2016-06-03 22:35:24]

The self-reference aria-labelledby case is fixed, take a look

[aboxhall, 2016-06-03 22:37:42]

lgtm

[dmazzoni, 2016-06-03 23:02:00]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-06-03 23:02:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
dglazkov@chromium.org
Link to the patchset: https://codereview.chromium.org/1939303002/#ps100001
(title: "Don't modify visited set when computing aria-labelledby")

[commit-bot: I haz the power, 2016-06-03 23:03:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1939303002/100001

[commit-bot: I haz the power, 2016-06-04 03:22:05]

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

[commit-bot: I haz the power, 2016-06-04 03:22:06]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dmazzoni, 2016-06-06 07:10:52]

The CQ bit was checked by dmazzoni@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1939303002/100001

[commit-bot: I haz the power, 2016-06-06 11:38:15]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-06 11:39:34]

Description was changed from

==========
Enable accessible name of a control to include multiple <label> elements.

The spec allows a form control element to have multiple <label> elements,
but we only supported one in accessibility code. Fix it by switching to
using the LabelableElement interface rather than some redundant code that
kept track of label elements in TreeScope, which allows us to get rid of
that now-unused code.

BUG=573494
==========

to

==========
Enable accessible name of a control to include multiple <label> elements.

The spec allows a form control element to have multiple <label> elements,
but we only supported one in accessibility code. Fix it by switching to
using the LabelableElement interface rather than some redundant code that
kept track of label elements in TreeScope, which allows us to get rid of
that now-unused code.

BUG=573494

Committed: https://crrev.com/054bdb4ca5858d7b65188f961a52b8b04e94078d
Cr-Commit-Position: refs/heads/master@{#398005}
==========

[commit-bot: I haz the power, 2016-06-06 11:39:35]

Patchset 6 (id:??) landed as
https://crrev.com/054bdb4ca5858d7b65188f961a52b8b04e94078d
Cr-Commit-Position: refs/heads/master@{#398005}