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)
Core
Disability Access APIs
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)
59 bytes,
text/x-review-board-request
|
surkov
:
review+
jcristau
:
approval-mozilla-beta+
|
Details |
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 hidden (mozreview-request) |
Comment 2•6 years ago
|
||
mozreview-review |
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 3•6 years ago
|
||
Because I'm paranoid, filed a try run to run the tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed1b5da05fe8e37c9017b7c2a5bdba9ec92c720a
Comment 4•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 6•6 years ago
|
||
mozreview-review-reply |
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
Comment 8•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f52a56dc215c
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Comment 9•6 years ago
|
||
Verified fixed in Firefox 61.0a1 20180415220108 (Windows 64 bit).
Assignee | ||
Comment 10•6 years ago
|
||
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?
Updated•6 years ago
|
status-firefox59:
--- → wontfix
status-firefox60:
--- → affected
status-firefox-esr52:
--- → unaffected
Comment 11•6 years ago
|
||
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+
Comment 12•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/b33e36957105
Comment 13•6 years ago
|
||
(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
Assignee | ||
Comment 14•6 years ago
|
||
(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.
Comment 15•6 years ago
|
||
I see, that makes sense. Yura, do we have/should have a bug on this?
Flags: needinfo?(yzenevich)
Comment 16•6 years ago
|
||
(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)
Assignee | ||
Updated•6 years ago
|
Blocks: content-select
You need to log in
before you can comment on or make changes to this bug.
Description
•