617300 - nsDocAccessible::AttributeChanged may update the accessible tree before layout

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Follow up from bug 498015.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: part1: update dependent ids for existing accessibles only

Comment 2

[alexander :surkov (:asurkov)]

Attachment: part2: don't fire events on attribute change if element is not accessible

Comment 3

[alexander :surkov (:asurkov)]

Attachment: part3: pend accessible recreation on attr change if necessary

Comment 4

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

Comment on attachment 496122 [details] [diff] [review]
part2: don't fire events on attribute change if element is not accessible

>+++ b/accessible/src/base/nsDocAccessible.cpp

>@@ -963,22 +963,32 @@ nsDocAccessible::AttributeWillChange(nsI
>+  // Proceed even the element is not accessible because element may become
>+  // accessible if it gets certain attribute.

Nit: Proceed even "if" the ...


>+bool
>+nsDocAccessible::UpdateAccessibleOnAttrChange(dom::Element* aElement,

>+  if (aAttribute == nsAccessibilityAtoms::href ||
>+      aAttribute == nsAccessibilityAtoms::onclick) {
>+    // Not worth the expense to ensure which namespace these are in
>+    // It doesn't kill use to recreate the accessible even if the attribute was used
>+    // in the wrong namespace or an element that doesn't support it
>+    RecreateAccessible(aElement);

How would a change to href or onclick change the accessible class we use?

>+++ b/accessible/src/base/nsDocAccessible.h

>+  /**
>+   * Update an accessible or recreates depending on changed attribute.

How about: Update or recreate and accessible depending on a changed attribute.

Comment 5

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

Comment on attachment 496122 [details] [diff] [review]
part2: don't fire events on attribute change if element is not accessible

>diff --git a/accessible/src/base/nsDocAccessible.cpp b/accessible/src/base/nsDocAccessible.cpp
>--- a/accessible/src/base/nsDocAccessible.cpp
>+++ b/accessible/src/base/nsDocAccessible.cpp
>@@ -963,22 +963,32 @@ nsDocAccessible::AttributeWillChange(nsI
> }
> 
> void
> nsDocAccessible::AttributeChanged(nsIDocument *aDocument,
>                                   dom::Element* aElement,
>                                   PRInt32 aNameSpaceID, nsIAtom* aAttribute,
>                                   PRInt32 aModType)
> {
>+  // Proceed even the element is not accessible because element may become
>+  // accessible if it gets certain attribute.

nit: Proceed even *if* the...

>+  if (UpdateAccessibleOnAttrChange(aElement, aAttribute))
>+    return;
>+

>+  // Ignore attribute change if the element doesn't have an accessible (at all
>+  // or still) iif the element is not a root content of this document accessible
>+  // (what is treated as attribute change on this document accessible).

nit: iif/iff
nit: what/which

>+  nsAccessible* accessible = GetCachedAccessible(aElement);
>+  if (!accessible && (mContent != aElement))
>+    return;

Why the && clause? When would we ever have both !accessible and mContent == aElement?

>+
>+  // Fire accessible events iif there's an accessible, otherwise we consider

nit: iff


>+++ b/accessible/src/base/nsDocAccessible.h

>-    static void ScrollTimerCallback(nsITimer *aTimer, void *aClosure);
>+  /**
>+   * Update an accessible or recreates depending on changed attribute.

how about: (Maybe) update or recreate an accessible depending on the changed attribute.

Comment 6

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

Comment on attachment 496122 [details] [diff] [review]
part2: don't fire events on attribute change if element is not accessible

r=me (with prior nits)

Comment 7

[alexander :surkov (:asurkov)]

part1 is landed on 2.0 beta9 - http://hg.mozilla.org/mozilla-central/rev/8b6d5951bc1d

Comment 8

[alexander :surkov (:asurkov)]

part1 is landed on 2.0 beta9 - http://hg.mozilla.org/mozilla-central/rev/9bb6c33a5ad1

Comment 9

[alexander :surkov (:asurkov)]

Attachment: part4: remove unnecessary checks

that's not a fix, rather a cleaning up but it makes sense to do. We can't be called when we're shutdown because document unregister itself from mutation notifications and if an accessible was created then we should fire events not depending on document was loaded or not since the accessible state was changed. We don't ignore events until the document is loaded anymore.

Comment 10

[alexander :surkov (:asurkov)]

the rest code looks good: we either getting an accessible for the target content or traverse accessible parent chain (GetMultiSelectableContainer or nsRootAccessible->FireAccessibleFocusEvent): all of these are created at this point.

Comment 11

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

Comment on attachment 504105 [details] [diff] [review]
part4: remove unnecessary checks

r=me. Please land ASAP for nightly testing.

>-
>-  if (!IsContentLoaded())
>-    return; // Still loading, ignore setting of initial attributes
>-
>-  nsCOMPtr<nsIPresShell> shell = GetPresShell();
>-  if (!shell) {
>-    return; // Document has been shut down
>-  }
>-
>-  NS_ASSERTION(aContent, "No node for attr modified");
>-

Can you add a comment like:
// Note: we don't bail if all the content hasn't finished loading because these attributes are changing for a loaded part of the content.

Comment 12

[alexander :surkov (:asurkov)]

part4 with David's addressed comment landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/6d61c7986a1b

Comment 13

[alexander :surkov (:asurkov)]

"part3: pend accessible recreation on attr change if necessary" landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/e807269acaa3