371394 - Fire text changed event when content of hypertext accessible is changed

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a3pre) Gecko/20070222 Minefield/3.0a3pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a3pre) Gecko/20070222 Minefield/3.0a3pre

Adding or removing text from entries in Firefox results in one object:children-changed:add event per character changed.  These events should not be generated.

We are getting the appropriate object:text-changed events both for insertion and deletion.

Reproducible: Always

Steps to Reproduce:
1. Launch at-poke and turn event logging on.
2. Move to any entry within Firefox (e.g. location, search, find, name in Add Bookmarks dialog, a text box on a web page), type some text and use Backspace to remove it.
Actual Results:  
We get an event similar to this one:

children changed event (object:children-changed:add) -1 0 parent:
A|co|ac|te|ed:entry:: child: <Null>

in addition to the appropriate object:text-changed event.

Expected Results:  
We would only get the object:text-changed event.

Comment 1

[Nian Liu(n/a in a long time)]

i can't see any logic error with current code.
aaron, should we add extra if condition for entry specific?

object:children_changed is called by 
nsDocAccessible::CharacterDataChanged which calls nsDocAccessible::InvalidateCacheSubtree

see http://mxr-test.landfill.bugzilla.org/seamonkey/source/accessible/src/base/nsDocAccessible.cpp#1297

Comment 2

[Aaron Leventhal]

Surkov, this is probably a dupe of some stuff you're working on.

Comment 3

[Aaron Leventhal]

In fact it's probably a dupe of bug 378468. I think the actual work is happening in bug 378175.

Comment 4

[alexander :surkov (:asurkov)]

Attachment: patch

bug 377891 isn't dupe of this one, right?

Comment 5

[Aaron Leventhal]

Yes, your patch makes it clear that bug 377891 is a dupe.

Comment 7

[Aaron Leventhal]

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

Cool fix! Since we were already firing text change events, does this cause duplicate text change events to be fired?

r+ if the answer is no.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> (From update of attachment 265475 [details] [diff] [review])
> Cool fix! Since we were already firing text change events, does this cause
> duplicate text change events to be fired?
> 
> r+ if the answer is no.
> 

It shouldn't because we fire text changed events for editor that owned by hyper text accessible and we have not accessible for editor to fire text changed events from nsIMutationObserver methods. But I'll check this.

Comment 9

[[e-mail]]

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

I don't see the relevance of this patch, but fortunately that's Aaron's job ;-)

>+  // Text has been removed.
>+  if (end - start > 0) {
(end > start) surely?

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> (From update of attachment 265475 [details] [diff] [review])
> Cool fix! Since we were already firing text change events, does this cause
> duplicate text change events to be fired?

Unfortunately you're right. First, nsDocAccessible::CharacterDataChanged() is called and then nsHypertextAccessible::DidInsertText() is called when I type charackters into input field. Aaron, is there testcases for problems that patch should fix?

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> (From update of attachment 265475 [details] [diff] [review])
> I don't see the relevance of this patch, but fortunately that's Aaron's job ;-)
> 
> >+  // Text has been removed.
> >+  if (end - start > 0) {
> (end > start) surely?
> 

Not sure. CharacterDataChangeInfo is not documented or I didn't find any documentation ;) I looked at http://lxr.mozilla.org/mozilla/source/content/base/src/nsGenericDOMDataNode.cpp#394 and if I'm right then end always is greater than start.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #11)
> (In reply to comment #9)
> > (From update of attachment 265475 [details] [diff] [review] [details])
> > I don't see the relevance of this patch, but fortunately that's Aaron's job ;-)
> > 
> > >+  // Text has been removed.
> > >+  if (end - start > 0) {
> > (end > start) surely?
> > 
> 
> Not sure. CharacterDataChangeInfo is not documented or I didn't find any
> documentation ;) I looked at
> http://lxr.mozilla.org/mozilla/source/content/base/src/nsGenericDOMDataNode.cpp#394
> and if I'm right then end always is greater than start.
> 

Please ignore my comment.

Comment 13

[Aaron Leventhal]

> Unfortunately you're right. 

Then maybe ContentInserted() and ContentRemoved() are redundant with nsHyperTextAccessible::[Will|Did][Insert|Delete]Node().

I wonder if we really need to use nsIEditActionListener at all or whether we should be doing it all via nsIDocumentObserver.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> > Unfortunately you're right. 
> 
> Then maybe ContentInserted() and ContentRemoved() are redundant with
> nsHyperTextAccessible::[Will|Did][Insert|Delete]Node().
> 
> I wonder if we really need to use nsIEditActionListener at all or whether we
> should be doing it all via nsIDocumentObserver.
> 

Yes, good question. Though nsIEditActionListener gives more information at the first sight than nsIMutationObserver. I'll look more precisely.

Comment 15

[Aaron Leventhal]

Attachment: Testcase for character data changes

Comment 16

[Aaron Leventhal]

Attachment: Testcase for node changes

Comment 17

[Aaron Leventhal]

Attachment: Test case with HTML input and textarea

Comment 18

[Aaron Leventhal]

I checked and they are redundant. I think we may really be better off getting rid of nsIEditActionListener if we can get the info we need otherwise. We don't know if changes will come from editing or from Javascript, but they are still important.

Here is a test case of a rich text editing area:
http://www.mozilla.org/editor/midasdemo/

Comment 19

[Ginn Chen]

I found CharacterDataChanged is not called for the first character when I input a string in HTML/XUL textfield.
Is it a bug of somewhere?

Also static text change seems won't get CharacterDataChanged called.
e.g. Testcase for node changes
Any plan for it?

Comment 20

[Aaron Leventhal]

Ginn, it's not a bug. As the first character is inserted or removed, a text node is inserted or removed.

Static text gets CharacterDataChanged unless all the text of a node is changing, in which case will you see a node removal and then a node insertion.

See the "Testcase for character data changes"

Comment 21

[Ginn Chen]

Thanks for your explanation.

Comment 22

[Aaron Leventhal]

Please see bug 376013 comment 8.

Comment 23

[alexander :surkov (:asurkov)]

Attachment: patch2

Ginn, may I ask you to check this patch with Orca (unfortunately, my Orca doesn't want to work at all)? Does elements like xul:textbox report text changes successfully?

Comment 24

[Ginn Chen]

Comment on attachment 270579 [details] [diff] [review]
patch2

It doesn't work.
Because we don't create atkobjects for plain text leaf?

See the assert in src/atk nsAccessibleWrap::FireAccessibleEvent

Comment 25

[Aaron Leventhal]

Also, doesn't nsHyperTextAccessible need to stop being an nsIEditActionListener? I don't think we need to do that.

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #25)
> Also, doesn't nsHyperTextAccessible need to stop being an
> nsIEditActionListener? I don't think we need to do that.
> 

I think no. Possibly it will be comfortable to have both of them because it is a trick to obtain right accessible from nsDocAccessible::CharacterDataChanged. I think we should use nsDocAccessible::CharacterDataChanged only if there is no editor. In this case these nsIEditActionListener and nsIMutationObserver won't be intersected.

Comment 27

[alexander :surkov (:asurkov)]

(In reply to comment #24)
> (From update of attachment 270579 [details] [diff] [review])
> It doesn't work.
> Because we don't create atkobjects for plain text leaf?

We don't create accessible but the method doesn't get them as arguments. The reason is we have not accessibles at all inside textfields. Right? Also, this method can't obtain document as argument. Therefore I guess the first patch should work correct. Ginn, may I ask you to check with Orca?

Comment 28

[Aaron Leventhal]

> I think no. Possibly it will be comfortable to have both of them because it is
> a trick to obtain right accessible from nsDocAccessible::CharacterDataChanged.

Don't we already need similar code for other things? If you have the content node you can GetAccessibleInParentChain()

It seems like having both kinds of listeners makes our code more complex and has the risk of double events and invalidations.

Comment 29

[Ginn Chen]

(In reply to comment #27)
> Therefore I guess the first patch
> should work correct. Ginn, may I ask you to check with Orca?
no, first patch doesn't work, either.

btw: what's the patch trying to solve? bug 377891?
the problem in this bug description is already gone, right?

Comment 30

[Aaron Leventhal]

The caret code needs to be able to convert a node, offset to a hypertext accessible with an offset. See nsCaretAccessible::NotifySelectionChanged()

Comment 31

[alexander :surkov (:asurkov)]

(In reply to comment #29)
> (In reply to comment #27)
> > Therefore I guess the first patch
> > should work correct. Ginn, may I ask you to check with Orca?
> no, first patch doesn't work, either.

What does "doesn't work" mean? Is text events are dublicated for textfields. Does text events are not fired when selected item is changed in combobox, for example?

> 
> btw: what's the patch trying to solve? bug 377891?

Yes, in general if I understand right then we want to fire text changed events if text has been changed everywhere in document.

> the problem in this bug description is already gone, right?
> 

Yes.

Comment 32

[alexander :surkov (:asurkov)]

(In reply to comment #30)
> The caret code needs to be able to convert a node, offset to a hypertext
> accessible with an offset. See nsCaretAccessible::NotifySelectionChanged()
> 

It's not the same. NotifySelectionChanged() gets selection where focused node is textfield itseld. But CharakterDataModified() gets text inside an editor of textfield.

Do you prefer to replace nsIEditActionListener on nsIMutationObserver?

Comment 33

[Aaron Leventhal]

Actually, NotifySelectionChanged() also gets changes anywhere in a document, when caret browsing is on.

so I think it is the same.

I prefer to do it all in one place with nsIMutationObserver if possible, because it catches more changes.

Comment 34

[Ginn Chen]

(In reply to comment #31)
> What does "doesn't work" mean? Is text events are dublicated for textfields.
> Does text events are not fired when selected item is changed in combobox, for
> example?

text-changed events are not fired when I input.

Comment 35

[alexander :surkov (:asurkov)]

(In reply to comment #34)
> (In reply to comment #31)
> > What does "doesn't work" mean? Is text events are dublicated for textfields.
> > Does text events are not fired when selected item is changed in combobox, for
> > example?
> 
> text-changed events are not fired when I input.
> 

input where? into xul:textbox or?

Comment 36

[Ginn Chen]

location bar, search field, xul:textbox, html:textbox

Comment 37

[alexander :surkov (:asurkov)]

(In reply to comment #36)
> location bar, search field, xul:textbox, html:textbox
> 

it's strange because the first patch doesn't change anything for textfields. Does we fire events at all? :)

Comment 38

[Ginn Chen]

my mistake, patch v1 works for text-changed events
but I got
###!!! ASSERTION: Event other than SHOW and HIDE fired for plain text leaves: 'type == nsIAccessibleEvent::EVENT_SHOW || type == nsIAccessibleEvent::EVENT_HIDE', file nsAccessibleWrap.cpp, line 1115
in terminal

Comment 39

[alexander :surkov (:asurkov)]

Attachment: patch3

Ginn, it should work for simple textbox but it may be problems with editor and with multiline textbox due to wrong offsets. Possibly we should use here DOMPointToOffset to get correct offsets in the case when hypertext accessible contains several textnodes.

Comment 40

[Aaron Leventhal]

Would a follow-up patch come later to remove the need for nsIEditActionListener completely?

Comment 41

[alexander :surkov (:asurkov)]

(In reply to comment #40)
> Would a follow-up patch come later to remove the need for nsIEditActionListener
> completely?
> 

Yes, I just try to find a way with Ginn's help.

Comment 42

[Ginn Chen]

Comment on attachment 272028 [details] [diff] [review]
patch3

Works fine. Thanks!

Comment 43

[alexander :surkov (:asurkov)]

(In reply to comment #42)
> (From update of attachment 272028 [details] [diff] [review])
> Works fine. Thanks!
> 

I have two questions.

1) I just don't get why DOMPointToOffset is used in nsIEditActionListener of hypertext accessible. I assume if editor will have two spans and I remove text from second span then now I will report offset relative second span, but I should have offset relative first span. If I'm right and since Ginn said patch works correct then eventually AT doesn't make difference for this. Probably not all.

2) WillDeleteNode fires text changed events if element node will be removed and it looks we set offset relative element nodes. Why do we need this?

Comment 44

[Aaron Leventhal]

Hi Surkov,

For #2, if a node that was deleted was represented by an embedded object character, and that character is removed, then we are saying that embedded object character is deleted. I'm not sure if we really need to do that though. It might be good to ask on #orca

For #1, I'm not sure I understand the question yet.

Comment 45

[Aaron Leventhal]

For #1, the offset should be relative to the hypertext itself. Catch me on IRC if you have more questions.

Comment 46

[Joanmarie Diggs]

> For #2, if a node that was deleted was represented by an embedded object
> character, and that character is removed, then we are saying that embedded
> object character is deleted. I'm not sure if we really need to do that though.
> It might be good to ask on #orca

Aaron, does this apply to your test case from Comment #16, or something else?  If it is that test case, would we be getting any events to let us know that the link had appeared/disappeared?

Comment 47

[Aaron Leventhal]

Joanie, yes it applies to that one.

If you have something like:
abc[link]def

Then the hierarchy should be something like
abc?def   (Accessible Text, ? = 0xfffc)
  \
   Child link

When the child is removed the text becomes
abcdef
and the child is removed.

Therefore I think you should get 2 events:
1) a text change event for the removed embedded object char
2) a children_changed:remove event for the removed link

For the link insertion case it should be the opposite.

One wrinkle is that the text object would no longer need to be a hyper text when there are no embedded objects. Changing interfaces mid-way is bad. The reverse problem, of becoming a hypertext when it wasn't one before, is also bad. So, we might just need to say that even if an accessible text has 0 embedded objects it it still needs to support the hypertext interface.

Ginn was looking at that issue recently?

Comment 48

[Ginn Chen]

Yes.
I asked Willie about making every object that inherits nsHyperTextAccessible always implements hypertext and text interface.

I think it's a way around.
I don't have Willie's answer yet. He's on GUADEC.

text editable interface is another problem we're facing. We solved it for nsDocAccessible, but since we have content editable feature now, maybe it's not enough.

Comment 49

[alexander :surkov (:asurkov)]

Attachment: patch4

Comment 50

[Ginn Chen]

1) for ns*Accessible::Shutdown
no need to verify mEditor is not null before setting it to null

2) I didn't see InvaliteChildren in FireTextChangedEventOnDOMNodeInserted.
I'm not sure whether it works, if we're adding a link or something.

3) Why add nsHyperTextAccessible::GetOffsetAtDOMPoint? Can't we just use DOMPointToOffset?

Comment 51

[alexander :surkov (:asurkov)]

(In reply to comment #50)

> 2) I didn't see InvaliteChildren in FireTextChangedEventOnDOMNodeInserted.
> I'm not sure whether it works, if we're adding a link or something.

I'm not sure because we file show event. Is InvalidateChildren necessary? Ginn, can you test it again?
> 
> 3) Why add nsHyperTextAccessible::GetOffsetAtDOMPoint? Can't we just use
> DOMPointToOffset?
> 

because last argument of DOMPointToOffset can be missed. It is impossible to do in idl?

Comment 52

[Willie Walker]

Hi All:

Implementing the interface is the appropriate thing to do if you ever
expect the object to have to support the interface.  This is definitely
a much better solution than deleting the object and creating a new one.

Returning nulls will probably be a troublesome thing.  It would be
better if you were able to return an actual value.  For example, for 
things that return strings, returning an empty string would be better 
than null.

Let me know if you need more information, and we can run through the
specific APIs to determine what desirable return values would be.

Thanks!

Comment 53

[Aaron Leventhal]

Will, how will you know if a given accessible node is really editable, or only potentially editable (which everything is).

Comment 54

[Willie Walker]

(In reply to comment #53)
> Will, how will you know if a given accessible node is really editable, or only
> potentially editable (which everything is).
> 

I think it may depend upon the type of node we're dealing with, but in general it would have the EDITABLE state set.

Comment 55

[Ginn Chen]

(In reply to comment #51)
> (In reply to comment #50)
> 
> > 2) I didn't see InvaliteChildren in FireTextChangedEventOnDOMNodeInserted.
> > I'm not sure whether it works, if we're adding a link or something.
> 
> I'm not sure because we file show event. Is InvalidateChildren necessary? Ginn,
> can you test it again?

It works. :)

> > 
> > 3) Why add nsHyperTextAccessible::GetOffsetAtDOMPoint? Can't we just use
> > DOMPointToOffset?
> > 
> 
> because last argument of DOMPointToOffset can be missed. It is impossible to do
> in idl?

I've an idea, I've a patch for QI to nsRefPtr<nsHyperTextAccessible>,
I think we can get it in, and then we can use DOMPointToOffset directly.
I think it makes more sense than adding  GetOffsetAtDOMPoint to nsIAccessibleText.idl.

Do you agree?

Comment 56

[alexander :surkov (:asurkov)]

(In reply to comment #55)

> I've an idea, I've a patch for QI to nsRefPtr<nsHyperTextAccessible>,
> I think we can get it in, and then we can use DOMPointToOffset directly.
> I think it makes more sense than adding  GetOffsetAtDOMPoint to
> nsIAccessibleText.idl.
> 
> Do you agree?
> 

It looks unusual for me but I think it's fine :). I'll put new patch for that.

Comment 57

[alexander :surkov (:asurkov)]

(In reply to comment #55)
> (In reply to comment #51)
> > (In reply to comment #50)
> > 
> > > 2) I didn't see InvaliteChildren in FireTextChangedEventOnDOMNodeInserted.
> > > I'm not sure whether it works, if we're adding a link or something.
> > 
> > I'm not sure because we file show event. Is InvalidateChildren necessary? Ginn,
> > can you test it again?
> 
> It works. :)

It's excellent! Thank you :)

Comment 58

[Ginn Chen]

(In reply to comment #56)
> It looks unusual for me but I think it's fine :). I'll put new patch for that.
> 

Actually I need that patch to fix the problem mentioned in #c48, bug 386235

Comment 59

[alexander :surkov (:asurkov)]

Attachment: patch5

Comment 60

[Ginn Chen]

Comment on attachment 273765 [details] [diff] [review]
patch5

very good

Comment 61

[alexander :surkov (:asurkov)]

checked in

Comment 62

[Aaron Leventhal]

Apparently this caused bug 389873.

Do we fire these events before the doc load is complete? We shouldn't.

Comment 63

[alexander :surkov (:asurkov)]

(In reply to comment #62)
> Apparently this caused bug 389873.
> 
> Do we fire these events before the doc load is complete? We shouldn't.
> 

Apparently we do.