[CTW] Stale <select> values reported with CTW enabled
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox106 | --- | fixed |
People
(Reporter: nlapre, Assigned: nlapre)
References
Details
(Whiteboard: [ctw-m3])
Attachments
(1 file)
Sometimes, a <select>
element will report a value other than the value that's actually selected. This only happens when accessibility.cache.enabled is true in about:config.
Steps to reproduce (tested on Windows 11, 106.0a1):
- Load
data:text/html,<select><option>First</option><option>Second</option></select>
. - Focus the combo box with the keyboard (i.e., tab to the combo box and press Enter). The combo box shows "First".
- Press the down arrow to select the next value down ("Second") without actually expanding the drop-down.
- Press Space or alt + down arrow to expand the combo box.
- Press the up arrow key once to select "First".
- Press Enter to confirm the selection.
Expected result: NVDA says "combo box First collapsed"
Actual result: NVDA says "combo box Second collapsed"
After the above steps, the actual selected value is "First". That shows up visually, too. However, the information given to NVDA appears stale.
Updated•2 years ago
|
Assignee | ||
Comment 1•2 years ago
|
||
The combo box value reported to NVDA is the first active combo box option. PivotStateRule::Match
relies on RemoteAccessibleBase::State
, which uses the cached 'active' value, which is incorrect/out-of-date in this buggy case. That value appears to be stale in the cache because it wasn't updated with a 'active' state change event. Specifically, this condition ensures we update combo box option active state when the combo box option list is collapsed. When the combo box is expanded, as it is in the reproduction steps above, we don't fire that 'active' state change event and therefore don't update the cache. A couple complicating factors: the parent process hosts the dropdown, and the dropdown is separate from the combo box in the accessibility tree.
I'm looking into what we can do on the content process side to ensure that we send the right events to update active state in the cache.
Assignee | ||
Comment 2•2 years ago
|
||
One more thing to mention on the diagnosis here: When responding to combo box value queries, the parent process checks states::ACTIVE
first to determine the right accessible, then falls back to checking states::SELECTED
if no 'active' accessible can be found. Also keep in mind that we only send 'active' state change updates for combo box options when the option list is collapsed (see implementation of AreItemsOperable
). These facts taken together mean that:
- If the user never changes the selected combo box option while the list is collapsed (i.e., they only ever change it while expanded), then the content process will never send an 'active' state change event for the combo box options, the parent process cache won't be updated, and
RemoteAccessibleBase::Value
will always fall back to searching forstate::SELECTED
. - If the user does change the selected combo box option while the list is collapsed, then the cached 'active' values for the combo box options won't change if the user subsequently changes the selected option while the list is expanded (this is the original bug).
The parent process does fire 'active' state change updates for combo box options, but those updates don't hit the cache.
I see three possible solutions:
- Modify the conditions for firing 'active' state changed events such that we fire them for combo box options even when the list is expanded.
- Always rely on
states::SELECTED
when getting the combo box value (and don't checkACTIVE
). - Make the parent process 'active' state changed events update the cache. I'm not sure if this is possible.
Eitan - it looks like you wrote most of this code so I figure you probably have an opinion on this. Do any of these solutions seem best to you? or something else?
Comment 3•2 years ago
|
||
We should weight for Eitan to comment here, but my thoughts below anyway:
(In reply to Nathan LaPré from comment #2)
- Modify the conditions for firing 'active' state changed events such that we fire them for combo box options even when the list is expanded.
This makes the most sense to me right now, assuming there's not something I'm missing.
- Always rely on
states::SELECTED
when getting the combo box value (and don't checkACTIVE
).
I originally thought this wasn't possible, as I thought there was some case where you could have a collapsed select with no selected option. However, I've just tried everything I can think of and there doesn't seem to be a way to do this. If you set the .selected attribute to false on a single option in a select, it just gets reset to true. Even removing the selected option causes another option to be selected automatically. So perhaps this could work... but I assume Eitan had some good reason for preferring the active state here.
- Make the parent process 'active' state changed events update the cache. I'm not sure if this is possible.
This is my least preferred solution. Even if we could find a way to reliably correlate the parent dropdown options with the content select options, I think this would be potentially very fragile.
Comment 4•2 years ago
|
||
(In reply to James Teh [:Jamie] from comment #3)
We should weight for Eitan to comment here, but my thoughts below anyway:
(In reply to Nathan LaPré from comment #2)
- Modify the conditions for firing 'active' state changed events such that we fire them for combo box options even when the list is expanded.
This makes the most sense to me right now, assuming there's not something I'm missing.
Yup. I agree this is the way to go.
- Always rely on
states::SELECTED
when getting the combo box value (and don't checkACTIVE
).I originally thought this wasn't possible, as I thought there was some case where you could have a collapsed select with no selected option. However, I've just tried everything I can think of and there doesn't seem to be a way to do this. If you set the .selected attribute to false on a single option in a select, it just gets reset to true. Even removing the selected option causes another option to be selected automatically. So perhaps this could work... but I assume Eitan had some good reason for preferring the active state here.
The wrong returned value in Value()
is only a symptom of us having a stale ACTIVE state. So while this solution might fix the symptom, the underlying issue will still persist.
- Make the parent process 'active' state changed events update the cache. I'm not sure if this is possible.
This is my least preferred solution. Even if we could find a way to reliably correlate the parent dropdown options with the content select options, I think this would be potentially very fragile.
Yeah, something not mentioned in your research and Jamie is only briefly alluding to is the fact that there are two drop-downs: the parent process one, that fires events ultimately originating from layout - and the content process one that only exists as markup (and i believe doesn't have layout frames). There is some IPC that happens when the parent process dropdown is interacted with. Some async messages are sent to the child process for it to update the DOM state.
Ideally you would ignore the parent process one and rely solely on the content process one since that is what we do right now.
Assignee | ||
Comment 5•2 years ago
|
||
Sweet, thanks! I'll put together a change to fire those 'active' state changed events for combo box options even when the drop-down is expanded.
Assignee | ||
Comment 6•2 years ago
|
||
Previously, we were not firing active state change events for the options in
<select> elements when the drop-down was expanded. This leads to possibly-stale
cached 'active' state in the parent process, which can cause Firefox to report
incorrect combo box values based on stale state. This revision addresses the
problem by more consistently firing 'active' state change events, even when
the drop-down is expanded. It also adds a test to verify this behavior and
modifies existing tests to reflect the changes.
Comment 7•2 years ago
|
||
(In reply to Eitan Isaacson [:eeejay] from comment #4)
- Modify the conditions for firing 'active' state changed events such that we fire them for combo box options even when the list is expanded.
Yup. I agree this is the way to go.
After some further reflection and discussion with Nathan, I'm no longer convinced this is the right path:
- The active state is inherently associated with focus. In the content process, options never get focused, even when the select is expanded.
- The active state is set based on CurrentItem. CurrentItem always returns null when the dropdown is collapsed. It has to do this because otherwise, FocusManager would treat the CurrentItem as the item which should have focus.
- Because of 2), the active state is never exposed on options in a collapsed select in the content process. This means the active state is out of sync between the two processes. While the CTW cache doesn't care about that, it's definitely not ideal.
- Always rely on
states::SELECTED
when getting the combo box value (and don't checkACTIVE
).The wrong returned value in
Value()
is only a symptom of us having a stale ACTIVE state. So while this solution might fix the symptom, the underlying issue will still persist.
Given the above, I don't think we should be exposing the active state on select options at all. We certainly don't in the content process. Again, the active state is only valid when the container delegates a11y focus to one of its children. A select combo box never does that (because the dropdown is handled in parent).
So, given all of that, I think the correct solution is to remove the active state event firing code here and instead rely entirely on selected for this case. Eitan, is there something I'm missing here which makes this incorrect; i.e. some other use of the active state that's important?
Yeah, something not mentioned in your research and Jamie is only briefly alluding to is the fact that there are two drop-downs: the parent process one, that fires events ultimately originating from layout - and the content process one that only exists as markup (and i believe doesn't have layout frames).
As noted on Slack, I completely forgot that the content process one doesn't have frames any more. That was a fairly recent change (bug 1744009). At that time, I filed bug 1745688 to remove these options from a11y too. It probably makes sense to do that separately, though, as it's a much bigger change.
Assignee | ||
Comment 9•2 years ago
|
||
Sweet, I'll update the change set to reflect this new approach. Thanks!
Updated•2 years ago
|
Comment 10•2 years ago
|
||
Pushed by nlapre@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c9287a7163f6 Rely on selection state to determine combo box value, r=Jamie
Comment 11•2 years ago
|
||
bugherder |
Description
•