Relation list properties for Accessibility Object Model phase 1

Relation list properties for Accessibility Object Model phase 1

This change implements AOM support for controls, describedBy,
flowTo, labeledBy, and owns. The corresponding ARIA attributes
all take lists of IDREFs, and the AOM properties all take references
to AccessibleNodeLists instead.

BUG=680345
Review-Url: https://codereview.chromium.org/2967193003
Cr-Commit-Position: refs/heads/master@{#484950}
Committed: https://chromium.googlesource.com/chromium/src/+/0ae8387e4e3efa43990288217ed2f042c78a8dd8

[dmazzoni, 2017-07-06 04:55:40]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-06 04:55:50]

Dry run: CQ is trying da patch.

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

[dmazzoni, 2017-07-06 05:09:04]

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

[aboxhall, 2017-07-06 05:37:14]

https://codereview.chromium.org/2967193003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/AccessibleNode.cpp (right):

https://codereview.chromium.org/2967193003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/AccessibleNode.cpp:86: // Note that there are
two valid spellings of this attribute.
Perhaps "allowed" instead of "valid"?

https://codereview.chromium.org/2967193003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/AccessibleNode.cpp:88: return
aria_labeledbyAttr;
I think we should prefer the English spelling here, since that's the actual
valid spelling in the spec.

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

https://codereview.chromium.org/2967193003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:1003:
NameSource(*found_text_alternative, aria_labeledbyAttr));
Hm, I think we should have a different source for AOM properties (and should
there be an AOM version for aria_label as well?)

[dmazzoni, 2017-07-06 06:34:25]

https://codereview.chromium.org/2967193003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/AccessibleNode.cpp (right):

https://codereview.chromium.org/2967193003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/AccessibleNode.cpp:86: // Note that there are
two valid spellings of this attribute.
On 2017/07/06 05:37:14, aboxhall wrote:
> Perhaps "allowed" instead of "valid"?

Done.

https://codereview.chromium.org/2967193003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/AccessibleNode.cpp:88: return
aria_labeledbyAttr;
On 2017/07/06 05:37:14, aboxhall wrote:
> I think we should prefer the English spelling here, since that's the actual
> valid spelling in the spec.

Done, I flipped a couple other places in this file so that the English
spelling is now the preferred one and the alternate spelling is the
one also checked just in case.

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

https://codereview.chromium.org/2967193003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:1003:
NameSource(*found_text_alternative, aria_labeledbyAttr));
On 2017/07/06 05:37:14, aboxhall wrote:
> Hm, I think we should have a different source for AOM properties (and should
> there be an AOM version for aria_label as well?)

For aria-label and almost every other place in the code where we were
previously getting an ARIA attribute, we're now calling
GetAOMPropertyOrARIAAttribute,
which allows us to not duplicate all of that code.

I'd be totally open to being more explicit about the AOM in developer tools,
but I think it's beyond the scope of this change since we haven't been doing
it for other properties yet.

Considering AOM in general is still experimental and subject to change, we
may also want to consider holding off on adding "extra" dev tools support,
and defer that until we're closer to wanting to ship this.

[dmazzoni, 2017-07-06 06:39:40]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-06 06:39:48]

Dry run: CQ is trying da patch.

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

[aboxhall, 2017-07-06 06:55:15]

lgtm

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

https://codereview.chromium.org/2967193003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:1003:
NameSource(*found_text_alternative, aria_labeledbyAttr));
On 2017/07/06 06:34:25, dmazzoni wrote:
> On 2017/07/06 05:37:14, aboxhall wrote:
> > Hm, I think we should have a different source for AOM properties (and should
> > there be an AOM version for aria_label as well?)
> 
> For aria-label and almost every other place in the code where we were
> previously getting an ARIA attribute, we're now calling
> GetAOMPropertyOrARIAAttribute,
> which allows us to not duplicate all of that code.
> 
> I'd be totally open to being more explicit about the AOM in developer tools,
> but I think it's beyond the scope of this change since we haven't been doing
> it for other properties yet.
> 
> Considering AOM in general is still experimental and subject to change, we
> may also want to consider holding off on adding "extra" dev tools support,
> and defer that until we're closer to wanting to ship this.

Makes sense!

[dmazzoni, 2017-07-06 07:31:13]

Hmmm, I just realized that now aria-labelledby is showing up twice.
I guess that's pretty confusing.

I'm assuming a better short-term solution for it to just have a single
entry for aria-labelledby that's taken from the AOM only if the AOM
property is present...

  "name": {
    "type": "computedString",
    "value": "Select All",
    "sources": [
      {
        "type": "relatedElement",
        "attribute": "aria-labeledby"
      },
      {
        "type": "relatedElement",
        "attribute": "aria-labelledby"
      },
      {
        "type": "attribute",
        "attribute": "aria-label"
      },
      {
        "type": "relatedElement",
        "nativeSource": "label"
      },
      {
        "type": "contents",
        "value": {
          "type": "computedString",
          "value": "Select All"
        }
      },
      {
        "type": "attribute",
        "attribute": "title",
        "superseded": true
      }
    ]
  },

[dmazzoni, 2017-07-07 07:14:20]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-07 07:14:40]

Dry run: CQ is trying da patch.

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

[dmazzoni, 2017-07-07 07:16:17]

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

[dmazzoni, 2017-07-07 07:16:18]

+mkwst for owners review

[commit-bot: I haz the power, 2017-07-07 08:05:24]

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

[commit-bot: I haz the power, 2017-07-07 08:05:25]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Mike West, 2017-07-07 11:08:32]

LGTM!

[dmazzoni, 2017-07-07 15:32:29]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2017-07-07 15:32:30]

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/2967193003/#ps60001
(title: "Get rid of accidental duplication in inspector output")

[commit-bot: I haz the power, 2017-07-07 15:32:45]

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, 2017-07-07 16:32:30]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1499441549875520,
"parent_rev": "a22efb271e62974dc0f1016fbb2d00730079eff9", "commit_rev":
"0ae8387e4e3efa43990288217ed2f042c78a8dd8"}

[commit-bot: I haz the power, 2017-07-07 16:32:47]

Description was changed from

==========
Relation list properties for Accessibility Object Model phase 1

This change implements AOM support for controls, describedBy,
flowTo, labeledBy, and owns. The corresponding ARIA attributes
all take lists of IDREFs, and the AOM properties all take references
to AccessibleNodeLists instead.

BUG=680345
==========

to

==========
Relation list properties for Accessibility Object Model phase 1

This change implements AOM support for controls, describedBy,
flowTo, labeledBy, and owns. The corresponding ARIA attributes
all take lists of IDREFs, and the AOM properties all take references
to AccessibleNodeLists instead.

BUG=680345

Review-Url: https://codereview.chromium.org/2967193003
Cr-Commit-Position: refs/heads/master@{#484950}
Committed:
https://chromium.googlesource.com/chromium/src/+/0ae8387e4e3efa43990288217ed2...
==========

[commit-bot: I haz the power, 2017-07-07 16:32:49]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/0ae8387e4e3efa43990288217ed2...

[loonybear, 2017-08-16 21:20:20]

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

[loonybear, 2017-08-16 21:20:21]

Hi, I am looking at your code:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Acces...

  value.SimplifyWhiteSpace();
  Vector<String> ids;
  value.Split(' ', ids);

SimplyfyWhiteSpace() returns a String, but it doesn't modify the feeding String
in place.

Either "value.SimplifyWhiteSpace();" was unnecessary or it should be "value_new
= value.SimplifyWhiteSpace(); ... ; value_new.Split(' ', ids)"

Please take another look at this.

Thanks