1986576 - Fields in Google Forms expose unavailable state when they shouldn't

Status: NEW

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

STR (with the NVDA screen reader):

1. Open a Google Form.
   • I'm not sure whether this occurs with all forms or just some. I have a link I can provide where this is 100% reproducible, but I'm not posting it here to avoid form spam. Please ask me if you need it.
2. Navigate through form fields by pressing f.
   • Expected: All the fields report as "unavailable".
   • Actual: The fields should not report "unavailable".

I would have thought this was fixed by bug 1899960, but it doesn't appear to be.

Comment 1

[Rich Caloggero]

Here is a simple test form:

https://docs.google.com/forms/d/e/1FAIpQLScF-y9v8g9vPYhcZQ5iquOuPCU3l_Kk0AWcdF8Cnu-6U6wtIw/viewform

which exhibits the behavior described for me with NVDA and latest Firefox.

Comment 2

[James Teh [:Jamie]]

This was quite challenging to track down.

Initially, Google sets disabled on the input, aria-disabled="true" on the input and aria-disabled="true" on an ancestor div. This is redundant and misguided, but not a spec violation.

Once the form loads, the following happens in this order:

1. aria-disabled is set to "false" on the input. We don't fire an event because the UNAVAILABLE state is still present due to the disabled attribute.
2. disabled is removed from the input. We get the new value of the UNAVAILABLE state by calling State(), which still implicitly includes the UNAVAILABLE state due to aria-disabled="true" being set on the ancestor. Thus, we fire a state change which enables the UNAVAILABLE state. That means that the input RemoteAccessible cache still includes the UNAVAILABLE state, even though it's not explicitly set there.
3. aria-disabled is removed from the div. We fire the correct state change on the div, which updates the div RemoteAccessible cache.
4. Because the input RemoteAccessible cache still explicitly (and incorrectly) includes the UNAVAILABLE state, we continue to expose that, even though there's no UNAVAILABLE state anywhere in reality.

Distilled test case:

data:text/html,<div id="div" aria-disabled="true"><input id="input" disabled></div><script>setTimeout(() => { input.disabled= false; div.ariaDisabled = "false"; }, 100);</script>

I'm not sure how to fix this yet. If we fire the state change for the disabled attribute based on the actual DOM disabled state while there is an implicit UNAVAILABLE state due to aria-disabled on an ancestor, that's okay for the RemoteAccessible cache, but it means we tell clients that the UNAVAILABLE state is going away when it isn't. We could compare between ExplicitState() and State(), but we still need some way to remove the explicit state from the RemoteAccessible cache without firing that event to a client.

Comment 3

[James Teh [:Jamie]]

Note that this also happens if there's no disabled attribute and only aria-disabled on the input and aria-disabled on the ancestor, with aria-disabled set to false on the input first:

data:text/html,<div id="div" aria-disabled="true"><input id="input" aria-disabled="true"></div><script>setTimeout(() => { input.ariaDisabled = "false"; div.ariaDisabled = "false"; }, 100);</script>

Comment 4

[James Teh [:Jamie]]

Eitan, I'd love your thoughts on the following two solutions:

1. Cache the explicit unavailable state as a separate boolean key. Always use that to determine whether the unavailable state is set or unset in RemoteAccessible::State before ApplyImplicitState.
2. Add an argument to PDocAccessible::StateChangeEvent specifying whether the state is explicitly enabled/disabled. aEnabled would be used to fire the event to clients, but aExplicitEnabled would be used to determine how the cache is updated. For the unavailable state, if aEnabled is true, we check ExplicitState(), and if the state isn't present there, aExplicitEnabled would be passed as false, thus removing the unavailable state from the RemoteAccessible cache.

Comment 5

[Eitan Isaacson [:eeejay]]

(In reply to James Teh [:Jamie] from comment #2)

2. disabled is removed from the input. We get the new value of the UNAVAILABLE state by calling State(), which still implicitly includes the UNAVAILABLE state due to aria-disabled="true" being set on the ancestor. Thus, we fire a state change which enables the UNAVAILABLE state. That means that the input RemoteAccessible cache still includes the UNAVAILABLE state, even though it's not explicitly set there.

This obviously is the bug, right? We should not be firing a state change for the input if its state isn't changing due to an ancestor with aria-disabled. Wouldn't suppressing this event solve the issue?

Comment 6

[James Teh [:Jamie]]

(In reply to Eitan Isaacson [:eeejay] from comment #5)

This obviously is the bug, right? We should not be firing a state change for the input if its state isn't changing due to an ancestor with aria-disabled. Wouldn't suppressing this event solve the issue?

No. The unavailable state is still present in the cache from the initial tree build, since the field itself has disabled and/or aria-disabled which are correctly included by ExplicitState(). So when aria-disabled is removed from the ancestor, the RemoteAccessible cache for the form fields will still include the unavailable state from the initial tree build.

That probably does lean more towards my proposed solution 1, though, since we ideally shouldn't fire a state change at all. Unless you have any better solutions?