Closed Bug 617300 Opened 9 years ago Closed 9 years ago

nsDocAccessible::AttributeChanged may update the accessible tree before layout

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b10
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: surkov, Assigned: surkov)

References

Details

(Keywords: access, regression, Whiteboard: [hardblocker])

Attachments

(4 files)

Follow up from bug 498015.
blocking2.0: --- → ?
blocking2.0: ? → final+
Attachment #496083 - Flags: review?(bolterbugz) → review+
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.
Keywords: regression
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.
Attachment #496127 - Flags: review?(bolterbugz) → review+
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)
Attachment #496122 - Flags: review?(bolterbugz) → review+
blocking2.0: final+ → betaN+
Whiteboard: [hardblocker]
Blocks: 625653
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.
Attachment #504105 - Flags: review?(bolterbugz)
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 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.
Attachment #504105 - Flags: review?(bolterbugz) → review+
part4 with David's addressed comment landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/6d61c7986a1b
"part3: pend accessible recreation on attr change if necessary" landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/e807269acaa3
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b10
Depends on: 627099
You need to log in before you can comment on or make changes to this bug.