Finish implementation and tests of 5 ARIA 1.1 attributes.

Finish implementation and tests of 5 ARIA 1.1 attributes.

The changes that add support for these 5 new ARIA 1.1
attributes have landed:

  aria-details
  aria-errormessage
  aria-keyshortcuts
  aria-modal
  aria-roledescription

This change completes the implementation in Chrome, to the extent
that they are specified, and adds tests.

BUG=675994, 644766
Review-Url: https://codereview.chromium.org/2684543002
Cr-Commit-Position: refs/heads/master@{#449545}
Committed: https://chromium.googlesource.com/chromium/src/+/ca0b1e8456c010011b838b1f8d35027e45fc1c7d

[dmazzoni, 2017-02-07 08:26:31]

Description was changed from

==========
5 ARIA 1.1

BUG=
==========

to

==========
Finish implementation and tests of 5 ARIA 1.1 attributes.

The changes that add support for these 5 new ARIA 1.1
attributes have landed:

  aria-details
  aria-errormessage
  aria-keyshortcuts
  aria-modal
  aria-roledescription

This change completes the implementation in Chrome, to the extent
that they are specified, and adds tests.

BUG=675994, 644766
==========

[dmazzoni, 2017-02-07 08:27:15]

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

[commit-bot: I haz the power, 2017-02-07 08:27:31]

Dry run: 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-02-07 08:33:55]

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

[commit-bot: I haz the power, 2017-02-07 08:33:56]

Dry run: 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...)

[dmazzoni, 2017-02-07 08:49:45]

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

[commit-bot: I haz the power, 2017-02-07 08:49:58]

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

[dmazzoni, 2017-02-07 08:53:18]

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

[dmazzoni, 2017-02-07 08:53:36]

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

[commit-bot: I haz the power, 2017-02-07 08:53:36]

Dry run: 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-02-07 09:28:02]

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

[commit-bot: I haz the power, 2017-02-07 09:28:03]

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_...)

[dmazzoni, 2017-02-07 22:50:08]

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

[commit-bot: I haz the power, 2017-02-07 22:50:56]

Dry run: 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-02-07 23:42:26]

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

[commit-bot: I haz the power, 2017-02-07 23:42:27]

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

[dmazzoni, 2017-02-07 23:54:48]

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

[commit-bot: I haz the power, 2017-02-07 23:56:06]

Dry run: 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-02-08 01:59:56]

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

[commit-bot: I haz the power, 2017-02-08 01:59:57]

Dry run: This issue passed the CQ dry run.

[aboxhall, 2017-02-08 02:35:18]

https://codereview.chromium.org/2684543002/diff/60001/content/browser/accessi...
File content/browser/accessibility/accessibility_tree_formatter_blink.cc
(right):

https://codereview.chromium.org/2684543002/diff/60001/content/browser/accessi...
content/browser/accessibility/accessibility_tree_formatter_blink.cc:205: if
(dict.GetString(ui::ToString(attr), &string_value)) {
very minor nit: you early out below but not here, even though they are more or
less parallel otherwise.

https://codereview.chromium.org/2684543002/diff/60001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/2684543002/diff/60001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_win.cc:662: if
(target->HasStringAttribute(ui::AX_ATTR_KEY_SHORTCUTS)) {
Should roledescription go in here somewhere as well?

https://codereview.chromium.org/2684543002/diff/60001/content/test/data/acces...
File content/test/data/accessibility/aria/aria-details-expected-blink.txt
(right):

https://codereview.chromium.org/2684543002/diff/60001/content/test/data/acces...
content/test/data/accessibility/aria/aria-details-expected-blink.txt:1:
rootWebArea
Where is the Windows expectation for this?

https://codereview.chromium.org/2684543002/diff/60001/content/test/data/acces...
File content/test/data/accessibility/aria/aria-errormessage-expected-blink.txt
(right):

https://codereview.chromium.org/2684543002/diff/60001/content/test/data/acces...
content/test/data/accessibility/aria/aria-errormessage-expected-blink.txt:1:
rootWebArea
Windows expectation?

https://codereview.chromium.org/2684543002/diff/60001/content/test/data/acces...
File content/test/data/accessibility/aria/aria-keyshortcuts-expected-win.txt
(right):

https://codereview.chromium.org/2684543002/diff/60001/content/test/data/acces...
content/test/data/accessibility/aria/aria-keyshortcuts-expected-win.txt:1:
ROLE_SYSTEM_DOCUMENT READONLY FOCUSABLE
Do we want a blink expectation here too?

https://codereview.chromium.org/2684543002/diff/60001/content/test/data/acces...
File content/test/data/accessibility/aria/aria-modal.html (right):

https://codereview.chromium.org/2684543002/diff/60001/content/test/data/acces...
content/test/data/accessibility/aria/aria-modal.html:10: <div role="dialog"
aria-modal="true">Modal</div>
The ED has some advice about implementing this for Mac now - it's a bit scary
though.

"Prune the accessibility tree such that the background content is no longer
exposed. No specific property is set on the accessible object that corresponds
to the element with aria-modal="true". Only the tree whose root is that modal
accessible object is exposed."

http://w3c.github.io/aria/core-aam/core-aam.html

[dmazzoni, 2017-02-08 20:23:42]

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

[commit-bot: I haz the power, 2017-02-08 20:24:24]

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

[dmazzoni, 2017-02-08 21:44:15]

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

[commit-bot: I haz the power, 2017-02-08 21:44:55]

Dry run: 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-02-08 22:29:42]

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

[commit-bot: I haz the power, 2017-02-08 22:29:43]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[dmazzoni, 2017-02-09 18:22:42]

https://codereview.chromium.org/2684543002/diff/60001/content/browser/accessi...
File content/browser/accessibility/accessibility_tree_formatter_blink.cc
(right):

https://codereview.chromium.org/2684543002/diff/60001/content/browser/accessi...
content/browser/accessibility/accessibility_tree_formatter_blink.cc:205: if
(dict.GetString(ui::ToString(attr), &string_value)) {
On 2017/02/08 02:35:18, aboxhall wrote:
> very minor nit: you early out below but not here, even though they are more or
> less parallel otherwise.

Done.

https://codereview.chromium.org/2684543002/diff/60001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_win.cc (right):

https://codereview.chromium.org/2684543002/diff/60001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_win.cc:662: if
(target->HasStringAttribute(ui::AX_ATTR_KEY_SHORTCUTS)) {
On 2017/02/08 02:35:18, aboxhall wrote:
> Should roledescription go in here somewhere as well?

Oh yeah, it should.

The spec says localizedExtendedRole, so I implemented that
and added a test expectation.

I also added them to the attributes string since screen readers tend to expect
ARIA attributes there.

https://codereview.chromium.org/2684543002/diff/60001/content/test/data/acces...
File content/test/data/accessibility/aria/aria-details-expected-blink.txt
(right):

https://codereview.chromium.org/2684543002/diff/60001/content/test/data/acces...
content/test/data/accessibility/aria/aria-details-expected-blink.txt:1:
rootWebArea
On 2017/02/08 02:35:18, aboxhall wrote:
> Where is the Windows expectation for this?

Added. We should probably try to improve the ability to
test things like relations, but this is a start.

https://codereview.chromium.org/2684543002/diff/60001/content/test/data/acces...
File content/test/data/accessibility/aria/aria-errormessage-expected-blink.txt
(right):

https://codereview.chromium.org/2684543002/diff/60001/content/test/data/acces...
content/test/data/accessibility/aria/aria-errormessage-expected-blink.txt:1:
rootWebArea
On 2017/02/08 02:35:18, aboxhall wrote:
> Windows expectation?

same

https://codereview.chromium.org/2684543002/diff/60001/content/test/data/acces...
File content/test/data/accessibility/aria/aria-keyshortcuts-expected-win.txt
(right):

https://codereview.chromium.org/2684543002/diff/60001/content/test/data/acces...
content/test/data/accessibility/aria/aria-keyshortcuts-expected-win.txt:1:
ROLE_SYSTEM_DOCUMENT READONLY FOCUSABLE
On 2017/02/08 02:35:18, aboxhall wrote:
> Do we want a blink expectation here too?

Done.

https://codereview.chromium.org/2684543002/diff/60001/content/test/data/acces...
File content/test/data/accessibility/aria/aria-modal.html (right):

https://codereview.chromium.org/2684543002/diff/60001/content/test/data/acces...
content/test/data/accessibility/aria/aria-modal.html:10: <div role="dialog"
aria-modal="true">Modal</div>
On 2017/02/08 02:35:18, aboxhall wrote:
> The ED has some advice about implementing this for Mac now - it's a bit scary
> though.
> 
> "Prune the accessibility tree such that the background content is no longer
> exposed. No specific property is set on the accessible object that corresponds
> to the element with aria-modal="true". Only the tree whose root is that modal
> accessible object is exposed."
> 
> http://w3c.github.io/aria/core-aam/core-aam.html

Isn't that what we do already with <dialog>?

I don't want to try to implement that as part of this change
but we should consider either trying to implement that or push back on
the spec as a follow-up.

[dmazzoni, 2017-02-09 18:22:45]

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

[commit-bot: I haz the power, 2017-02-09 18:23:14]

Dry run: 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-02-09 19:37:01]

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

[commit-bot: I haz the power, 2017-02-09 19:37:02]

Dry run: This issue passed the CQ dry run.

[aboxhall, 2017-02-10 03:05:08]

lgtm

https://codereview.chromium.org/2684543002/diff/60001/content/test/data/acces...
File content/test/data/accessibility/aria/aria-modal.html (right):

https://codereview.chromium.org/2684543002/diff/60001/content/test/data/acces...
content/test/data/accessibility/aria/aria-modal.html:10: <div role="dialog"
aria-modal="true">Modal</div>
On 2017/02/09 18:22:42, dmazzoni wrote:
> On 2017/02/08 02:35:18, aboxhall wrote:
> > The ED has some advice about implementing this for Mac now - it's a bit
scary
> > though.
> > 
> > "Prune the accessibility tree such that the background content is no longer
> > exposed. No specific property is set on the accessible object that
corresponds
> > to the element with aria-modal="true". Only the tree whose root is that
modal
> > accessible object is exposed."
> > 
> > http://w3c.github.io/aria/core-aam/core-aam.html
> 
> Isn't that what we do already with <dialog>?
> 
> I don't want to try to implement that as part of this change
> but we should consider either trying to implement that or push back on
> the spec as a follow-up.

Agreed that that's a bit big for this change. Yeah, we do it for dialog, but it
seems like a big hammer for an ARIA attribute - then again, aria-hidden has the
same effect, so maybe it's fine. It's just a bit funny that on other platforms
it's an attribute, then on mac it's tree rewriting.

[dmazzoni, 2017-02-10 03:51:55]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2017-02-10 03:52:14]

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-02-10 03:59:14]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1486698715149700,
"parent_rev": "b9dae93e677cb7d8eaac27575d37bcfdf0d4608b", "commit_rev":
"ca0b1e8456c010011b838b1f8d35027e45fc1c7d"}

[commit-bot: I haz the power, 2017-02-10 04:00:03]

Description was changed from

==========
Finish implementation and tests of 5 ARIA 1.1 attributes.

The changes that add support for these 5 new ARIA 1.1
attributes have landed:

  aria-details
  aria-errormessage
  aria-keyshortcuts
  aria-modal
  aria-roledescription

This change completes the implementation in Chrome, to the extent
that they are specified, and adds tests.

BUG=675994, 644766
==========

to

==========
Finish implementation and tests of 5 ARIA 1.1 attributes.

The changes that add support for these 5 new ARIA 1.1
attributes have landed:

  aria-details
  aria-errormessage
  aria-keyshortcuts
  aria-modal
  aria-roledescription

This change completes the implementation in Chrome, to the extent
that they are specified, and adds tests.

BUG=675994, 644766

Review-Url: https://codereview.chromium.org/2684543002
Cr-Commit-Position: refs/heads/master@{#449545}
Committed:
https://chromium.googlesource.com/chromium/src/+/ca0b1e8456c010011b838b1f8d35...
==========

[commit-bot: I haz the power, 2017-02-10 04:00:04]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/ca0b1e8456c010011b838b1f8d35...

[David Greens, 2017-02-10 09:44:01]

hektve@yahoo.com changed reviewers:
+ Hektve@yahoo.com, greedhector28@gmail.com, hektve@yahoo.com