1476854 - [a11y] Expose whether a tab is (multi)selected for accessibility

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P1)

Description

[James Teh [:Jamie]]

When you select multiple tabs, the fact that a tab (other than the current one) is selected is only presented visually. It is not exposed semantically for accessibility.

This can be fixed by setting the aria-selected="true" attribute on selected tabs.
Also, aria-multiselectable="true" should probably be set on the tabs element (#tabbrowser-tabs) to indicate that multiple selection is supported.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1476854 - Expose whether tabs are multiselected for accessibility.

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

Comment 2

[James Teh [:Jamie]]

Thanks for working on this. The patch looks good. I started testing it, only to discover that aria-selected/aria-multiselectable don't get applied to XUL tab/tabs by the a11y code. I'm working on a patch to fix that. The front-end patch doesn't have the expected effect without the a11y fix.

Comment 3

[Dão Gottwald [:dao]]

Jamie, can you confirm that this patch works now?

Comment 4

[James Teh [:Jamie]]

Comment on attachment 8996445 [details]
Bug 1476854 - Expose whether tabs are multiselected for accessibility.

https://reviewboard.mozilla.org/r/260532/#review269044

Thanks for this. Sorry about the delay; I wanted to get the required a11y core fixes into central before this got landed so we didn't have potentially dead code floating about. The patch looks good to me, and I tested this against latest central and all works as expected. Note that it needs some rebase intervention, but it's pretty trivial.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8996445 [details]
Bug 1476854 - Expose whether tabs are multiselected for accessibility.

https://reviewboard.mozilla.org/r/260532/#review269068

::: browser/base/content/tabbrowser.xml:182
(Diff revision 1)
>          </setter>
>        </property>
>  
> +      <property name="_multiselectEnabled">
> +        <setter>
> +          // Can't use toggleAttribute since the value is needed.

It took me some time to understand this sentence.

Why is aria-multiselectable different from standard HTML attributes?

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Dão Gottwald [::dao] from comment #5)
> Comment on attachment 8996445 [details]
> Bug 1476854 - Expose whether tabs are multiselected for accessibility.
> 
> https://reviewboard.mozilla.org/r/260532/#review269068
> 
> ::: browser/base/content/tabbrowser.xml:182
> (Diff revision 1)
> >          </setter>
> >        </property>
> >  
> > +      <property name="_multiselectEnabled">
> > +        <setter>
> > +          // Can't use toggleAttribute since the value is needed.
> 
> It took me some time to understand this sentence.
> 
> Why is aria-multiselectable different from standard HTML attributes?

http://www.maxability.co.in/2015/06/aria-multiselectable-property/ and https://www.w3.org/TR/wai-aria-1.1/#aria-multiselectable

I'll redirect to Jamie.

Comment 7

[James Teh [:Jamie]]

(In reply to Dão Gottwald [::dao] from comment #5)
> It took me some time to understand this sentence.

It could perhaps be clarified by noting that ARIA requires literal "true"/"false" strings.

> Why is aria-multiselectable different from standard HTML attributes?

The use of "true"/"false" strings for boolean ARIA attributes is specified in the ARIA spec:
https://www.w3.org/WAI/PF/aria-1.1/appendices#typemapping
I'm not sure of the exact reasons, but I can take an educated guess:
1. ARIA isn't actually part of HTML. It works within HTML, but it's not part of HTML itself.
2. I believe ARIA was designed so it could theoretically be used in markup languages other than HTML.
At the end of the day, we need to do it this way because the spec says so. :)

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8996445 [details]
Bug 1476854 - Expose whether tabs are multiselected for accessibility.

https://reviewboard.mozilla.org/r/260532/#review269216

::: browser/base/content/tabbrowser.xml:187
(Diff revision 1)
> +          // Can't use toggleAttribute since the value is needed.
> +          if (val) {
> +            this.setAttribute("aria-multiselectable", "true");
> +          } else {
> +            this.removeAttribute("aria-multiselectable");
> +          }

Instead of the two cases, how about:

this.setAttribute("aria-multiselectable", !!val);

::: browser/base/content/tabbrowser.xml:191
(Diff revision 1)
> +            this.removeAttribute("aria-multiselectable");
> +          }
> +          return val;
> +        </setter>
> +        <getter>
> +          return this.hasAttribute("aria-multiselectable");

So this should really return this.getAttribute("aria-multiselectable") == "true";

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_selectedTabs.js:15
(Diff revision 1)
>        [PREF_MULTISELECT_TABS, true]
>      ]
>    });
>  
>    function testSelectedTabs(tabs) {
> +    ok(gBrowser.tabContainer.hasAttribute("aria-multiselectable"),

should check the actual value

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_selectedTabs.js:25
(Diff revision 1)
>      if (tabs.length == 1) {
>        ok(!selectedTab.multiselected, "Selected tab shouldn't be multi-selected because we are not in multi-select context yet");
>        ok(!_multiSelectedTabsSet.has(selectedTab), "Selected tab shouldn't be in _multiSelectedTabsSet");
>        is(selectedTabs.length, 1, "selectedTabs should contain a single tab");
>        is(selectedTabs[0], selectedTab, "selectedTabs should contain the selected tab");
> +      ok(!selectedTab.hasAttribute("aria-selected"),

here too, I guess?

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8996445 [details]
Bug 1476854 - Expose whether tabs are multiselected for accessibility.

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

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8996445 [details]
Bug 1476854 - Expose whether tabs are multiselected for accessibility.

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

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8996445 [details]
Bug 1476854 - Expose whether tabs are multiselected for accessibility.

https://reviewboard.mozilla.org/r/260532/#review269412

::: browser/base/content/tabbrowser.xml:182
(Diff revision 3)
>          </setter>
>        </property>
>  
> +      <property name="_multiselectEnabled">
> +        <setter>
> +          // Unlike HTML, the value of the attribute is used for ARIA.

This comment is possibly even more confusing, because some HTML attribute values are actually used for accessibility.

How about:

Unlike boolean HTML attributes, the value of boolean ARIA attributes actually matters.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8996445 [details]
Bug 1476854 - Expose whether tabs are multiselected for accessibility.

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c22115a387fe
Expose whether tabs are multiselected for accessibility. r=dao,Jamie

Comment 14

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/c22115a387fe