390414 - text-changed:delete event details no longer correct effective 26th July build

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

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

Comment 1

[alexander :surkov (:asurkov)]

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

Comment 2

[Joanmarie Diggs]

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

Comment 3

[Aaron Leventhal]

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

Comment 4

[Aaron Leventhal]

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.

Comment 5

[Aaron Leventhal]

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.

Comment 6

[alexander :surkov (:asurkov)]

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

Comment 7

[Aaron Leventhal]

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.

Comment 8

[alexander :surkov (:asurkov)]

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

Comment 9

[Aaron Leventhal]

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?

Comment 10

[Aaron Leventhal]

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

Comment 11

[alexander :surkov (:asurkov)]

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.

Comment 12

[alexander :surkov (:asurkov)]

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

Comment 13

[Aaron Leventhal]

> 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.

Comment 14

[alexander :surkov (:asurkov)]

(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?

Comment 15

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 16

[Aaron Leventhal]

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.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 18

[Boris Zbarsky [:bzbarsky]]

Right.  The suggestion is to have a CharacterDataWillChange.

Comment 19

[Aaron Leventhal]

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

Comment 20

[alexander :surkov (:asurkov)]

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?

Comment 21

[alexander :surkov (:asurkov)]

Attachment: patch

does this help?

Comment 22

[Aaron Leventhal]

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

Comment 23

[alexander :surkov (:asurkov)]

(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 24

[alexander :surkov (:asurkov)]

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

foxevent shows correct info.

Comment 25

[Boris Zbarsky [:bzbarsky]]

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.

Comment 26

[Aaron Leventhal]

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?

Comment 27

[Aaron Leventhal]

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);
  }

Comment 28

[Boris Zbarsky [:bzbarsky]]

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.

Comment 29

[Aaron Leventhal]

Surkov, you can write that test in JS.

Comment 30

[Aaron Leventhal]

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

Surkov, can you change it to use rendered text length instead of content length?

Comment 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 32

[alexander :surkov (:asurkov)]

Attachment: perf test

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.

Comment 33

[Boris Zbarsky [:bzbarsky]]

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

Comment 34

[alexander :surkov (:asurkov)]

(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"?

Comment 35

[Boris Zbarsky [:bzbarsky]]

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....

Comment 36

[alexander :surkov (:asurkov)]

Attachment: perf test2

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%.

Comment 37

[alexander :surkov (:asurkov)]

Attachment: patch2

fixed Aaron's comment

Comment 38

[Aaron Leventhal]

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

Comment 39

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 40

[Aaron Leventhal]

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.

Comment 41

[Boris Zbarsky [:bzbarsky]]

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.

Comment 42

[Aaron Leventhal]

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.

Comment 43

[Aaron Leventhal]

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 44

[Aaron Leventhal]

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

Comment 45

[alexander :surkov (:asurkov)]

Attachment: perf test3

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?

Comment 46

[Boris Zbarsky [:bzbarsky]]

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.

Comment 47

[alexander :surkov (:asurkov)]

(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?

Comment 48

[alexander :surkov (:asurkov)]

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

Comment 49

[Boris Zbarsky [:bzbarsky]]

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

Comment 50

[alexander :surkov (:asurkov)]

Attachment: perf test4

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?

Comment 51

[Aaron Leventhal]

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

Comment 52

[Aaron Leventhal]

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

Comment 53

[alexander :surkov (:asurkov)]

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

Comment 54

[alexander :surkov (:asurkov)]

(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%.

Comment 55

[Boris Zbarsky [:bzbarsky]]

> 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...

Comment 56

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 57

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 58

[Aaron Leventhal]

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

Comment 59

[Damon Sicore (:damons)]

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

Comment 60

[alexander :surkov (:asurkov)]

Attachment: patch3

fixed Aaron's comment
updated to trunk

Comment 61

[Damon Sicore (:damons)]

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 62

[Boris Zbarsky [:bzbarsky]]

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.

Comment 63

[alexander :surkov (:asurkov)]

checked in with bz comment