Closed Bug 371394 Opened 15 years ago Closed 15 years ago

Fire text changed event when content of hypertext accessible is changed

Categories

(Core :: Disability Access APIs, defect)

All
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jdiggs, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(4 files, 4 obsolete files)

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.
Keywords: access
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
Assignee: nobody → aaronleventhal
Component: Disability Access → Disability Access APIs
Product: Firefox → Core
QA Contact: disability.access → accessibility-apis
Surkov, this is probably a dupe of some stuff you're working on.
Assignee: aaronleventhal → surkov.alexander
Blocks: 371279
In fact it's probably a dupe of bug 378468. I think the actual work is happening in bug 378175.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch patch (obsolete) — Splinter Review
bug 377891 isn't dupe of this one, right?
Attachment #265475 - Flags: superreview?(neil)
Attachment #265475 - Flags: review?(aaronleventhal)
Yes, your patch makes it clear that bug 377891 is a dupe.
Duplicate of this bug: 377891
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.
Attachment #265475 - Flags: review?(aaronleventhal) → review+
(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 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?
Attachment #265475 - Flags: superreview?(neil) → superreview+
(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?


(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.
(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.
> 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.
(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.
Attachment #265941 - Attachment description: Testcase → Testcase for character data changes
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/
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?
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"
Thanks for your explanation.
Please see bug 376013 comment 8.
Attached patch patch2 (obsolete) — Splinter Review
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?
Attachment #270579 - Flags: review?(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
Attachment #270579 - Flags: review?(ginn.chen) → review-
Also, doesn't nsHyperTextAccessible need to stop being an nsIEditActionListener? I don't think we need to do that.
(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.
(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?
> 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.
(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?
The caret code needs to be able to convert a node, offset to a hypertext accessible with an offset. See nsCaretAccessible::NotifySelectionChanged()
(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.
(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?
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.
(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.
(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?
location bar, search field, xul:textbox, html:textbox
(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? :)
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
Attached patch patch3 (obsolete) — Splinter Review
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.
Attachment #270579 - Attachment is obsolete: true
Attachment #272028 - Flags: review?(ginn.chen)
Would a follow-up patch come later to remove the need for nsIEditActionListener completely?
(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 on attachment 272028 [details] [diff] [review]
patch3

Works fine. Thanks!
Attachment #272028 - Flags: review?(ginn.chen) → review+
(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?
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.
For #1, the offset should be relative to the hypertext itself. Catch me on IRC if you have more questions.
> 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?
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?
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.
Attached patch patch4 (obsolete) — Splinter Review
Attachment #265475 - Attachment is obsolete: true
Attachment #272028 - Attachment is obsolete: true
Attachment #272971 - Flags: review?(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?
(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?
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!
Will, how will you know if a given accessible node is really editable, or only potentially editable (which everything is).
(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.
(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?
(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.
(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 :)
(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
Attached patch patch5Splinter Review
Attachment #272971 - Attachment is obsolete: true
Attachment #273765 - Flags: review?(ginn.chen)
Attachment #272971 - Flags: review?(ginn.chen)
Comment on attachment 273765 [details] [diff] [review]
patch5

very good
Attachment #273765 - Flags: review?(ginn.chen) → review+
Summary: Erroneous/unnecessary object:children-changed at-spi events are generated in entries → Fire text changed event when content of hypertext accessible is changed
checked in
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Depends on: 389873
Apparently this caused bug 389873.

Do we fire these events before the doc load is complete? We shouldn't.
(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.
You need to log in before you can comment on or make changes to this bug.