implement IsInDocument as accessible flag

RESOLVED FIXED in mozilla16

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: surkov, Assigned: capella)

Tracking

unspecified
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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
(Assignee)

Comment 1

5 years ago
Created attachment 631667 [details] [diff] [review]
Patch (v1)

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)
(Reporter)

Comment 2

5 years ago
I think because of
inline bool IsInDocument() const { return mFlags & eIsNotInDocument; }
(Reporter)

Comment 3

5 years ago
and because of this
DocAccessible::UncacheChildrenInSubtree(Accessible* aRoot)
{
  mFlags |= eIsNotInDocument;
(Reporter)

Updated

5 years ago
Attachment #631667 - Flags: feedback?(surkov.alexander)
(Assignee)

Comment 4

5 years ago
Created attachment 631831 [details] [diff] [review]
Patch (v2)

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
(Reporter)

Comment 5

5 years ago
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+
(Assignee)

Comment 6

5 years ago
Created attachment 631904 [details] [diff] [review]
Patch (v3)

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)
(Reporter)

Comment 7

5 years ago
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);
(Assignee)

Comment 8

5 years ago
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+
(Assignee)

Comment 10

5 years ago
Created attachment 631927 [details] [diff] [review]
Patch (v4)

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).
(Assignee)

Comment 11

5 years ago
Created attachment 631942 [details] [diff] [review]
Patch (v5)

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+
(Assignee)

Comment 12

5 years ago
push to inbound TRY
https://tbpl.mozilla.org/?tree=Try&rev=314aed2afe92
(Assignee)

Comment 13

5 years ago
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
(Assignee)

Comment 14

5 years ago
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ab582792079c
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/ab582792079c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.