Closed Bug 760354 Opened 8 years ago Closed 8 years ago

implement IsInDocument as accessible flag

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: surkov, Assigned: capella)

References

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(1 file, 4 obsolete files)

spun off bug 759875. DocAccessible::IsInDocument returns wrong result for an accessible that was destroyed and new accessible for the same DOM node was created.

1) add eIsNotInDocument flag to Accessible::StateFlags (don't forget to update AccessibleTypes enum)
2) add IsInDocument() method to Accessible
3) remove DocAccessible::IsInDocument
4) make DocAccessible::UncacheChildrenInSubtree to set IsNotInDocument flag on the accessible
Attached patch Patch (v1) (obsolete) — Splinter Review
I followed what I thought the intent was here, but I'm getting test failures in test_stale.html. Did I mess up the code? Or simply expose the problem we're trying to fix?

Ties into original Bug 676267 ... ?
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #631667 - Flags: feedback?(surkov.alexander)
I think because of
inline bool IsInDocument() const { return mFlags & eIsNotInDocument; }
and because of this
DocAccessible::UncacheChildrenInSubtree(Accessible* aRoot)
{
  mFlags |= eIsNotInDocument;
Attachment #631667 - Flags: feedback?(surkov.alexander)
Attached patch Patch (v2) (obsolete) — Splinter Review
Lord, I've got to stop jumping on bugs I don't fully understand :)

This seems to make more sense, and it passes all tests ...
Attachment #631667 - Attachment is obsolete: true
Comment on attachment 631831 [details] [diff] [review]
Patch (v2)

Review of attachment 631831 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/generic/Accessible.h
@@ +479,5 @@
>      return mContent->IsHTML() &&
>        (mContent->Tag() == nsGkAtoms::abbr || mContent->Tag() == nsGkAtoms::acronym);
>    }
>  
> +  inline bool IsInDocument(Accessible* aAccessible) const

no inline, I'd keep it near of IsDefunct, please comment it

@@ +484,5 @@
> +  {
> +    Accessible* acc = aAccessible;
> +    while (acc && !acc->IsPrimaryForNode())
> +      acc = acc->Parent();
> +

you don't need that because you set the flag on each accessible you uncache from document

@@ +488,5 @@
> +
> +    return acc ? !(acc->mFlags & eIsNotInDocument) : false;
> +  }
> +
> +  inline void SetIsNotInDocument() { mFlags |= eIsNotInDocument; }

not sure I like it's public (you could friend classes) but perhaps it's ok.
Attachment #631831 - Flags: review?(trev.saunders)
Attachment #631831 - Flags: feedback+
Attached patch Patch (v3) (obsolete) — Splinter Review
I went ahead and tweaked those bits while waiting ...
Attachment #631831 - Attachment is obsolete: true
Attachment #631831 - Flags: review?(trev.saunders)
Attachment #631904 - Flags: review?(trev.saunders)
Comment on attachment 631904 [details] [diff] [review]
Patch (v3)

Review of attachment 631904 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/generic/Accessible.h
@@ +694,5 @@
> +   */
> +  bool IsInDocument(Accessible* aAccessible) const
> +  {
> +    return aAccessible ? !(aAccessible->mFlags & eIsNotInDocument) : false;
> +  }

you're on accessible so you don't need aAccessible argument, jsut
return !(mFlags & eIsNotInDocument);
I though I tried that and it was failinng ... lemme try again ...
Comment on attachment 631904 [details] [diff] [review]
Patch (v3)

># HG changeset patch
># Parent 0f6ae110182a267ab1a20bca6730194cbff72ea3
># User Mark Capella <markcapella@twcny.rr.com>
>Bug 760354 - implement IsInDocument as accessible flag, r=surkov, f=tbsaunde

might want to update that ;-)

>+  bool IsInDocument(Accessible* aAccessible) const
>+  {
>+    return aAccessible ? !(aAccessible->mFlags & eIsNotInDocument) : false;

if we're going to move this from DocAccessible to Accessible we should get rid of the argument and make it work on this, and then update the callers.  Hopefully that'll help perf some since callers won't need to get the doc anymore.
Attachment #631904 - Flags: review?(trev.saunders) → review+
Attached patch Patch (v4) (obsolete) — Splinter Review
This is how I had the patch without passing an argument ... it fails test_stale.html with:

failed | wrong extra state bits for ['div node', address: [object HTMLDivElement], role: section, address: [xpconnect wrapped nsIAccessible]]!got '0', expected 'stale'

Changing it to take the arg works / passes ... (I had the debugger out and was hacking at it for awhile before I tried the new approach).
Attached patch Patch (v5)Splinter Review
Changed the caller and this works again ...
Attachment #631904 - Attachment is obsolete: true
Attachment #631927 - Attachment is obsolete: true
Attachment #631942 - Flags: review?(trev.saunders)
Attachment #631942 - Flags: review?(trev.saunders) → review+
previous push was in the middle of many TRY and INBOUND issues ... lotsa backouts and such ... waited, and am trying again:

https://tbpl.mozilla.org/?tree=Try&rev=982d37b69ab6
https://hg.mozilla.org/mozilla-central/rev/ab582792079c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.