Switch all LayoutTests to use new AX name calculation

Switch all LayoutTests to use new AX name calculation

BUG=473220, 550663
Committed: https://crrev.com/3f6ec08f8035d976cf5fe8a338de3202f42fd152
Cr-Commit-Position: refs/heads/master@{#358900}

[dmazzoni, 2015-11-05 17:15:43]

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

[dmazzoni, 2015-11-05 17:15:44]

Suggestion: review the cpp/h files first, then we can work through all of the
changes to test.

Most of the tests had only trivial changes. Around 20 or so had to be updated or
fixed to match the new spec.

[aboxhall, 2015-11-05 17:49:21]

Just commenting on cpp/h. Only one major issue.

https://codereview.chromium.org/1417213006/diff/1/components/test_runner/web_...
File components/test_runner/web_ax_object_proxy.cc (left):

https://codereview.chromium.org/1417213006/diff/1/components/test_runner/web_...
components/test_runner/web_ax_object_proxy.cc:317: // FIXME: Concatenate all
attributes of the AXObject.
Did we abandon this idea then?

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

https://codereview.chromium.org/1417213006/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:1501: return
deprecatedTextUnderElement(TextUnderElementAll);
Should this be addressed in this change?

https://codereview.chromium.org/1417213006/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:1945: return
stringValue();
I'm not sure how I feel about this. It's calling into old, partly-deprecated
code. What issue does this address?

https://codereview.chromium.org/1417213006/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2862: //
Fieldset / legend.
Wohoo! You can add http://crbug.com/550663 to the BUG= line.

https://codereview.chromium.org/1417213006/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2887:
source.nativeSource = AXTextFromNativeHTMLLegend;
No need to re-set this.

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

https://codereview.chromium.org/1417213006/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:39: #include
"core/html/parser/HTMLParserIdioms.h"
Unused?

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

https://codereview.chromium.org/1417213006/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp:235: if
(isMenuListOption(node) || isHTMLAreaElement(node))
Add a comment to explain this?

https://codereview.chromium.org/1417213006/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp:402: if
(node->layoutObject() && !isHTMLAreaElement(node))
And a comment for this too?

[dmazzoni, 2015-11-06 00:15:38]

Description was changed from

==========
Switch all LayoutTests to use new AX name calculation

BUG=473220
==========

to

==========
Switch all LayoutTests to use new AX name calculation

BUG=473220,550663
==========

[dmazzoni, 2015-11-06 00:21:18]

https://codereview.chromium.org/1417213006/diff/1/components/test_runner/web_...
File components/test_runner/web_ax_object_proxy.cc (left):

https://codereview.chromium.org/1417213006/diff/1/components/test_runner/web_...
components/test_runner/web_ax_object_proxy.cc:317: // FIXME: Concatenate all
attributes of the AXObject.
On 2015/11/05 17:49:21, aboxhall wrote:
> Did we abandon this idea then?

This is inherited from WebKit tests. Concatenating all of the attributes just
made for brittle, hard to read test expectations. Basically I'm only leaving
this here to support existing tests, but I don't like using it for new tests.

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

https://codereview.chromium.org/1417213006/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:1501: return
deprecatedTextUnderElement(TextUnderElementAll);
On 2015/11/05 17:49:21, aboxhall wrote:
> Should this be addressed in this change?

Not yet. Changing this could affect existing Chromium code that calls
stringValue, and I wanted to only change the tests so far.

https://codereview.chromium.org/1417213006/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:1945: return
stringValue();
On 2015/11/05 17:49:21, aboxhall wrote:
> I'm not sure how I feel about this. It's calling into old, partly-deprecated
> code. What issue does this address?

The current code only handles the string value of date/time controls, select
elements, and a few others, which is important when embedded in the middle of a
text alternative.

Note that even though this calls stringValue(), that will never result in a call
to deprecatedTextUnderElement since we're only calling it when the current node
is a control.

https://codereview.chromium.org/1417213006/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2862: //
Fieldset / legend.
On 2015/11/05 17:49:21, aboxhall wrote:
> Wohoo! You can add http://crbug.com/550663 to the BUG= line.

Done.

https://codereview.chromium.org/1417213006/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2887:
source.nativeSource = AXTextFromNativeHTMLLegend;
On 2015/11/05 17:49:21, aboxhall wrote:
> No need to re-set this.

Done.

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

https://codereview.chromium.org/1417213006/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:39: #include
"core/html/parser/HTMLParserIdioms.h"
On 2015/11/05 17:49:21, aboxhall wrote:
> Unused?

It's where we get isHTMLSpace

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

https://codereview.chromium.org/1417213006/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp:235: if
(isMenuListOption(node) || isHTMLAreaElement(node))
On 2015/11/05 17:49:21, aboxhall wrote:
> Add a comment to explain this?

Done.

https://codereview.chromium.org/1417213006/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp:402: if
(node->layoutObject() && !isHTMLAreaElement(node))
On 2015/11/05 17:49:21, aboxhall wrote:
> And a comment for this too?

Done.

[aboxhall, 2015-11-06 01:11:22]

Missing upload? :(

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

https://codereview.chromium.org/1417213006/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:1945: return
stringValue();
On 2015/11/06 00:21:17, dmazzoni wrote:
> On 2015/11/05 17:49:21, aboxhall wrote:
> > I'm not sure how I feel about this. It's calling into old, partly-deprecated
> > code. What issue does this address?
> 
> The current code only handles the string value of date/time controls, select
> elements, and a few others, which is important when embedded in the middle of
a
> text alternative.
> 
> Note that even though this calls stringValue(), that will never result in a
call
> to deprecatedTextUnderElement since we're only calling it when the current
node
> is a control.

Could we possibly pull out the control-related logic into a
"textValueForControl" (or whatever) method and call that from both places?

[aboxhall, 2015-11-06 01:11:23]

Missing upload? :(

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

https://codereview.chromium.org/1417213006/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:1945: return
stringValue();
On 2015/11/06 00:21:17, dmazzoni wrote:
> On 2015/11/05 17:49:21, aboxhall wrote:
> > I'm not sure how I feel about this. It's calling into old, partly-deprecated
> > code. What issue does this address?
> 
> The current code only handles the string value of date/time controls, select
> elements, and a few others, which is important when embedded in the middle of
a
> text alternative.
> 
> Note that even though this calls stringValue(), that will never result in a
call
> to deprecatedTextUnderElement since we're only calling it when the current
node
> is a control.

Could we possibly pull out the control-related logic into a
"textValueForControl" (or whatever) method and call that from both places?

[dmazzoni, 2015-11-06 04:27:52]

On 2015/11/06 01:11:23, aboxhall wrote:
> Missing upload? :(

Sorry! Shuttle wifi wasn't able to handle it. :(

Uploaded now.

[dmazzoni, 2015-11-06 04:54:06]

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

https://codereview.chromium.org/1417213006/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:1945: return
stringValue();
On 2015/11/06 01:11:23, aboxhall wrote:
> On 2015/11/06 00:21:17, dmazzoni wrote:
> > On 2015/11/05 17:49:21, aboxhall wrote:
> > > I'm not sure how I feel about this. It's calling into old,
partly-deprecated
> > > code. What issue does this address?
> > 
> > The current code only handles the string value of date/time controls, select
> > elements, and a few others, which is important when embedded in the middle
of
> a
> > text alternative.
> > 
> > Note that even though this calls stringValue(), that will never result in a
> call
> > to deprecatedTextUnderElement since we're only calling it when the current
> node
> > is a control.
> 
> Could we possibly pull out the control-related logic into a
> "textValueForControl" (or whatever) method and call that from both places?

Good idea!

Done.

[aboxhall, 2015-11-06 19:24:11]

lgtm

LGTM with minor issues

https://codereview.chromium.org/1417213006/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/accessibility/aria-labelledby-overrides-label.html
(right):

https://codereview.chromium.org/1417213006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/aria-labelledby-overrides-label.html:29:
shouldBe("firstNameElement.role", "'AXRole: AXUnknown'");
Not a big deal, but this looks a little flaky now that the expectations have
changed subtly. Perhaps we could bypass the label altogether, something like
<label id="labelElement" for="shutdownTime">Shutdown delay: </label>
<span id="shutdownLabelBefore">Shut down computer after</span>
<input id="shutdownTime" type="text" value="10"
aria-labelledby="shutdownLabelBefore shutdownTime shutdownUnit" />
<span id="shutdownUnit">minutes</span>

Then the text content will show that the label is not involved, and this check
is just a sanity check.

https://codereview.chromium.org/1417213006/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/accessibility/focusable-div-expected.txt
(right):

https://codereview.chromium.org/1417213006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/focusable-div-expected.txt:15: FAIL
axLink.name should be . Was A.
This doesn't look good? Wrong expectation, given this is not a generic focusable
div?

https://codereview.chromium.org/1417213006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/focusable-div-expected.txt:21: FAIL
axDiv3.name should be . Was D.
Wrong expectation again?

https://codereview.chromium.org/1417213006/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/accessibility/svg-bounds-expected.txt
(right):

https://codereview.chromium.org/1417213006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/svg-bounds-expected.txt:9: Face
label: face
I love this test.

[dmazzoni, 2015-11-06 22:32:25]

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

[dmazzoni, 2015-11-06 22:32:26]

+pfeldman for trivial change to Source/devtools/

https://codereview.chromium.org/1417213006/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/accessibility/aria-labelledby-overrides-label.html
(right):

https://codereview.chromium.org/1417213006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/aria-labelledby-overrides-label.html:29:
shouldBe("firstNameElement.role", "'AXRole: AXUnknown'");
On 2015/11/06 19:24:11, aboxhall wrote:
> Not a big deal, but this looks a little flaky now that the expectations have
> changed subtly. Perhaps we could bypass the label altogether, something like
> <label id="labelElement" for="shutdownTime">Shutdown delay: </label>
> <span id="shutdownLabelBefore">Shut down computer after</span>
> <input id="shutdownTime" type="text" value="10"
> aria-labelledby="shutdownLabelBefore shutdownTime shutdownUnit" />
> <span id="shutdownUnit">minutes</span>
> 
> Then the text content will show that the label is not involved, and this check
> is just a sanity check.

Good idea.

I rewrote it to show the label is not involved without the weird role.

https://codereview.chromium.org/1417213006/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/accessibility/focusable-div-expected.txt
(right):

https://codereview.chromium.org/1417213006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/focusable-div-expected.txt:15: FAIL
axLink.name should be . Was A.
On 2015/11/06 19:24:11, aboxhall wrote:
> This doesn't look good? Wrong expectation, given this is not a generic
focusable
> div?

Done.

https://codereview.chromium.org/1417213006/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/focusable-div-expected.txt:21: FAIL
axDiv3.name should be . Was D.
On 2015/11/06 19:24:11, aboxhall wrote:
> Wrong expectation again?

Done.

[pfeldman, 2015-11-09 19:52:35]

protocol.json lgtm

[dmazzoni, 2015-11-09 19:54:23]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-11-09 19:54:23]

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/1417213006/#ps80001
(title: "Removed platform-specific expectations for one test")

[commit-bot: I haz the power, 2015-11-09 19:56:33]

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

[commit-bot: I haz the power, 2015-11-09 21:55:36]

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

[commit-bot: I haz the power, 2015-11-09 21:55:38]

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

[dmazzoni, 2015-11-10 00:36:55]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-11-10 00:36:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
aboxhall@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1417213006/#ps120001
(title: "Update an oilpan expectation too")

[commit-bot: I haz the power, 2015-11-10 00:40:40]

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

[commit-bot: I haz the power, 2015-11-10 02:30:42]

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

[commit-bot: I haz the power, 2015-11-10 02:30:43]

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

[dmazzoni, 2015-11-10 17:52:43]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-11-10 17:52:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
aboxhall@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1417213006/#ps140001
(title: "Fix canvas-fallback-content-labels-expected.txt")

[commit-bot: I haz the power, 2015-11-10 17:53:27]

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

[commit-bot: I haz the power, 2015-11-10 21:06:56]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-11-10 21:07:52]

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