1899960 - Incorrect a11y states on focusable controls when aria-disabled on ancestor is changed

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

Spun off bug 1896367 comment 11.

STR (with a screen reader):

1. Open this test case:
   data:text/html,<div id="group" role="group" aria-disabled="true"><input></div><button onclick="group.ariaDisabled = (group.ariaDisabled == 'true') ? 'false' : 'true';">Toggle
2. Press the Toggle button.
3. Tab to the text box.
   • Expected: The screen reader should not report it as unavailable/disabled.
   • Actual: The screen reader reports it as unavailable/disabled.

I fixed this for the HTML disabled attribute on fieldsets in bug 1896367. However, I neglected to realise that aria-disabled also gets propagated to focusable descendants. This is per spec, not specific to Gecko, so we do need to handle it correctly.

I think we might need to move that logic out of LocalAccessible::ApplyARIAState and into Accessible::ApplyImplicitState. Otherwise, we'll have to walk through all focusable descendants and fire state change events or push cache changes for them, which is very much not ideal.

Copying you Eitan because I think you worked on aria-activedescendant state stuff for CtW.

Comment 1

[James Teh [:Jamie]]

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

Otherwise, we'll have to walk through all focusable descendants and fire state change events or push cache changes for them, which is very much not ideal.

I think we'll need to do that anyway because otherwise, Windows screen reader virtual buffers won't be updated and will continue to report disabled unless the user forces a buffer refresh.

Comment 3

[Eitan Isaacson [:eeejay]]

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

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

Otherwise, we'll have to walk through all focusable descendants and fire state change events or push cache changes for them, which is very much not ideal.

I think we'll need to do that anyway because otherwise, Windows screen reader virtual buffers won't be updated and will continue to report disabled unless the user forces a buffer refresh.

So we have to have actual focusable state changes?

Comment 4

[James Teh [:Jamie]]

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

So we have to have actual focusable state changes?

For MSAA/IA2, there is only a singlestate change event, so it wouldn't matter whether it was the disabled state or the focusable state. However, if we wanted buffers to pick up the change, we'd need to fire at least one of those on each control.

That said:

1. I do worry that walk would be expensive and potentially generate a lot of events. I think this particular technique is used pretty rarely, but if we are super worried about that, we could try to get AT vendors to handle a state change on role="group" themselves.
2. Chromium has bugs here too. It doesn't fire state changes and the cached state is stale. However, if you focus one of the controls, it fires a state change and the cached state is updated. That's not to suggest we shouldn't fix this, but we might have some wiggle room in terms of how we choose to do it.

Comment 5

[Eitan Isaacson [:eeejay]]

Yeah. I'm worried how this would scale. I would also solve this very differently if we just want to update the state vs. emit an event. (In reply to

James Teh [:Jamie] from comment #4)

2. Chromium has bugs here too. It doesn't fire state changes and the cached state is stale. However, if you focus one of the controls, it fires a state change and the cached state is updated. That's not to suggest we shouldn't fix this, but we might have some wiggle room in terms of how we choose to do it.

I guess that works because setting the aria-disabled state does not actually affect the focusability, so tabbing to the control even when it is "unavailable" would still work.. I really don't like that.

When you say windows screen reader virtual buffers - do you mean all of them? jaws/nvda/etc? Or is there one we have in mind?

Comment 6

[James Teh [:Jamie]]

I've only tested with NVDA. I assume the same problem applies to JAWS, but I can't be sure of that.

Comment 7

[Eitan Isaacson [:eeejay]]

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

Copying you Eitan because I think you worked on aria-activedescendant state stuff for CtW.

For the record, we never solved this for aria-activedescendant either. If you remove that attribute from a container, all the children list items keep their focusable state.

Both for that case and this one, I am very wary of firing state changes for the simple reason that we have no control over the scale of those kinds of sets, I could see a case where a spreadsheet/grid gets aria-disabled and we fire thousands of state changes. If we lazily update the state with ApplyImplicitState, at least it would be similar to the Chrome behavior.

Comment 8

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1899960 - P1: Don't cache implicit states. r?Jamie

THis also removes some duplication and unifies the logic we use for
states that are calculated from other ones.

Comment 9

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1899960 - P2: Imply an unavailable state if focusable is in unavailable subtree. r?Jamie

I decided not to use aria-disabled directly because we currently don't
cache it in the parent, and I don't think the benefit would be great
enough.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/87b43339e3eb
P1: Don't cache implicit states. r=Jamie
https://hg.mozilla.org/integration/autoland/rev/29814a28732e
P2: Imply an unavailable state if focusable is in unavailable subtree. r=Jamie

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0bbfbdfab506
Revert "Bug 1899960 - P2: Imply an unavailable state if focusable is in unavailable subtree. r=Jamie" for causing Geckoview failures.

Comment 12

[Atila Butkovits]

Backed out for causing Geckoview failures.

Backout link: https://hg-edge.mozilla.org/integration/autoland/rev/0bbfbdfab506d72a2727ebab3af55156088fcd65

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&searchStr=android%2C7.0%2Cx86-64%2Copt%2Ctest-android-em-7.0-x86_64-qr%2Fopt-geckoview-junit-nofis-noship%2Cgv-junit-nofis-noship&revision=29814a28732ef4900890f7bf10ad9bdc7c2b36df&selectedTaskRun=D8T3pfZlTmWSccmvrl5XuQ.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=509701782&repo=autoland&lineNumber=1615

Comment 13

[Aaron Train [:aaronmt]]

Assuming the same Gecko crash as above on all Fenix/Focus UI-test jobs, browser seems to crash shortly after content load when looking at the videos on Firebase Test Lab - which affects majority of our tests.

https://treeherder.mozilla.org/jobs?repo=autoland&searchStr=ui-&revision=29814a28732ef4900890f7bf10ad9bdc7c2b36df

Testable on try via --preset firefox-android

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d0be1c953049
P1: Don't cache implicit states. r=Jamie
https://hg.mozilla.org/integration/autoland/rev/3287c0cad179
P2: Imply an unavailable state if focusable is in unavailable subtree. r=Jamie

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e376b5fc2e7
Revert "Bug 1899960 - P2: Imply an unavailable state if focusable is in unavailable subtree. r=Jamie" for casuing junit failures.

Comment 16

[Aaron Train [:aaronmt]]

Hello, this re-land attempt is still crashing Geckoview in over 300+ UI tests for Fenix/Focus/GeckoView Example tests on Firebase Test Lab on CI

https://treeherder.mozilla.org/jobs?repo=autoland&searchStr=ui-&revision=198df8d7a5a47e7a97d4fc4f4eca81b9c6e900f3

Example APK: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/GYsWIHRvSDGj6mvVTETJOg/artifacts/public/build/target.arm64-v8a.apk

Operating system: Android
                  0.0.0 Linux 6.1.23-android14-4-00257-g7e35917775b8-ab9964412 #1 SMP PREEMPT Mon Apr 17 20:50:58 UTC 2023 aarch64
CPU: arm64
     4 CPUs

Crash reason:  SIGSEGV / SEGV_MAPERR
Crash address: 0x0000000000007230
Process uptime: not available

Linux memory map count: 4810

Thread 0 AndroidUI (crashed) - tid: 9531
 0  libxul.so!mozilla::CycleCollectedJSContext::Context() const [CycleCollectedJSContext.h:e22c6ee37f2a9a8dac1fe933a0035bc887c788eb : 188]
    Found by: inlining
 1  libxul.so!mozilla::dom::danger::GetJSContext() [ScriptSettings.cpp:e22c6ee37f2a9a8dac1fe933a0035bc887c788eb : 263 + 0x0]
      x0 = 0x0000000000000000     x1 = 0x0000007ac105f680
      x2 = 0x0000000000000000     x3 = 0x0000007ad0e57fa4
      x4 = 0x0000007fe7e47c70     x5 = 0x00000000656c646e
      x6 = 0x00000000656c646e     x7 = 0x0000007ad9d11e4f
      x8 = 0x0000007e2862f300     x9 = 0x0000007e2862f040
     x10 = 0x0000000000000001    x11 = 0x0000000000000000
     x12 = 0x0000000000000100    x13 = 0x0000007ac8ae6708
     x14 = 0x000000008d44e65a    x15 = 0x00000000ebad6a89
     x16 = 0x0000007ac8c60b50    x17 = 0x0000007e2372b280
     x18 = 0x0000000000000000    x19 = 0x0000007ac105f680
     x20 = 0x0000007b4105fd80    x21 = 0x00000079d8e0d0e0
     x22 = 0x0000007e2862f000    x23 = 0x0000000000000000
     x24 = 0x0000007e2862f000    x25 = 0x0003001100000000
     x26 = 0x000000000000003f    x27 = 0x0000007b42fe7b78
     x28 = 0x0000007fe7e47d80     fp = 0x0000007fe7e471f0
      lr = 0x0000007ac5b1652c     sp = 0x0000007fe7e471f0
      pc = 0x0000007ac5b1652c
    Found by: given as instruction pointer in context
 2  libxul.so!mozilla::dom::CustomElementRegistry::CallGetCustomInterface(mozilla::dom::Element*, nsID const&) [CustomElementRegistry.cpp:e22c6ee37f2a9a8dac1fe933a0035bc887c788eb : 1419 + 0x0]
     x19 = 0x0000007ac105f680    x20 = 0x0000007b4105fd80
     x21 = 0x00000079d8e0d0e0    x22 = 0x0000007e2862f000
     x23 = 0x0000000000000000    x24 = 0x0000007e2862f000
     x25 = 0x0003001100000000    x26 = 0x000000000000003f
     x27 = 0x0000007b42fe7b78    x28 = 0x0000007fe7e47d80
      fp = 0x0000007fe7e472b0     sp = 0x0000007fe7e47200
      pc = 0x0000007ac3b44c44
    Found by: call frame info
 3  libxul.so!mozilla::dom::Element::GetCustomInterface<nsIDOMXULControlElement>(nsGetterAddRefs<nsIDOMXULControlElement>) [Element.cpp:e22c6ee37f2a9a8dac1fe933a0035bc887c788eb : 4745]
    Found by: inlining
 4  libxul.so!mozilla::dom::Element::AsXULControl() [Element.cpp:e22c6ee37f2a9a8dac1fe933a0035bc887c788eb : 4920 + 0x10]
     x19 = 0x00000079dc8878c0    x20 = 0x0000007e2862f000
     x21 = 0x00000079d8e0d0e0    x22 = 0x0000007e2862f000
     x23 = 0x0000000000000000    x24 = 0x0000007e2862f000
     x25 = 0x0003001100000000    x26 = 0x000000000000003f
     x27 = 0x0000007b42fe7b78    x28 = 0x0000007fe7e47d80
      fp = 0x0000007fe7e47300     sp = 0x0000007fe7e472f0

(Full trace in attachment)

Null deref at

Thread 0 AndroidUI (crashed)
libxul.so!mozilla::CycleCollectedJSContext::Context() const
[CycleCollectedJSContext.h:188]

libxul.so!mozilla::dom::danger::GetJSContext() 
[ScriptSettings.cpp:263]

From

mozilla::a11y::SessionAccessibility::GetNodeInfo(...)

Comment 17

[Serban Stanca [:SerbanS]]

Backed out for causing junit failures.

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | runjunit.py | Some tests did not run (probably due to a crash in the harness)

Please also check these Android Components Ui tests failures. Failure log.

Comment 18

[Aaron Train [:aaronmt]]

Attachment: crash-trace.txt

Comment 19

[Donal Meehan [:dmeehan]]

Comment on attachment 9489528 [details]
Bug 1899960 - P1: Don't cache implicit states. r?Jamie

Removing beta uplift request flag, added due to Bug 1844878

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/eacb22ed13fd
https://hg.mozilla.org/integration/autoland/rev/f73706c72bb6
P1: Don't cache implicit states. r=Jamie
https://github.com/mozilla-firefox/firefox/commit/6e4b35e663c2
https://hg.mozilla.org/integration/autoland/rev/34bb96ae4e09
P2: Imply an unavailable state if focusable is in unavailable subtree. r=Jamie

Comment 21

[Cristina Horotan [:chorotan]]

Backed out fir causing ba failures

Backout link

Push with failures

Failure log

Comment 22

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/f73706c72bb6
https://hg.mozilla.org/mozilla-central/rev/34bb96ae4e09

Comment 23

[Mathew Hodson]

This was backed out

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/940e0c0106c5
https://hg.mozilla.org/integration/autoland/rev/a0633e317fea
P1: Don't cache implicit states. r=Jamie
https://github.com/mozilla-firefox/firefox/commit/b6ffead8a73b
https://hg.mozilla.org/integration/autoland/rev/a7a0bc87fcde
P2: Imply an unavailable state if focusable is in unavailable subtree. r=Jamie

Comment 25

[Serban Stanca [:SerbanS]]

Reverted this because it was causing mochitests failures in browser_simplePatterns.js.

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | accessible/tests/browser/windows/uia/browser_simplePatterns.js | Uncaught exception in test bound uiaTask - at chrome://mochitests/content/browser/accessible/tests/browser/shared-head.js:1084 - Error: Traceback (most recent call last):

Comment 26

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/a0633e317fea
https://hg.mozilla.org/mozilla-central/rev/a7a0bc87fcde

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/433172e96d0f
https://hg.mozilla.org/integration/autoland/rev/57bb7d23082e
P1: Don't cache implicit states. r=Jamie
https://github.com/mozilla-firefox/firefox/commit/d1d75407f77b
https://hg.mozilla.org/integration/autoland/rev/1734449ae32d
P2: Imply an unavailable state if focusable is in unavailable subtree. r=Jamie

Comment 28

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/57bb7d23082e
https://hg.mozilla.org/mozilla-central/rev/1734449ae32d