1474108 - Convert listbox accessibility tests to use the "richlistbox" element

Status: RESOLVED FIXED

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

Description

[:Paolo Amadini]

The "listbox" element will be removed in bug 1472555, but the XULListboxAccessible class still applies to "richlistbox".

Comment 1

[:Paolo Amadini]

Attachment: Bug 1474108 - Convert listbox accessibility tests to use the "richlistbox" element.

This prepares for the removal of the "listbox" element and binding.

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

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 8990520 [details]
Bug 1474108 - Convert listbox accessibility tests to use the "richlistbox" element.

https://reviewboard.mozilla.org/r/255586/#review263374

elm/test_listbox.xul -> elm/test_richlistbox.xul and same change in the test wording please

[not yet finished, stopped at removed files]

::: accessible/tests/mochitest/attributes/test_obj_group.xul:154
(Diff revision 1)
> -  <listbox>
> -    <listitem label="listitem1" id="listitem1"/>
> -    <listitem label="listitem2" id="listitem2" type="checkbox"/>
> -    <listitem label="listitem3" id="listitem3" type="checkbox"/>
> -    <listitem label="listitem4" id="listitem4"/>
> -  </listbox>
> +  <richlistbox>
> +    <richlistitem id="listitem1"/>
> +    <richlistitem id="listitem2"><label value="listitem2"/></richlistitem>
> +    <richlistitem id="listitem3"/>
> +    <richlistitem id="listitem4"><label value="listitem4"/></richlistitem>
> +  </richlistbox>

it doesn't affect on the test, but the conversion is not equivalent

::: accessible/tests/mochitest/events/test_focus_listcontrols.xul
(Diff revision 1)
> -        </listitem>
> -        <listitem id="hlb_item2">
> -          <listcell label="Mary Ellen"/>
> -          <listcell label="Candle Maker"/>
> -        </listitem>
> -      </listbox>

I would convert it into richlistbox instead, the testing code of richlistbox and listbox don't look equivalent to safely remove the latter.

::: accessible/tests/mochitest/states/test_controls.xul:53
(Diff revision 1)
>      }
>  
>      var gQueue = null;
>      function doTest()
>      {
> +      document.getElementById("listbox-disabled").selectedIndex = -1;

does it affect on selected state? If so why "listbox" is different?

Comment 3

[:Paolo Amadini]

(In reply to alexander :surkov from comment #2)
> it doesn't affect on the test, but the conversion is not equivalent

Yes, I added some test coverage for elements with no label, while I removed the checkbox elements since they are not supported.

> I would convert it into richlistbox instead, the testing code of richlistbox
> and listbox don't look equivalent to safely remove the latter.

They differ because the functionality offered is different. As noted in bug 1472555 comment 46, multi-column support in "richlistbox" is absent, and list headers are used differently and only partially supported.

If we need better support for list headers in "richlistbox", I'd implement it with its own tests in a separate follow-up. The changes here are not removing any test coverage or functionality, because we already didn't use "listbox" with multi-column or list headers in production code.

> does it affect on selected state? If so why "listbox" is different?

The single-selection "richlistbox" element automatically selects the first element, which "listbox" didn't use to do.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to :Paolo Amadini from comment #3)
> (In reply to alexander :surkov from comment #2)
> > it doesn't affect on the test, but the conversion is not equivalent
> 
> Yes, I added some test coverage for elements with no label, while I removed
> the checkbox elements since they are not supported.
> 
> > I would convert it into richlistbox instead, the testing code of richlistbox
> > and listbox don't look equivalent to safely remove the latter.
> 
> They differ because the functionality offered is different. As noted in bug
> 1472555 comment 46, multi-column support in "richlistbox" is absent, and
> list headers are used differently and only partially supported.

fair enough

> If we need better support for list headers in "richlistbox", I'd implement
> it with its own tests in a separate follow-up. The changes here are not
> removing any test coverage or functionality, because we already didn't use
> "listbox" with multi-column or list headers in production code.
> 
> > does it affect on selected state? If so why "listbox" is different?
> 
> The single-selection "richlistbox" element automatically selects the first
> element, which "listbox" didn't use to do.

right, could you add SELECTED state for it since it's a default state of richlistbox (instead doing selectedIndex = -1)

Comment 5

[alexander :surkov (:asurkov)]

Comment on attachment 8990520 [details]
Bug 1474108 - Convert listbox accessibility tests to use the "richlistbox" element.

https://reviewboard.mozilla.org/r/255586/#review264418

Comment 6

[:Paolo Amadini]

Comment on attachment 8990520 [details]
Bug 1474108 - Convert listbox accessibility tests to use the "richlistbox" element.

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

Comment 7

[:Paolo Amadini]

(In reply to alexander :surkov from comment #4)
> right, could you add SELECTED state for it since it's a default state of
> richlistbox (instead doing selectedIndex = -1)

Ah, in doing this I found out that the item in the disabled richlistbox is "selected" but not "selectable", which was apparently not supported by the testStates helper.

Is this a valid state for accessibility? In practice, it's possible for one or more items to be selected in a disabled list, they would still be visually highlighted but there would be no way to change the selection. Maybe this could actually occur before, but was not tested anywhere.

See https://reviewboard.mozilla.org/r/255586/diff/1-2/ for the changes I made in the helper.

Comment 8

[alexander :surkov (:asurkov)]

yeah, I think this is a valid state: an item can be selected but be not selectable (i.e user can't select or unselect it) when a control is disabled

Comment 9

[alexander :surkov (:asurkov)]

(In reply to :Paolo Amadini from comment #7)

> See https://reviewboard.mozilla.org/r/255586/diff/1-2/ for the changes I
> made in the helper.

the change looks good

Comment 10

[:Paolo Amadini]

Thanks!

Comment 11

[:Paolo Amadini]

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/99869599a2f5
Convert listbox accessibility tests to use the "richlistbox" element. r=surkov

Comment 13

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/99869599a2f5