569652 - Make sure aria-activedescendant changes can cause only async events

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[David Bolter [:davidb] (NeedInfo me for attention)]

nsDocAccessible::ARIAAttributeChanged should only fire delayed events since it is called in our nsIMutationObserver handling.

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: make nsRootAccessible::FireAccessibleFocusEvent safe

After some code inspection I realized the best fix is to make nsRootAccessible::FireAccessibleFocusEvent only fire async/delayed events. This allows the code that checks for aria-activedescendant to exist in one place, and alleviates the need for any specialized nsAccEvent for this particular case.

So I made the menu start event fire async, which is probably a good idea anyways since the menu end event is async, and so this should preserve the ordering better.

I'll post a link to try for Marco when I see it and I will report any mochitest failures.

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-a537fbf987aa/

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

Marco, mainly we need to make sure that menu start events still work.

Comment 4

[Marco Zehe (:MarcoZ)]

Comment on attachment 450551 [details] [diff] [review]
make nsRootAccessible::FireAccessibleFocusEvent safe

All Windows screen readers work fine with this try-server build. Menus, sub menus etc. all still are announced as opening and closing like they do in a regular nightly.

Comment 5

[alexander :surkov (:asurkov)]

Comment on attachment 450551 [details] [diff] [review]
make nsRootAccessible::FireAccessibleFocusEvent safe


>-      if (rootAcc)
>+      if (rootAcc) {
>+        // We can call this method safely because it only fires delayed events.
>         rootAcc->FireAccessibleFocusEvent(nsnull, currentFocus, nsnull, PR_TRUE);

this is unnecessary change I think. "Implementors: " comment inside the method should be enough.
>+            if (menuStartEvent) {
>+              // We must fire this delayed in case it came from a menu that
>+              // uses aria-activedescendant to denote fake menu item focus,
>+              // since in this case we might be unsafe.

this comment must be unnecessary as well.

please add mochitest while you're here ;)

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #4)
> (From update of attachment 450551 [details] [diff] [review])
> All Windows screen readers work fine with this try-server build. Menus, sub
> menus etc. all still are announced as opening and closing like they do in a
> regular nightly.

Thanks Marco!

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

No worrisome errors reported by try server.

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: test (try fail - don't check in)

(In reply to comment #5)

Test coverage for the related change.

Comment 9

[Marco Zehe (:MarcoZ)]

Comment on attachment 451097 [details] [diff] [review]
test (try fail - don't check in)

r=me thanks!

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

Fix landed: http://hg.mozilla.org/mozilla-central/rev/2348611d0230

Fixing menu testing fragility will happen on bug 572103.