Steps to reproduce: 1. Launch Accerciser and turn event monitoring on 2. Go to google.com 3. In the search entry type "test" and then backspace over the characters. Expected results: The any_data for the object:text-changed:delete event would contain the character that was just deleted a la: object:text-changed:delete(3, 1, t) source: [entry | Google Search] application: [application | Minefield] object:text-changed:delete(2, 1, s) source: [entry | Google Search] application: [application | Minefield] Actual results: The any_data for the event contains the current character (if there is one): object:text-changed:delete(3, 1, ) source: [entry | Google Search] application: [application | Minefield] object:text-changed:delete(2, 1, ) source: [entry | Google Search] application: [application | Minefield] This worked as expected through Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a7pre) Gecko/2007072604 Minefield/3.0a7pre. It is broken effective: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a7pre) Gecko/2007072504 Minefield/3.0a7pre

It looks it's the thing I was afraid. We fire delete event after text was deleted, previously we did it in visa versa.

Aha! This sounds like Aaron's question from a while back: > Currently we fire /text_changed::delete/before the text has been removed > from the given Accessible Text object. > > Is it a problem if the event is fired afterwards? We're thinking that it > should be okay, because you get the event data. To which I responded: -------------------- If we get the event data, and the order of events doesn't change *from our perspective*, then it should be fine. -------------------- So we're getting the event data -- it's just the wrong data because of the ordering of the event (?) perhaps w.r.t. the caret offset being used (??)

Surkov, is this not a problem in IA2 because of oldText?

This is a problem in IA2 as well. We need to go back to using mutation events -- it supplies the old text value. Surkov, this should be fixed after bug 390692. It's very important we fix this for Firefox 3.

Also, when we fire a text change event for a dom node inserted or removed, it will often just be for a single character (for the embedded object character). We should be firing that on the hyper text that changed. Looking at the code I'm not sure we get that correct.

(In reply to comment #4) > This is a problem in IA2 as well. We need to go back to using mutation events > -- it supplies the old text value. Sorry, to go back to what? We removed usage of nsIEditActionListener. Therefore I gues we got a problems. If I'm right then unique way is to get back to that interface or allow us to extend nsIMutationObserver.

We can extend nsIMutationObserver or we can listen to the raw mutation event, correct? We can't go back to nsIEditActionListener because it won't tell us about changes that aren't in an editor. Therefore if text is deleted by JS we won't get it right with that interface.

(In reply to comment #7) > We can extend nsIMutationObserver or we can listen to the raw mutation event, > correct? What is raw mutation events? Is it mutation events of DOM Events specification? If yes then how will it help? > We can't go back to nsIEditActionListener because it won't tell us about > changes that aren't in an editor. Therefore if text is deleted by JS we won't > get it right with that interface. > Yes. But it's a way to get rid an regression :) Not good though.

CC'ing bzbarsky for his advice. Bz, we need the old value of text when text is being deleted. We can't get this from nsIMutationObserver right now, and we decided not to use nsIEditActionListener because it doesn't inform us of text removals from JS where there was no editor involved (dynamic pages) I would not like to use DOM mutation events because they cause a 3-5% slowdown from what I've heard. Just by using it once it switches a flag internally that causes more work to be done. Since a11y is often turned on on Linux machines without people realizing what it is, that would be an unfortunate penalty, and it would show up in tbox. In nsGenericDOMDataNode::SetTextInternal() we call nsIMutationObserver::CharacterDataChanged(). It's also where the DOMCharacterDataModified dom mutation event is generated. But apparently for perf reasons the prevValue is only calculated if mutation events are currently active. I think that text changes don't happen that often, and it should be okay to calculate text changes and pass them through nsIMutationObserver::CharacterDataChanged(). If necessary we could limit it to text removals or when a11y is active, but I don't think this would affect perf in any noticeable way. Bz, is that reasonable to do right now?

Another option is to add nsIMutationObserver::CharacterDataWillChange() -- that's ideal from the a11y point of view but we can live without it.

iirc bz pointed me bug to pass old value to nsIMutationObserver::ChararacterChanged. But calculate previous offset of deleted text by old value usage is trick I think. Ideally we need CharacterDataWillChange() as Aaron noticed.

(In reply to comment #10) > Another option is to add nsIMutationObserver::CharacterDataWillChange() -- > that's ideal from the a11y point of view but we can live without it. > And nsIMutationObserver::ContentWillRemoved() too to support embedded objects.

> And nsIMutationObserver::ContentWillRemoved() too to support embedded objects. I think for embedded objects we're okay right now. > iirc bz pointed me bug to pass old value to > nsIMutationObserver::ChararacterChanged. But calculate previous offset of > deleted text by old value usage is trick I think. We have the old text and the offset already no? It's in CharacterDataChangeInfo.mChangeStart. Instead of getting the deleted text from the current text we'd get it from the old text instead.

(In reply to comment #13) > > And nsIMutationObserver::ContentWillRemoved() too to support embedded objects. > I think for embedded objects we're okay right now. What is the difference between text delete event when charackter changed or embeded object is removed? > > iirc bz pointed me bug to pass old value to > > nsIMutationObserver::ChararacterChanged. But calculate previous offset of > > deleted text by old value usage is trick I think. > We have the old text and the offset already no? It's in > CharacterDataChangeInfo.mChangeStart. Instead of getting the deleted text from > the current text we'd get it from the old text instead. > How do we get an access to old text?

> I would not like to use DOM mutation events because they cause a 3-5% slowdown Depends on the events. We do have separate bits for different events, last I checked, so if you're listening for CharacterDataModified, then the only codepaths that will be affected are those that might dispatch that event. It might in fact be OK, if those codepaths are not hit much. > I think that text changes don't happen that often In that case you should be good with mutation events. > Another option is to add nsIMutationObserver::CharacterDataWillChange() I'm sure this has come up before; I think sicking was involved, or maybe it was roc. We could probably do that, but I'd start with mutation events and actually measuring the impact.

Surkov, when ContentRemoved() is called we get a pointer to the DOM node, so we know what text it has. Right? In the CharacterDataChanged case we would need to access to the old text by passing it from nsGenericDOMDataNode::SetTextInternal() via CharacterDataChangeInfo. Or, if we use mutation events we just QI to nsIDOMMutationEvent and get the prevValue.

The problem with passing the old data in CharacterDataChanged is that it is probably a decent perf hit.

Right. The suggestion is to have a CharacterDataWillChange.

CharacterDataWillChange() would help -- we could make it only fire when a11y is active. Or is it needed for other things?

Let I press a key 'a' inside textbox then nsGenericDOMDataNode::SetTextInternal() is called if I'm right. When caret position is changed? I assume it should be setted after text change. Correct?

does this help?

Are you asking for review or testing? BTW, I learned that we can easily create test builds using the Mozilla Try Server here: http://wiki.mozilla.org/Build:TryServer

(In reply to comment #22) > Are you asking for review or testing? Yes, for testing. I have not idea how to test it on windows.

Comment on attachment 276761 [details] [diff] [review] patch foxevent shows correct info.

What sort of performance testing have you done on this, if any? Note that I'm still waiting on Jonas' opinion of the suggestion; I won't be reviewing until that happens.

Jonas, is it alright if we use this change if it does not turn out to create a noticeable perf regression? Bz, how can we do perf testing? I was going to suggest that after reviews we just try checking it in and watch tbox. Or is there something that you recommend Surkov can do now?

There is code which can be shrunk in nsDocAccessible::FireTextChangedEventOnDOMCharacterDataModified: This is the current code in the patch: PRInt32 length; // Text will be modified if (aWillBeModified) { PRUint32 end = aInfo->mChangeEnd; PRInt32 length = end - start; if (length > 0) { nsCOMPtr<nsIAccessibleTextChangeEvent> event = new nsAccTextChangeEvent(accessible, offset, length, PR_FALSE); textAccessible->FireAccessibleEvent(event); } } // Text has been added. if (!aWillBeModified) { PRUint32 replaceLen = aInfo->mReplaceLength; if (replaceLen) { nsCOMPtr<nsIAccessibleTextChangeEvent> event = new nsAccTextChangeEvent(accessible, offset, replaceLen, PR_TRUE); textAccessible->FireAccessibleEvent(event); } } It could be compressed to this (although notice that I changed aWillBeModified to aInsert, which is the opposite so callers must change what they pass): PRInt32 length = aInsert ? aInfo->mChangeEnd - start : aInfo->mReplaceLength; if (length > 0) { nsCOMPtr<nsIAccessibleTextChangeEvent> event = new nsAccTextChangeEvent(accessible, offset, length, aInsert); textAccessible->FireAccessibleEvent(event); }

I doubt the tinderbox tests catch the conditions in which this could really suck (lots of mutation observers in flight, etc). Try writing a DOM test which has a bunch of live nodelists on the root at once then mutates textnodes and times it all.

Surkov, you can write that test in JS.

Comment on attachment 276761 [details] [diff] [review] patch Surkov, can you change it to use rendered text length instead of content length?

If a test like the one boris suggests turns out ok then I think this is ok. I'm not real exited about just firing when accessibility is installed though so if we can avoid that that'd be great.

I do not remember probability theory actually so I did easiest thing. I compared an time average values of tests before and after modifications. In general I see 3% slowdown of average value. Though I see max value is lesser than original but min value is bigger than original. I'm not sure whether this test show anything. I need some advice how to make valid test I guess.

A good start would be the "has a bunch of live nodelists on the root at once" part?

(In reply to comment #33) > A good start would be the "has a bunch of live nodelists on the root at once" > part? > Boris, sorry what is "live nodelists on the root at once"?

A number of return values from getElementsByTagName(), say. So create an array of, say, 100 such return values, then time how long the text mutations take with and without this patch to get a feel for the notification overhead....

Boris, like this? I create 400 xul:description nodes and text nodes inside of them. Then I watch how much a time takes to change data of text nodes. Results are: With my patch: 1 - max: 72, min: 53, avg: 56.5 2 - max: 159, min: 53, avg: 60.5 3 - max: 159, min: 53, avg: 60.46666666666667 4 - max: 159, min: 53, avg: 59.25 5 - max: 159, min: 52, avg: 58.44 6 - max: 159, min: 51, avg: 58.46666666666667 7 - max: 159, min: 51, avg: 58.02857142857143 8 - max: 159, min: 51, avg: 57.9 9 - max: 159, min: 51, avg: 57.6 10 - max: 159, min: 39, avg: 57.78 without my patch: 1 - max: 78, min: 52, avg: 56.4 2 - max: 156, min: 52, avg: 59.7 3 - max: 156, min: 51, avg: 57.8 4 - max: 156, min: 51, avg: 58.1 5 - max: 156, min: 50, avg: 57.22 6 - max: 156, min: 50, avg: 56.88333333333333 7 - max: 156, min: 50, avg: 56.94285714285714 8 - max: 156, min: 50, avg: 56.4625 9 - max: 156, min: 50, avg: 56.08888888888889 10 - max: 156, min: 50, avg: 56.42 regression for average value is about 2%.

fixed Aaron's comment

I think Boris is expecting you to use getElementsByTagName(). The return value is called a "live node list", which *I think* means that as the DOM tree mutates, the node list is actually updated. It's a bit counter-intuitive for C++ programmers, who expect the return value of a function not to change once it's been received. For an explanation of live NodeList objects see http://www.w3.org/TR/DOM-Level-2-Core/core.html#td-live Also see http://developer.mozilla.org/en/docs/DOM:element.getElementsByTagName

> which *I think* means that as the DOM tree mutates, the node list is actually > updated. Exactly. Which means is needs to listen for mutation notifications. Which means that this change would slow them down somewhat.

An aside: perhaps it should be possible for a mutation observer to indicate which notifications it doesn't need. For example, I don't think a nodelist from getElementsByTagName() should need to listen to any char data notifications.

One other thing. Make sure you pass different tag names to each getElementByTagName call; otherwise you'll just get 100 refs to the same object.

I believe you want to change offset to offset + renderedStartOffset, because the event is created for the entire hypertext. Therefore, the start offset for the event must be relative to the hypertext, not just this child of the hypertext. new nsAccTextChangeEvent(accessible, offset, renderedEndOffset - renderedStartOffset, !aWillBeModified); Nit: a slight simplification is to change this method so it accepts aWasModifed or aHandleInsertion or something like that, then you won't need the ! to negate the last argument. The boolean you pass in will already be ready to pass to the nsAccTextChangeEvent() constructor.

Wait, I might be wrong about the offset + renderedStartOffset. It seems that's already taken into account by passing |start| into DOMPointToOffset. So this is probably correct.

Comment on attachment 277234 [details] [diff] [review] patch2 r+, but as I mentioned, I like the idea of changing the paramemter from aWillBeModified to something which expresses the opposite (aWasModified or aFireInsertEvent).

I create 200 elements with text nodes, get them by getElementsById() and change their textnodes 10 times, then calculated max, min and avg values. Mostly average results are the same or are changed slightly (1%-2% of slowdown). Rarely regression achive 10% percents. before: 1 - max: 47, min: 37, avg: 39.77777777777778 2 - max: 83, min: 37, avg: 47 3 - max: 53, min: 36, avg: 39.77777777777778 4 - max: 48, min: 38, avg: 44.111111111111114 5 - max: 49, min: 37, avg: 41.111111111111114 6 - max: 49, min: 39, avg: 42.888888888888886 7 - max: 51, min: 37, avg: 39.666666666666664 8 - max: 48, min: 36, avg: 41.44444444444444 9 - max: 85, min: 37, avg: 45 10 - max: 51, min: 36, avg: 38.111111111111114 after: 1 - max: 62, min: 38, avg: 43 2 - max: 60, min: 39, avg: 47.22222222222222 3 - max: 53, min: 40, avg: 43.77777777777778 4 - max: 67, min: 39, avg: 45.666666666666664 5 - max: 48, min: 36, avg: 41.111111111111114 6 - max: 52, min: 38, avg: 43.44444444444444 7 - max: 53, min: 38, avg: 42.55555555555556 8 - max: 55, min: 41, avg: 46 9 - max: 52, min: 41, avg: 45.44444444444444 10 - max: 54, min: 41, avg: 44.333333333333336 Correct testcase?

I must be missing something. Where in that testcase are there 100 nodelists all live at the same time? Pseudocode: for (var i = 0; i < 100; ++i) { gLists[i] = document.getElementsByTagName(i); } // Do performance testing of DOM mutation.

(In reply to comment #46) > I must be missing something. Where in that testcase are there 100 nodelists > all live at the same time? Pseudocode: > > for (var i = 0; i < 100; ++i) { > gLists[i] = document.getElementsByTagName(i); > } > > // Do performance testing of DOM mutation. > In my testcase there is no proposed pseudocode. There is something different: in function start() gNodesArray = document.getElementsByTagName(tagName); and in function loop() I run through those nodes and change their text nodes. Is it Ok?

and then by clickin button 'start' I initialize gNodesArray again by another list of elements.

> Is it Ok? No. It's not. Your testcase has one single nodelist live at a time, effectively. I asked you to test the impact on a situation with a number of live nodelists (which is what you get on a typical DOM-heavy HTML page).

I have 100 node live list, every of them is consisted by 100 nodes. I change text inside of some 100 nodes. I reproduce 5 times these operation. Results: before: 5 828 4 63 3 39 2 27 1 38 1 - max: 828, min: 27, avg: 199 after: 5 828 4 78 3 55 2 42 1 54 1 - max: 828, min: 42, avg: 211.4 Is test ok?

Bz, why does it suddenly jump to 828 when there are 5 nodelists? That's both before or after the patch.

In any case, for 1-4 nodelists this is a significant slowdown when live nodelists are used.

(In reply to comment #51) > Bz, why does it suddenly jump to 828 when there are 5 nodelists? That's both > before or after the patch. > Aaron. It's always 100 nodelist. Test is running on load and we get 828.

(In reply to comment #52) > In any case, for 1-4 nodelists this is a significant slowdown when live > nodelists are used. > regression is about 20%.

> Is test ok? Apart from being way more complicated than what was actually required (and hence a heck of a lot slower to run than you'd really need), yeah. ;) > Bz, why does it suddenly jump to 828 That's the first run though, and this testcase actually forces those nodelists to walk the whole DOM on that run. sicking, thoughts on the impact? Given those numbers I'd be tempted to try it, at least if we think we'll have any other consumers for this...

Yeah, I guess we can try. I wish we had better dhtml tests though :(

But keep a close eye on Tp, and try landing when there's not much other tree activity going on.

Okay, just to be clear, it still needs sr=bz for that.

Waiting on sr=bz before we'll approve to land in 1.9.

fixed Aaron's comment updated to trunk

Comment on attachment 278776 [details] [diff] [review] patch3 -'ing the approval until reviews are completed. Just re-request approval when done. This way, I can quickly clear out the queue. :)

Comment on attachment 278776 [details] [diff] [review] patch3 >Index: content/base/public/nsIMutationObserver.h >+ virtual void CharacterDataWillChange(nsIDocument *aDocument, Please put this before CharacterDataChanged. Same in nsNodeUtils.h. With that, sr=bzbarsky.

checked in with bz comment