1480058 - XUL tabs don't support ARIA selection

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement, P1)

Description

[James Teh [:Jamie]]

Multiple browser tabs can now be selected to perform operations on multiple tabs at once. We want to expose this selection for accessibility (bug 1476854). XUL tab/tabs don't natively support multiple selection, so this is all tabbrowser specific code. The intuitive approach to make this accessible is to use aria-selected and aria-multiselectable, as I suggested in that bug. Unfortunately, it turns out we don't support these on XUL tab/tabs elements.

A hacky way around this is to specify role="tab"/role="tablist", but really, even though there's obviously no mapping spec for XUL, I think it makes sense to implicitly map these to the tab/tablist roles. Besides, there are a few problems with just forcing the role:
1. We don't support aria-multiselectable on role tablist. The spec says we should, so I'll file a separate bug for this.
2. Even if 1) were fixed, XULSelectControlAccessible (the base class for XULTabsAccessible) overrides the selection methods to do XUL specific stuff and they don't handle ARIA. I think we can safely fall back to the base implementations if aria-multiselectable is present. Note that XUL tabs don't support nsIDOMXULMultiSelectControlElement anyway, so there's no conflict here.

Comment 1

[James Teh [:Jamie]]

Attachment: Bug 1480058 part 1: Expose states for aria-selected/aria-multiselectable on XUL tab/tabs.

Review commit: https://reviewboard.mozilla.org/r/261134/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261134/

Comment 2

[James Teh [:Jamie]]

Attachment: Bug 1480058 part 2: Add nsAccUtils::IsARIAMultiSelectable and make nsAccUtils::ISARIASelected const.

Review commit: https://reviewboard.mozilla.org/r/261136/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261136/

Comment 3

[James Teh [:Jamie]]

Attachment: Bug 1480058 part 3: Support accessible selection retrieval methods for XUL tabs with aria-multiselectable.

The XULSelectControlAccessible selection methods don't handle ARIA selection.
Therefore, if aria-multiselectable is set, use the base implementation of the selection retrieval methods.
We don't bother overriding the selection setting methods because implementations (e.g. browser tabs) don't support setting of aria-selected by the a11y engine and we still want to be able to set the primary selected item according to XUL.
Being able to retrieve multiple selection programmatically is far more important than being able to set it.

Review commit: https://reviewboard.mozilla.org/r/261138/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261138/

Comment 4

[Code Review Bot [:reviewbot]]

Comment on attachment 8997269 [details]
Bug 1480058 part 3: Support accessible selection retrieval methods for XUL tabs with aria-multiselectable.

https://reviewboard.mozilla.org/r/261138/#review268182


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: accessible/xul/XULTabAccessible.cpp:214
(Diff revision 1)
> +Accessible*
> +XULTabsAccessible::GetSelectedItem(uint32_t aIndex)
> +{
> +  if (nsAccUtils::IsARIAMultiSelectable(this)) {
> +    return AccessibleWrap::GetSelectedItem(aIndex);
> +  } else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

  } else {
    ^~~~~~

Comment 5

[James Teh [:Jamie]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfeba8fd2371dad42662f290e4f01143bab88bf7

Comment 6

[James Teh [:Jamie]]

Comment on attachment 8997269 [details]
Bug 1480058 part 3: Support accessible selection retrieval methods for XUL tabs with aria-multiselectable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261138/diff/1-2/

Comment 7

[James Teh [:Jamie]]

Ug. Sorry. Test failures I didn't anticipate. Another try run and then an updated patch if it passes.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b044e494d14c8274d9d16ef9e5dee35524106fd9

Comment 8

[James Teh [:Jamie]]

Comment on attachment 8997268 [details]
Bug 1480058 part 2: Add nsAccUtils::IsARIAMultiSelectable and make nsAccUtils::ISARIASelected const.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261136/diff/1-2/

Comment 9

[James Teh [:Jamie]]

Comment on attachment 8997269 [details]
Bug 1480058 part 3: Support accessible selection retrieval methods for XUL tabs with aria-multiselectable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261138/diff/2-3/

Comment 10

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #0)
> Multiple browser tabs can now be selected to perform operations on multiple
> tabs at once. We want to expose this selection for accessibility (bug
> 1476854). XUL tab/tabs don't natively support multiple selection, so this is
> all tabbrowser specific code. The intuitive approach to make this accessible
> is to use aria-selected and aria-multiselectable, as I suggested in that
> bug. Unfortunately, it turns out we don't support these on XUL tab/tabs
> elements.

I can see the use case, interesting way to workaround markup restrictions :) curious why they don't support multi-selection.

Comment 11

[alexander :surkov (:asurkov)]

Comment on attachment 8997267 [details]
Bug 1480058 part 1: Expose states for aria-selected/aria-multiselectable on XUL tab/tabs.

https://reviewboard.mozilla.org/r/261134/#review268246

::: accessible/tests/mochitest/states/test_aria.xul:54
(Diff revision 1)
>       </a>
> +     <a target="_blank"
> +         href="https://bugzilla.mozilla.org/show_bug.cgi?id=1480058"
> +         title="XUL tabs don't support ARIA selection">
> +        Mozilla Bug 1480058
> +     </a>

I don't add links to bugs lately, actually I even was removing the ones when touched the code. hg history provides all info and often it is more correct than links.

::: accessible/xul/XULTabAccessible.cpp:133
(Diff revision 1)
> +XULTabAccessible::ApplyARIAState(uint64_t* aState) const
> +{
> +  nsIContent* content = GetContent();
> +  if (!content) {
> +    return;
> +  }

it should be a safe assumption that content is not null, XUL tab accessible doens't exist without a DOM node.

::: accessible/xul/XULTabAccessible.cpp:138
(Diff revision 1)
> +  }
> +
> +  HyperTextAccessibleWrap::ApplyARIAState(aState);
> +
> +  // XUL tab has an implicit ARIA role of tab, so support aria-selected.
> +  aria::MapToState(aria::eARIASelectable, content->AsElement(), aState);

content->AsElement() -> Elm()

::: accessible/xul/XULTabAccessible.cpp:180
(Diff revision 1)
>  
> +void
> +XULTabsAccessible::ApplyARIAState(uint64_t* aState) const
> +{
> +  nsIContent* content = GetContent();
> +  if (!content) {

same here

::: accessible/xul/XULTabAccessible.cpp:188
(Diff revision 1)
> +
> +  XULSelectControlAccessible::ApplyARIAState(aState);
> +
> +  // XUL tabs has an implicit ARIA role of tablist, so support
> +  // aria-multiselectable.
> +  aria::MapToState(aria::eARIAMultiSelectable, content->AsElement(), aState);

and here

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 8997268 [details]
Bug 1480058 part 2: Add nsAccUtils::IsARIAMultiSelectable and make nsAccUtils::ISARIASelected const.

https://reviewboard.mozilla.org/r/261136/#review268258

::: accessible/base/nsAccUtils.cpp:264
(Diff revision 2)
> +  if (!aAccessible->GetContent()->IsElement())
> +    return false;
> +
> +  return aAccessible->GetContent()->AsElement()->
> +    AttrValueIs(kNameSpaceID_None, nsGkAtoms::aria_multiselectable,
> +                nsGkAtoms::_true, eCaseMatters);

dom::Element* el = aAccessible->Elm();
return el && el->AttrValueIs();

also you could combine these methods by a template method if you want.

Comment 13

[alexander :surkov (:asurkov)]

Comment on attachment 8997269 [details]
Bug 1480058 part 3: Support accessible selection retrieval methods for XUL tabs with aria-multiselectable.

https://reviewboard.mozilla.org/r/261138/#review268260

::: accessible/tests/mochitest/selectable/test_tabs.xul:29
(Diff revision 3)
> +    ////////////////////////////////////////////////////////////////////////////
> +    // Test
> +
> +    function doTest()
> +    {
> +      var id = "tabs_single";

pls use 'let' instead 'var', let is more strict in defining variable's scope.

::: accessible/tests/mochitest/selectable/test_tabs.xul:34
(Diff revision 3)
> +      var id = "tabs_single";
> +      ok(isAccessible(id, [nsIAccessibleSelectable]),
> +         "No selectable accessible for tabs_single");
> +      var select = getAccessible(id, [nsIAccessibleSelectable]);
> +
> +      testSelectableSelection(select, ["tab_single1"]);

nit: msg, here and below

::: accessible/tests/mochitest/selectable/test_tabs.xul:52
(Diff revision 3)
> +
> +      select.unselectAll();
> +      select.addItemToSelection(2); // tab_multi_xul2
> +      // We can only affect XUL selection, so ARIA selection won't change.
> +      testSelectableSelection(select, ["tab_multi_aria", "tab_multi_xul2"],
> +                              "select tab_multi_xul2: ");

Yeah, we can count it as a feature I guess, if you don't need it in the wilds, however it looks a bit weird - the author can set the selection by aria-selected, but AT user can't.

::: accessible/tests/mochitest/selectable/test_tabs.xul:68
(Diff revision 3)
> +    <body xmlns="http://www.w3.org/1999/xhtml">
> +      <a target="_blank"
> +        href="https://bugzilla.mozilla.org/show_bug.cgi?id=1480058"
> +        title="XUL tabs don't support ARIA selection">
> +       Mozilla Bug 1480058
> +      </a><br/>

nit: you don't have to put a link to the bug

::: accessible/xul/XULTabAccessible.cpp:138
(Diff revision 3)
>    }
>  
>    HyperTextAccessibleWrap::ApplyARIAState(aState);
>  
>    // XUL tab has an implicit ARIA role of tab, so support aria-selected.
> -  aria::MapToState(aria::eARIASelectable, content->AsElement(), aState);
> +  // Don't use aria::MapToState because that will set the SELECTABLE state

why you don't want to expose SELECTABLE state? because AT can't change its value by making the browser to change aria-selected, correct? It'd be good to capture this by a comment. It's not obvious.

::: accessible/xul/XULTabAccessible.cpp:195
(Diff revision 3)
>    // XUL tabs has an implicit ARIA role of tablist, so support
>    // aria-multiselectable.
>    aria::MapToState(aria::eARIAMultiSelectable, content->AsElement(), aState);
>  }
>  
> +// The XULSelectControlAccessible selection methods don't handle ARIA selection.

maybe:
XUL tabs is a single selection control and doesn't allow ARIA selection. However if aria-multiselectable is applied to the XUL tabs control, then it turns it into a multiselectable control, where ARIA and native markup are used to set selection.

::: accessible/xul/XULTabAccessible.cpp:199
(Diff revision 3)
>  
> +// The XULSelectControlAccessible selection methods don't handle ARIA selection.
> +// Therefore, if aria-multiselectable is set, use the base implementation of
> +// the selection retrieval methods.
> +// We don't bother overriding the selection setting methods because
> +// implementations don't support setting of aria-selected by the a11y engine

it is not very accurate statement, see https://dxr.mozilla.org/mozilla-central/source/accessible/generic/Accessible.cpp#714

Comment 14

[James Teh [:Jamie]]

Comment on attachment 8997267 [details]
Bug 1480058 part 1: Expose states for aria-selected/aria-multiselectable on XUL tab/tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261134/diff/1-2/

Comment 15

[James Teh [:Jamie]]

Comment on attachment 8997268 [details]
Bug 1480058 part 2: Add nsAccUtils::IsARIAMultiSelectable and make nsAccUtils::ISARIASelected const.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261136/diff/2-3/

Comment 16

[James Teh [:Jamie]]

Comment on attachment 8997269 [details]
Bug 1480058 part 3: Support accessible selection retrieval methods for XUL tabs with aria-multiselectable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261138/diff/3-4/

Comment 17

[James Teh [:Jamie]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dc4aea202f64630542ec9cdd9794c8d8310c8ea

Comment 18

[James Teh [:Jamie]]

Comment on attachment 8997269 [details]
Bug 1480058 part 3: Support accessible selection retrieval methods for XUL tabs with aria-multiselectable.

https://reviewboard.mozilla.org/r/261138/#review268260

> why you don't want to expose SELECTABLE state? because AT can't change its value by making the browser to change aria-selected, correct? It'd be good to capture this by a comment. It's not obvious.

Nothing to do with AT actually. We have a test that says that a disabled XUL tab should not have the selected state. Rather than changing the test and risking breakage elsewhere, I decided to just make the test pass. :)

Having said that, it could certainly be argued that a disabled control shouldn't have the selectable state, since it can't be selected when it's disabled. That's probably something to be addressed in the Core AAM, though.

> it is not very accurate statement, see https://dxr.mozilla.org/mozilla-central/source/accessible/generic/Accessible.cpp#714

When I said "implementations", I meant XUL tabs implementations (e.g. browser tabs) don't handle aria-selected being set externally (i.e. they don't watch for attribute mutations and alter internal state accordingly), not that our a11y engine can't handle doing it. I clarified the comment.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a0a9bd6c09de
part 1: Expose states for aria-selected/aria-multiselectable on XUL tab/tabs. r=surkov
https://hg.mozilla.org/integration/autoland/rev/0f3121479e3f
part 2: Add nsAccUtils::IsARIAMultiSelectable and make nsAccUtils::ISARIASelected const. r=surkov
https://hg.mozilla.org/integration/autoland/rev/ab8293506be2
part 3: Support accessible selection retrieval methods for XUL tabs with aria-multiselectable. r=surkov

Comment 20

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/a0a9bd6c09de
https://hg.mozilla.org/mozilla-central/rev/0f3121479e3f
https://hg.mozilla.org/mozilla-central/rev/ab8293506be2