Closed Bug 1453555 Opened 6 years ago Closed 6 years ago

[e10s a11y] Fix group position for <select size="1">

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- wontfix
firefox60 --- fixed
firefox61 --- verified

People

(Reporter: Jamie, Assigned: Jamie)

References

Details

(Keywords: regression)

Attachments

(1 file)

STR:
1. Open this URl:
data:text/html,<select size="1"><option>a</option><option>b</option></select>
2. Right click the combo box and choose Inspect Accessibility Properties.
3. In the accessibility tree, expand the combobox, expand the combobox list and select combobox option:"a".
4. In the accessibility properties, expand attributes and look at the setsize attribute.
Expected: 2
Actual: 1

This works as expected if you disable e10s.

This originally came up with regard to the prototype implementation of <select> dropdowns in the content process (bug 1421229 comment 24). It came up again with regard to JAWS handling of <select>. There's some uncertainty  at present as to whether JAWS handling of <select> will change; there's other brokenness outside the scope of this bug. However, if bug 1421229 really happens, we'll need this fixed anyway. And aside from anything else, it's broken behaviour.
Comment on attachment 8967269 [details]
Bug 1453555: Fix accessibility group info for <select size="1"> options.

https://reviewboard.mozilla.org/r/235954/#review241714

::: accessible/html/HTMLSelectAccessible.cpp:201
(Diff revision 1)
>    } else if (selectState & states::COLLAPSED) {
>      // <select> is COLLAPSED: add OFFSCREEN, if not the currently
>      // visible option
>      if (!selected) {
>        state |= states::OFFSCREEN;
> -      state ^= states::INVISIBLE;
> +      // Ensur the invisible state is removed. Otherwise, group info will skip

Nit: ensure (missing e at the end)
Comment on attachment 8967269 [details]
Bug 1453555: Fix accessibility group info for <select size="1"> options.

https://reviewboard.mozilla.org/r/235954/#review241862

::: accessible/html/HTMLSelectAccessible.cpp:204
(Diff revision 1)
>      if (!selected) {
>        state |= states::OFFSCREEN;
> -      state ^= states::INVISIBLE;
> +      // Ensur the invisible state is removed. Otherwise, group info will skip
> +      // this option. Furthermore, this gets cached and this doesn't get
> +      // invalidated even once the select is expanded.
> +      state &= ~states::INVISIBLE;

r=me since it fixes the bug and I don't have any concerns about the fix. Not sure I follow Trev's thinking that ^= is clearer, see bug 907682#c15, but anyway &=~ seems appropriate here. However I don't get why e10s is special in case of invisible state handling, so it'd be nice to leave a coment for that.
Attachment #8967269 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8967269 [details]
Bug 1453555: Fix accessibility group info for <select size="1"> options.

https://reviewboard.mozilla.org/r/235954/#review241862

> r=me since it fixes the bug and I don't have any concerns about the fix. Not sure I follow Trev's thinking that ^= is clearer, see bug 907682#c15, but anyway &=~ seems appropriate here. However I don't get why e10s is special in case of invisible state handling, so it'd be nice to leave a coment for that.

Honestly, I'm not entirely sure why e10s is special regarding the invisible state either. The rendering of select dropdowns changed significantly with e10s, so I guess that must have changed the way layout exposes the stuff we use to calculate visibility.

I didn't end up adding a comment because the difference between e10s and non-e10s is irrelevant when we use "&= ~", so the comment applies to the old code, not the new code. (I did explain this in the commit message, though.) The point is that we want to ensure the invisible state is *removed*; we never want to *add* it, even if it wasn't there before. "^=" and "&=~" are subtly different. "^=" flips the bits, adding them if they weren't there and removing them if they were, whereas "= ~" ensures they are removed, regardless of their previous state.
Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f52a56dc215c
Fix accessibility group info for <select size="1"> options. r=surkov
https://hg.mozilla.org/mozilla-central/rev/f52a56dc215c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Verified fixed in Firefox 61.0a1 20180415220108 (Windows 64 bit).
Comment on attachment 8967269 [details]
Bug 1453555: Fix accessibility group info for <select size="1"> options.

Approval Request Comment
[Feature/Bug causing the regression]: E10s Windows accessibility.
[User impact if declined]: Certain assistive technology products will not be able to determine the position of the item or number of items in a <select> element.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Trivial, clear patch with very narrow scope.
[String changes made/needed]: None.
Attachment #8967269 - Flags: approval-mozilla-beta?
Comment on attachment 8967269 [details]
Bug 1453555: Fix accessibility group info for <select size="1"> options.

a11y regression fix, approved for 60.0b13
Attachment #8967269 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to James Teh [:Jamie] from comment #6)

> I didn't end up adding a comment because the difference between e10s and
> non-e10s is irrelevant when we use "&= ~", so the comment applies to the old
> code, not the new code. (I did explain this in the commit message, though.)
> The point is that we want to ensure the invisible state is *removed*; we
> never want to *add* it, even if it wasn't there before. "^=" and "&=~" are
> subtly different. "^=" flips the bits, adding them if they weren't there and
> removing them if they were, whereas "= ~" ensures they are removed,
> regardless of their previous state.

Agreed, commit message is enough.

Don't you know btw, why states/test_selects.html tests doesn't catch this case? https://dxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/states/test_selects.html?q=test_selects.html&redirect_type=direct#76
(In reply to alexander :surkov from comment #13)
> Don't you know btw, why states/test_selects.html tests doesn't catch this
> case?

I asked the same question about attributes/test_obj_group.html. Yura said he thinks our a11y mochitests run with the document in the parent process, not in the content process, so this behaviour wouldn't trigger. Ideally, we really want to run the HTML mochitests in the content process, since that's going to be the state of things going forward.
I see, that makes sense. Yura, do we have/should have a bug on this?
Flags: needinfo?(yzenevich)
(In reply to alexander :surkov from comment #15)
> I see, that makes sense. Yura, do we have/should have a bug on this?

well that means re-writing our mochitests as browser chrome.
Flags: needinfo?(yzenevich)
You need to log in before you can comment on or make changes to this bug.