Various fixes for aria-activedescendant.

Various fixes for aria-activedescendant.

1. Elements with an ARIA role and an ancestor that exposes an active descendant should be marked focusable according to the ARIA Spec.
2. Elements that are the target of an active descendant should be marked focused if the element that exposes the active descendant attribute is also focused.
3. If an active descendant is removed an event should fire.
4. According to the ARIA Spec, composite widgets, text boxes and ARIA groups should be permitted to expose an active descendant. We were not permitting text boxes to do that whilst we were permitting other widgets that should not have been, such as outlines.
5. ARIA list box options should be marked selectable in the accessibility API if they have an ancestor that exposes an aria-activedescendant attribute.
6. ARIA list box options that are focused via aria-activedescendant should also be marked as selected.
7. Focus and blur events should not fire from Bllink when the active descendant changes or when it is removed. Only an active descendant changed event should fire. The browser should handle any platform specific focus changed events and not Blink.
BUG=593646
R=dmazzoni@chromium.org
TESTED=layout test, manually with Jaws and NVDA

Committed: https://crrev.com/1c568219380053695ef94b4372c4911df9c05c99
Cr-Commit-Position: refs/heads/master@{#386489}

[nektarios, 2016-03-30 02:49:24]

Description was changed from

==========
Various fixes for aria-activedescendant.

1. Elements with an ARIA role and an ancestor that exposes an active descendant
should be marked focusable according to the ARIA Spec.
2. Elements that are the target of an active descendant should be marked focused
if the element that exposes the active descendant attribute is also focused.
3. If an active descendant is removed an event should fire.
4. According to the ARIA Spec, only composite widgets, text boxes and ARIA
groups should be permitted to expose an active descendant.
BUG=
R=dmazzoni@chromium.org
TESTED=layout test, manually with Jaws and NVDA
==========

to

==========
Various fixes for aria-activedescendant.

1. Elements with an ARIA role and an ancestor that exposes an active descendant
should be marked focusable according to the ARIA Spec.
2. Elements that are the target of an active descendant should be marked focused
if the element that exposes the active descendant attribute is also focused.
3. If an active descendant is removed an event should fire.
4. According to the ARIA Spec, only composite widgets, text boxes and ARIA
groups should be permitted to expose an active descendant.
BUG=593646
R=dmazzoni@chromium.org
TESTED=layout test, manually with Jaws and NVDA
==========

[nektarios, 2016-03-30 15:29:45]

Description was changed from

==========
Various fixes for aria-activedescendant.

1. Elements with an ARIA role and an ancestor that exposes an active descendant
should be marked focusable according to the ARIA Spec.
2. Elements that are the target of an active descendant should be marked focused
if the element that exposes the active descendant attribute is also focused.
3. If an active descendant is removed an event should fire.
4. According to the ARIA Spec, only composite widgets, text boxes and ARIA
groups should be permitted to expose an active descendant.
BUG=593646
R=dmazzoni@chromium.org
TESTED=layout test, manually with Jaws and NVDA
==========

to

==========
Various fixes for aria-activedescendant.

1. Elements with an ARIA role and an ancestor that exposes an active descendant
should be marked focusable according to the ARIA Spec.
2. Elements that are the target of an active descendant should be marked focused
if the element that exposes the active descendant attribute is also focused.
3. If an active descendant is removed an event should fire.
4. According to the ARIA Spec, composite widgets, text boxes and ARIA groups
should be permitted to expose an active descendant. We were not permitting text
boxes to do that whilst we were permitting other widgets that should not have
been, such as toolbars.
5. ARIA list box options should be marked selectable in the accessibility API if
they have an ancestor that exposes an aria-activedescendant attribute.
6. ARIA list box options that are focused should also be marked as selected.
BUG=593646
R=dmazzoni@chromium.org
TESTED=layout test, manually with Jaws and NVDA
==========

[dmazzoni, 2016-03-30 19:04:20]

It might be good to split this into more than one change.

[blink-reviews, 2016-03-30 19:51:07]

On 3/30/2016 3:04 PM, dmazzoni@chromium.org wrote:
> It might be good to split this into more than one change.
>
One way of splitting it is to have the event changes in one patch and 
the changes to the exposed states in another. But it's not as clear cut 
where the split should happen and I am not sure if the event changes 
will work correctly without the fixes to the focusable / focused / 
selectable / selected states.
I would prefer to keep all the Blink fixes into one patch. I'll send 
another one with changes to the browser anyway, so it's not like we are 
done.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-03-30 19:51:07]

On 3/30/2016 3:04 PM, dmazzoni@chromium.org wrote:
> It might be good to split this into more than one change.
>
One way of splitting it is to have the event changes in one patch and 
the changes to the exposed states in another. But it's not as clear cut 
where the split should happen and I am not sure if the event changes 
will work correctly without the fixes to the focusable / focused / 
selectable / selected states.
I would prefer to keep all the Blink fixes into one patch. I'll send 
another one with changes to the browser anyway, so it's not like we are 
done.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2016-03-31 05:48:30]

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

https://codereview.chromium.org/1841333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:449: bool
AXLayoutObject::isFocused() const
We don't actually call isFocused from outside of Blink anymore. It doesn't make
sense to implement here because a single Blink frame doesn't actually know
whether the frame has focus or not, so this is really only telling you if it
thinks it's the focused item within the current frame, but native APIs should
only return TRUE for isFocused for the node that really has focus globally.

Either way, I think it might make sense to not follow activeDescendant in Blink.

https://codereview.chromium.org/1841333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:1156: if
(!getLayoutObject() || !getNode())
Hmmm, is there a reason we need a layoutObject? Maybe we should move this code
to AXNodeObject.

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

https://codereview.chromium.org/1841333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXListBoxOption.cpp:127: if
(ancestorExposesActiveDescendant())
This shouldn't be before some of the other checks, for example we should first
check if it's disabled.

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

https://codereview.chromium.org/1841333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:680: bool
AXObject::ancestorExposesActiveDescendant() const
To be correct, shouldn't we check the role of the ancestor too?

For example, if someone creates a role="grid" and then (incorrectly) puts a
role="option" inside it, that shouldn't return true, right?

https://codereview.chromium.org/1841333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:1009: case
TextFieldRole:
You deleted toolbar and some others, but the ARIA spec specifically lists
toolbar, row, and rowgroup. Please double-check the spec again:

https://rawgit.com/w3c/aria/master/aria/aria.html#aria-activedescendant

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

https://codereview.chromium.org/1841333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp:181: if
(obj->supportsActiveDescendant()) {
Hmmm, this code makes it seem like we were already following the active
descendant when determining what node has focus. Why was this not working?

[chromium-reviews, 2016-03-31 21:35:40]

Addressed comments.
Yes, we do fire the focus changed event on the new active descendant but 
when the screen reader asks for the current focus we return the DOM 
focus not the active descendant.
Event test added but it doesn't work. I think there is a bug in 
accessibilityController. Not all events that fire get logged it seems.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[blink-reviews, 2016-03-31 21:35:40]

Addressed comments.
Yes, we do fire the focus changed event on the new active descendant but 
when the screen reader asks for the current focus we return the DOM 
focus not the active descendant.
Event test added but it doesn't work. I think there is a bug in 
accessibilityController. Not all events that fire get logged it seems.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dmazzoni, 2016-04-01 07:18:39]

The active descendant notifications are working fine, the reason your test isn't
working is because the notifications are being queued up asynchronously, but
you're making all of the changes synchronously.

The first time you call setAttribute, it queues up an active descendant changed
event.

But before the timer even fires, you call setAttribute again twice, then remove
the attribute, then hide the widget.

So by the time the active descendant changed notification timer fires, the
object it was going to fire on has already disappeared, so it throws away the
event.

Instead what you need to do is call setAttribute, then wait for the active
descendant changed event before trying the next test. Then only hide the
elements on the page with display:none after the test has succeeded.

[blink-reviews, 2016-04-01 16:29:40]

Okay, I fixed the test by only hiding the containers when the test is 
successful.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-04-01 16:29:40]

Okay, I fixed the test by only hiding the containers when the test is 
successful.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nektarios, 2016-04-01 16:32:22]

Description was changed from

==========
Various fixes for aria-activedescendant.

1. Elements with an ARIA role and an ancestor that exposes an active descendant
should be marked focusable according to the ARIA Spec.
2. Elements that are the target of an active descendant should be marked focused
if the element that exposes the active descendant attribute is also focused.
3. If an active descendant is removed an event should fire.
4. According to the ARIA Spec, composite widgets, text boxes and ARIA groups
should be permitted to expose an active descendant. We were not permitting text
boxes to do that whilst we were permitting other widgets that should not have
been, such as toolbars.
5. ARIA list box options should be marked selectable in the accessibility API if
they have an ancestor that exposes an aria-activedescendant attribute.
6. ARIA list box options that are focused should also be marked as selected.
BUG=593646
R=dmazzoni@chromium.org
TESTED=layout test, manually with Jaws and NVDA
==========

to

==========
Various fixes for aria-activedescendant.

1. Elements with an ARIA role and an ancestor that exposes an active descendant
should be marked focusable according to the ARIA Spec.
2. Elements that are the target of an active descendant should be marked focused
if the element that exposes the active descendant attribute is also focused.
3. If an active descendant is removed an event should fire.
4. According to the ARIA Spec, composite widgets, text boxes and ARIA groups
should be permitted to expose an active descendant. We were not permitting text
boxes to do that whilst we were permitting other widgets that should not have
been, such as outlines.
5. ARIA list box options should be marked selectable in the accessibility API if
they have an ancestor that exposes an aria-activedescendant attribute.
6. ARIA list box options that are focused via aria-activedescendant should also
be marked as selected.
7. Focus and blur events should not fire from Bllink when the active descendant
changes or when it is removed. Only an active descendant changed event should
fire. The browser should handle any platform specific focus changed events and
not Blink.
BUG=593646
R=dmazzoni@chromium.org
TESTED=layout test, manually with Jaws and NVDA
==========

[dmazzoni, 2016-04-05 05:37:22]

Thanks for your patience. Really close, I have just two more requests.

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

https://codereview.chromium.org/1841333002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:161: // An
active descendant is only useful if it has a layoutObject, because that's what's
needed to post the notification.
You can post a notification on any AXObject, so this check shouldn't be needed.

https://codereview.chromium.org/1841333002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXObject.h (right):

https://codereview.chromium.org/1841333002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXObject.h:649: bool
ancestorExposesActiveDescendant() const;
This needs to be cached, check the other cached attributes in AXObject and how
they work. The cached values are reset whenever the tree changes in any way so
it's safe to cache. It saves us time when we scan the whole tree because we
don't have to keep walking up the ancestors of every node.

[blink-reviews, 2016-04-06 00:01:53]

All comments addressed.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-04-06 00:01:53]

All comments addressed.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2016-04-06 15:51:12]

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

https://codereview.chromium.org/1841333002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:690: for (const
AXObject* object = this; object; object = object->parentObjectUnignored()) {
Should this return true for itself or only for an ancestor? I'd expect it to
only return true if activeDescendant is set on an ancestor, but it actually
returns true for the object it's called on if that object has an
activeDescendant.

Also, I think it should call ancestorExposesActiveDescendant() recursively
because then you can take advantage of memoization and make it log(n) times
faster. So something like this:

    AXObject* parent = parentObjectUnignored();
    if (!parent)
        return false;

    if (parent->supportsActiveDescendant() &&
!parent->getAttribute(aria_activedescendantAttr).isEmpty())
        return true;

    return parent->ancestorExposesActiveDescendant();

[blink-reviews, 2016-04-06 18:12:22]

Done.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-04-06 18:12:23]

Done.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2016-04-07 04:27:47]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-04-07 04:27:48]

lgtm

[commit-bot: I haz the power, 2016-04-07 04:28:00]

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

[commit-bot: I haz the power, 2016-04-07 04:30:22]

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

[commit-bot: I haz the power, 2016-04-07 04:30:24]

Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[nektarios, 2016-04-07 20:46:06]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2016-04-07 20:46:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1841333002/#ps140001
(title: "Rebased with master.")

[commit-bot: I haz the power, 2016-04-07 20:47:15]

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

[commit-bot: I haz the power, 2016-04-07 21:29:14]

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

[commit-bot: I haz the power, 2016-04-07 21:29:16]

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

[nektarios, 2016-04-07 23:43:29]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2016-04-07 23:43:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1841333002/#ps160001
(title: "Skipped tests until browser side code is implementated.")

[commit-bot: I haz the power, 2016-04-07 23:43:48]

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

[commit-bot: I haz the power, 2016-04-08 01:03:03]

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

[commit-bot: I haz the power, 2016-04-08 01:03:04]

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

[nektarios, 2016-04-09 01:05:05]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2016-04-09 01:05:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1841333002/#ps180001
(title: "Fixed some Blink tests.")

[commit-bot: I haz the power, 2016-04-09 01:05:31]

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

[commit-bot: I haz the power, 2016-04-09 04:50:36]

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

[commit-bot: I haz the power, 2016-04-09 04:50:38]

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

[nektarios, 2016-04-11 19:59:07]

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

[commit-bot: I haz the power, 2016-04-11 19:59:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841333002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841333002/180001

[commit-bot: I haz the power, 2016-04-11 21:48:06]

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

[commit-bot: I haz the power, 2016-04-11 21:48:07]

Dry run: This issue passed the CQ dry run.

[nektarios, 2016-04-11 21:53:04]

The CQ bit was checked by nektar@chromium.org

[commit-bot: I haz the power, 2016-04-11 21:54:14]

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

[commit-bot: I haz the power, 2016-04-11 22:07:18]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-04-11 22:09:28]

Description was changed from

==========
Various fixes for aria-activedescendant.

1. Elements with an ARIA role and an ancestor that exposes an active descendant
should be marked focusable according to the ARIA Spec.
2. Elements that are the target of an active descendant should be marked focused
if the element that exposes the active descendant attribute is also focused.
3. If an active descendant is removed an event should fire.
4. According to the ARIA Spec, composite widgets, text boxes and ARIA groups
should be permitted to expose an active descendant. We were not permitting text
boxes to do that whilst we were permitting other widgets that should not have
been, such as outlines.
5. ARIA list box options should be marked selectable in the accessibility API if
they have an ancestor that exposes an aria-activedescendant attribute.
6. ARIA list box options that are focused via aria-activedescendant should also
be marked as selected.
7. Focus and blur events should not fire from Bllink when the active descendant
changes or when it is removed. Only an active descendant changed event should
fire. The browser should handle any platform specific focus changed events and
not Blink.
BUG=593646
R=dmazzoni@chromium.org
TESTED=layout test, manually with Jaws and NVDA
==========

to

==========
Various fixes for aria-activedescendant.

1. Elements with an ARIA role and an ancestor that exposes an active descendant
should be marked focusable according to the ARIA Spec.
2. Elements that are the target of an active descendant should be marked focused
if the element that exposes the active descendant attribute is also focused.
3. If an active descendant is removed an event should fire.
4. According to the ARIA Spec, composite widgets, text boxes and ARIA groups
should be permitted to expose an active descendant. We were not permitting text
boxes to do that whilst we were permitting other widgets that should not have
been, such as outlines.
5. ARIA list box options should be marked selectable in the accessibility API if
they have an ancestor that exposes an aria-activedescendant attribute.
6. ARIA list box options that are focused via aria-activedescendant should also
be marked as selected.
7. Focus and blur events should not fire from Bllink when the active descendant
changes or when it is removed. Only an active descendant changed event should
fire. The browser should handle any platform specific focus changed events and
not Blink.
BUG=593646
R=dmazzoni@chromium.org
TESTED=layout test, manually with Jaws and NVDA

Committed: https://crrev.com/1c568219380053695ef94b4372c4911df9c05c99
Cr-Commit-Position: refs/heads/master@{#386489}
==========

[commit-bot: I haz the power, 2016-04-11 22:09:29]

Patchset 10 (id:??) landed as
https://crrev.com/1c568219380053695ef94b4372c4911df9c05c99
Cr-Commit-Position: refs/heads/master@{#386489}