Correctly fire active descendant changes for more roles

Correctly fire active descendant changes for more roles

1. Use roleValue rather than ariaRoleAttribute when checking which roles support active descendant.

The method itself should probably be named supportsOnActiveDescendantChangedEvent since the active descendant was still being exposed for popUpButton.

2. add popUpButtonRole to the supported list.

As a result of 1 and 2,
<button aria-haspopup="true"></button>
<div role="button" aria-haspopup="true"</div>

both work.

TEST=chromevox_tests --gtest_filter=BackgroundTest.ActiveDescendant*

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

[David Tseng, 2016-06-15 17:33:12]

Description was changed from

==========
Fix firing of active descendant change.
==========

to

==========
Fix firing of active descendant change.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[David Tseng, 2016-06-15 17:37:48]

Description was changed from

==========
Fix firing of active descendant change.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Correctly fire active descendant changes for more roles

1. Use roleValue rather than ariaRoleAttribute when checking which roles support
active descendant.

The method itself should probably be named
supportsOnActiveDescendantChangedEvent since the active descendant was still
being exposed for popUpButton.

2. add popUpButtonRole to the supported list.

As a result of 1 and 2,
<button aria-haspopup="true"></button>
<div role="button" aria-haspopup="true"</div>

both work.

TEST=chromevox_tests --gtest_filter=BackgroundTest.ActiveDescendant*

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[David Tseng, 2016-06-15 17:37:49]

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

[dmazzoni, 2016-06-15 19:15:07]

lgtm but it looks like you have the menuitemcheckbox
patch in here too

https://codereview.chromium.org/2072503002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
(left):

https://codereview.chromium.org/2072503002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:234:
earconId: 'BUTTON'
This looks like part of a previous patch?

[David Tseng, 2016-06-15 20:12:05]

On 2016/06/15 19:15:07, dmazzoni wrote:
> lgtm but it looks like you have the menuitemcheckbox
> patch in here too
> 
>
https://codereview.chromium.org/2072503002/diff/1/chrome/browser/resources/ch...
> File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
> (left):
> 
>
https://codereview.chromium.org/2072503002/diff/1/chrome/browser/resources/ch...
> chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:234:
> earconId: 'BUTTON'
> This looks like part of a previous patch?
Yup; removed; thanks!

[David Tseng, 2016-08-11 00:43:51]

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

[commit-bot: I haz the power, 2016-08-11 00:44:54]

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, 2016-08-11 00:44:55]

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

[commit-bot: I haz the power, 2016-08-11 00:44:55]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[David Tseng, 2016-08-16 01:50:45]

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

[commit-bot: I haz the power, 2016-08-16 01:51:08]

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, 2016-08-16 01:51:14]

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

[commit-bot: I haz the power, 2016-08-16 01:51:14]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[dmazzoni, 2016-08-22 19:23:56]

lgtm

[nektarios, 2016-08-23 15:21:49]

Sorry but I am not sure about this. Could we have another look?
The PopupButtonRole is applied to buttons that have the aria-popup attribute.
According to the ARIA Spec it's not the button itself that should be the
container of the menu, listbox or dialog that will pop up, but another element
having these roles. Since we already say that ListBoxRole and MenuRole can have
an active descendant, doesn't this cover the PopupButtonRole as well, since the
aria-activedescendant attribute should be on the contained listbox or menu and
not on the button itself.
Also, if I were to make some corrections I would do the following:
1. Add ApplicationRole. It's mentioned in the ARIA Spec as supporting active
descendant.
2. Add MenuListPopupRole and modify the comment above the switch to say that
"select" elements also implicitly have an active descendant.

[nektarios, 2016-08-23 15:24:55]

BTW, I think the bug that prompted this change is an issue with Drive's New
popup menu that wasn't working?
I tested it just now and it seems to work. Perhaps another change fixed it
already.

[David Tseng, 2016-08-23 18:37:52]

Which platform? It didn't work on Mac or Chrome OS. Regardless, we should
be firing the event whereever we allow an active descendant to be set. That
list is more than a node with an aria role.
On Tue, Aug 23, 2016 at 8:24 AM <nektar@chromium.org> wrote:

> BTW, I think the bug that prompted this change is an issue with Drive's New
> popup menu that wasn't working?
> I tested it just now and it seems to work. Perhaps another change fixed it
> already.
>
>
> https://codereview.chromium.org/2072503002/
>
> --
> 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.
>

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

[David Tseng, 2016-08-23 18:37:53]

Which platform? It didn't work on Mac or Chrome OS. Regardless, we should
be firing the event whereever we allow an active descendant to be set. That
list is more than a node with an aria role.
On Tue, Aug 23, 2016 at 8:24 AM <nektar@chromium.org> wrote:

> BTW, I think the bug that prompted this change is an issue with Drive's New
> popup menu that wasn't working?
> I tested it just now and it seems to work. Perhaps another change fixed it
> already.
>
>
> https://codereview.chromium.org/2072503002/
>
> --
> 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.
>

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