Make use of new AX name calc in Chromium.

Make use of new AX name calc in Chromium.

Previously we finished implementing the new standards-based
accessible name calculation code in Blink but still
called the old code from Chromium. This change switches
all code paths to call the new name calculation code and
deletes the old code.

This required updating a bunch of tests. All changes
are either a net positive (the new accessible
name is better) or neutral / arbitrary.

BUG=473220
TBR=mkwst,pfeldman

Committed: https://crrev.com/6185ca046850b24b92a5dffd9ab7d1ecc3048001
Cr-Commit-Position: refs/heads/master@{#360146}

[dmazzoni, 2015-11-13 16:14:43]

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

[dmazzoni, 2015-11-13 16:14:44]

Just a handful of tests left to fix, this is ready for a first pass!

[aboxhall, 2015-11-13 23:13:49]

First round...

https://codereview.chromium.org/1435113003/diff/120001/components/html_viewer...
File components/html_viewer/ax_provider_impl.cc (right):

https://codereview.chromium.org/1435113003/diff/120001/components/html_viewer...
components/html_viewer/ax_provider_impl.cc:87: blink::WebString name =
ax_object.name(nameFrom, nameObjects);
Would it make sense to create a version of name() which didn't take nameObjects,
which just creates the throwaway version on the Blink side?

https://codereview.chromium.org/1435113003/diff/120001/content/browser/access...
File content/browser/accessibility/browser_accessibility_win.cc (left):

https://codereview.chromium.org/1435113003/diff/120001/content/browser/access...
content/browser/accessibility/browser_accessibility_win.cc:3445: // Now
implement the main logic: the descripion should become the name if
This is so great.

https://codereview.chromium.org/1435113003/diff/120001/content/browser/access...
File content/browser/accessibility/browser_accessibility_win.h (right):

https://codereview.chromium.org/1435113003/diff/120001/content/browser/access...
content/browser/accessibility/browser_accessibility_win.h:860: void
AddRelations(ui::AXIntListAttribute src_attr,
Comment for this to be consistent with other methods?

https://codereview.chromium.org/1435113003/diff/120001/content/renderer/acces...
File content/renderer/accessibility/blink_ax_enum_conversion.cc (right):

https://codereview.chromium.org/1435113003/diff/120001/content/renderer/acces...
content/renderer/accessibility/blink_ax_enum_conversion.cc:508: switch
(description_from) {
space??

https://codereview.chromium.org/1435113003/diff/120001/content/renderer/acces...
File content/renderer/accessibility/blink_ax_tree_source.cc (right):

https://codereview.chromium.org/1435113003/diff/120001/content/renderer/acces...
content/renderer/accessibility/blink_ax_tree_source.cc:283: blink::WebString
web_description = src.description(
more spaces??

https://codereview.chromium.org/1435113003/diff/120001/content/test/data/acce...
File content/test/data/accessibility/html/input-datetime-local-expected-win.txt
(left):

https://codereview.chromium.org/1435113003/diff/120001/content/test/data/acce...
content/test/data/accessibility/html/input-datetime-local-expected-win.txt:6:
++++++++++ROLE_SYSTEM_SPINBUTTON name='Month' FOCUSABLE ia2_hypertext='Month'
This change is expected?

https://codereview.chromium.org/1435113003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXListBoxOption.cpp
(right):

https://codereview.chromium.org/1435113003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXListBoxOption.cpp:150:
nameSources->append(NameSource(true, aria_labelAttr));
Why true?

https://codereview.chromium.org/1435113003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/1435113003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:1490: case
PopUpButtonRole:
Why has this changed?

[dmazzoni, 2015-11-16 18:45:10]

Description was changed from

==========
Make use of new AX name calc in Chromium.

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

to

==========
Make use of new AX name calc in Chromium.

This completely gets rid of all of the old accessible
name calculation code in favor of the new standards-based
implementation!

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

[dmazzoni, 2015-11-16 18:52:02]

dmazzoni@chromium.org changed reviewers:
+ mkwst@chromium.org, pfeldman@chromium.org, sky@chromium.org

[dmazzoni, 2015-11-16 18:52:04]

+pfeldman for devtools/protocol.json
+sky for components/html_viewer
+mkwst for third_party/WebKit/public

https://codereview.chromium.org/1435113003/diff/120001/components/html_viewer...
File components/html_viewer/ax_provider_impl.cc (right):

https://codereview.chromium.org/1435113003/diff/120001/components/html_viewer...
components/html_viewer/ax_provider_impl.cc:87: blink::WebString name =
ax_object.name(nameFrom, nameObjects);
On 2015/11/13 23:13:48, aboxhall wrote:
> Would it make sense to create a version of name() which didn't take
nameObjects,
> which just creates the throwaway version on the Blink side?

Done.

https://codereview.chromium.org/1435113003/diff/120001/content/browser/access...
File content/browser/accessibility/browser_accessibility_win.cc (left):

https://codereview.chromium.org/1435113003/diff/120001/content/browser/access...
content/browser/accessibility/browser_accessibility_win.cc:3445: // Now
implement the main logic: the descripion should become the name if
On 2015/11/13 23:13:48, aboxhall wrote:
> This is so great.

Yay for deleted code!

https://codereview.chromium.org/1435113003/diff/120001/content/browser/access...
File content/browser/accessibility/browser_accessibility_win.h (right):

https://codereview.chromium.org/1435113003/diff/120001/content/browser/access...
content/browser/accessibility/browser_accessibility_win.h:860: void
AddRelations(ui::AXIntListAttribute src_attr,
On 2015/11/13 23:13:49, aboxhall wrote:
> Comment for this to be consistent with other methods?

Done.

https://codereview.chromium.org/1435113003/diff/120001/content/renderer/acces...
File content/renderer/accessibility/blink_ax_enum_conversion.cc (right):

https://codereview.chromium.org/1435113003/diff/120001/content/renderer/acces...
content/renderer/accessibility/blink_ax_enum_conversion.cc:508: switch
(description_from) {
On 2015/11/13 23:13:49, aboxhall wrote:
> space??

Huh? I don't see the error

https://codereview.chromium.org/1435113003/diff/120001/content/renderer/acces...
File content/renderer/accessibility/blink_ax_tree_source.cc (right):

https://codereview.chromium.org/1435113003/diff/120001/content/renderer/acces...
content/renderer/accessibility/blink_ax_tree_source.cc:283: blink::WebString
web_description = src.description(
On 2015/11/13 23:13:49, aboxhall wrote:
> more spaces??

Same - the code looks fine to me. Is codereview.chromium.org hiding underscores?
It does that for me sometimes.

https://codereview.chromium.org/1435113003/diff/120001/content/test/data/acce...
File content/test/data/accessibility/html/input-datetime-local-expected-win.txt
(left):

https://codereview.chromium.org/1435113003/diff/120001/content/test/data/acce...
content/test/data/accessibility/html/input-datetime-local-expected-win.txt:6:
++++++++++ROLE_SYSTEM_SPINBUTTON name='Month' FOCUSABLE ia2_hypertext='Month'
On 2015/11/13 23:13:49, aboxhall wrote:
> This change is expected?

Yes. The previous logic promoted this to the accessible name but the new logic
leaves it as the description, because we want the author to be able to label the
overall control.

For example:

<label>
  Event start date
  <input type="datetime">
</label>

We don't want the "Month" label on the first inner text field to override the
"event start date" text, so we let "event start date" be the name, and "month"
the description.

Perhaps aria-placeholder would be a better fit for this once we support it. I
think it's in the draft spec now.

https://codereview.chromium.org/1435113003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXListBoxOption.cpp
(right):

https://codereview.chromium.org/1435113003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXListBoxOption.cpp:150:
nameSources->append(NameSource(true, aria_labelAttr));
On 2015/11/13 23:13:49, aboxhall wrote:
> Why true?

Oh, good catch! I misinterpreted what that first argument meant. Fixed here and
in a few other places.

https://codereview.chromium.org/1435113003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (left):

https://codereview.chromium.org/1435113003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:1489: case
TreeItemRole:
Same here - I don't care what the spec says, this just doesn't make sense
because quite often tree items contain lots of children

https://codereview.chromium.org/1435113003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/1435113003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:1490: case
PopUpButtonRole:
On 2015/11/13 23:13:49, aboxhall wrote:
> Why has this changed?

For an ARIA pop-up button, it didn't seem to make sense not to get the
accessible name from contents, like:

<div role=button aria-haspopup=true>More options</div>

For a native HTML Select element, we override it in AXMenuList.h so we don't get
the name from contents for that.

[dmazzoni, 2015-11-16 18:55:03]

Description was changed from

==========
Make use of new AX name calc in Chromium.

This completely gets rid of all of the old accessible
name calculation code in favor of the new standards-based
implementation!

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

to

==========
Make use of new AX name calc in Chromium.

Previously we finished implementing the new standards-based
accessible name calculation code in Blink but still
called the old code from Chromium. This change switches
all code paths to call the new name calculation code and
deletes the old code.

This required updating a bunch of tests. All changes
are either a net positive (the new accessible
name is better) or neutral / arbitrary.

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

[sky, 2015-11-16 21:46:40]

components/html_viewer LGTM

[aboxhall, 2015-11-16 22:03:28]

https://codereview.chromium.org/1435113003/diff/120001/content/renderer/acces...
File content/renderer/accessibility/blink_ax_tree_source.cc (right):

https://codereview.chromium.org/1435113003/diff/120001/content/renderer/acces...
content/renderer/accessibility/blink_ax_tree_source.cc:283: blink::WebString
web_description = src.description(
On 2015/11/16 18:52:04, dmazzoni wrote:
> On 2015/11/13 23:13:49, aboxhall wrote:
> > more spaces??
> 
> Same - the code looks fine to me. Is http://codereview.chromium.org hiding
underscores?
> It does that for me sometimes.

Super weird. Yeah, it looks fine now.

https://codereview.chromium.org/1435113003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXListBoxOption.cpp
(right):

https://codereview.chromium.org/1435113003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXListBoxOption.cpp:154: return
ariaLabel;
Note: we shouldn't return immediately if nameSources exists, but instead
continue to get all the possible name source options. See
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
for an example of this in AXNodeObject.

https://codereview.chromium.org/1435113003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXListBoxOption.cpp:162:
nameSources->append(NameSource(false));
Then, this needs to be dependent on whether we found an aria-label above or not.

[dmazzoni, 2015-11-17 17:04:29]

https://codereview.chromium.org/1435113003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXListBoxOption.cpp
(right):

https://codereview.chromium.org/1435113003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXListBoxOption.cpp:154: return
ariaLabel;
On 2015/11/16 22:03:28, aboxhall wrote:
> Note: we shouldn't return immediately if nameSources exists, but instead
> continue to get all the possible name source options. See
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> for an example of this in AXNodeObject.

Got it.

I refactored this out into a new helper, ariaTextAlternative, which we now call
from three places. That gets rid of even more duplication than before.

https://codereview.chromium.org/1435113003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXListBoxOption.cpp:162:
nameSources->append(NameSource(false));
On 2015/11/16 22:03:28, aboxhall wrote:
> Then, this needs to be dependent on whether we found an aria-label above or
not.

Done.

[aboxhall, 2015-11-17 20:06:22]

lgtm

https://codereview.chromium.org/1435113003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXListBoxOption.cpp
(right):

https://codereview.chromium.org/1435113003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXListBoxOption.cpp:154: return
ariaLabel;
On 2015/11/17 17:04:29, dmazzoni wrote:
> On 2015/11/16 22:03:28, aboxhall wrote:
> > Note: we shouldn't return immediately if nameSources exists, but instead
> > continue to get all the possible name source options. See
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > for an example of this in AXNodeObject.
> 
> Got it.
> 
> I refactored this out into a new helper, ariaTextAlternative, which we now
call
> from three places. That gets rid of even more duplication than before.

Nice.

[dmazzoni, 2015-11-17 20:13:34]

Description was changed from

==========
Make use of new AX name calc in Chromium.

Previously we finished implementing the new standards-based
accessible name calculation code in Blink but still
called the old code from Chromium. This change switches
all code paths to call the new name calculation code and
deletes the old code.

This required updating a bunch of tests. All changes
are either a net positive (the new accessible
name is better) or neutral / arbitrary.

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

to

==========
Make use of new AX name calc in Chromium.

Previously we finished implementing the new standards-based
accessible name calculation code in Blink but still
called the old code from Chromium. This change switches
all code paths to call the new name calculation code and
deletes the old code.

This required updating a bunch of tests. All changes
are either a net positive (the new accessible
name is better) or neutral / arbitrary.

BUG=473220
TBR=mkwst,pfeldman
==========

[dmazzoni, 2015-11-17 20:14:31]

TBRing pfeldman and mkwst for OWNERS review because changes are trivial (mostly
deleting code) and this is a large change that's hard to continually rebase.
Happy to apply fixes after the fact if needed.

Thanks!

[dmazzoni, 2015-11-17 20:14:43]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-11-17 20:14:44]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1435113003/#ps280001
(title: "Fix issue with ariaTextAlternative")

[commit-bot: I haz the power, 2015-11-17 20:16:22]

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

[commit-bot: I haz the power, 2015-11-17 20:25:26]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-11-17 20:26:08]

Patchset 15 (id:??) landed as
https://crrev.com/6185ca046850b24b92a5dffd9ab7d1ecc3048001
Cr-Commit-Position: refs/heads/master@{#360146}