625009 - text offsets don't get updated when text of first child text accessible is changed

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Results in hypertext interface return wrong results.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

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

Comment on attachment 503122 [details] [diff] [review]
patch

r=me.

>diff --git a/accessible/src/html/nsHyperTextAccessible.cpp b/accessible/src/html/nsHyperTextAccessible.cpp

> nsHyperTextAccessible::GetChildOffset(PRUint32 aChildIndex,
>                                       PRBool aInvalidateAfter)

I see we want to invalidate after when calling from: nsDocAccessible::FireTextChangeEventForText
It is undesirable to have a getter be able to have a side effect like this even if it is somewhat documented by having the second argument. Maybe we should file a follow up on this, to either rename the getter or separate out the updating into another function?

> 
>   PRInt32 count = mOffsets.Length() - aChildIndex;
>   if (count > 0) {
>     if (aInvalidateAfter)
>       mOffsets.RemoveElementsAt(aChildIndex, count);
> 
>     return mOffsets[aChildIndex - 1];
>   }

>+++ b/accessible/tests/mochitest/hypertext/Makefile.in

>+# The Initial Developer of the Original Code is
>+# Mozilla Foundation.
>+# Portions created by the Initial Developer are Copyright (C) 2010

2011 :)

Observation we seem have, and to be adding event tests outside of our events directory. I guess that's okay.

Comment 3

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

Approving soft blocker.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #2)

> > nsHyperTextAccessible::GetChildOffset(PRUint32 aChildIndex,
> >                                       PRBool aInvalidateAfter)
> 
> I see we want to invalidate after when calling from:
> nsDocAccessible::FireTextChangeEventForText
> It is undesirable to have a getter be able to have a side effect like this even
> if it is somewhat documented by having the second argument. Maybe we should
> file a follow up on this, to either rename the getter or separate out the
> updating into another function?

yeah, there's something weird here. I'll file a bug.
]
> >+# Portions created by the Initial Developer are Copyright (C) 2010
> 
> 2011 :)

you know different time zones, different years :)

> Observation we seem have, and to be adding event tests outside of our events
> directory. I guess that's okay.

event queues are just a tool here, we wait for event to do a test, so that's ok.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)

> > if it is somewhat documented by having the second argument. Maybe we should
> > file a follow up on this, to either rename the getter or separate out the
> > updating into another function?
> 
> yeah, there's something weird here. I'll file a bug.

done, bug 625255

Comment 6

[alexander :surkov (:asurkov)]

landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/de195d1171d6