Closed Bug 1480058 Opened 4 years ago Closed 4 years ago

XUL tabs don't support ARIA selection

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: Jamie, Assigned: Jamie)

References

Details

Attachments

(3 files)

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.
Assignee: nobody → jteh
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 {
    ^~~~~~
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
(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 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
Attachment #8997267 - Flags: review?(surkov.alexander) → review+
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.
Attachment #8997268 - Flags: review?(surkov.alexander) → review+
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
Attachment #8997269 - Flags: review?(surkov.alexander) → review+
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.
Pushed by jteh@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.