635618 - Typing goes incredibly slow in a contentEditable div when there are a lot of divs

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P1)

Description

[Art Geigel]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/534.13 (KHTML, like Gecko) Chrome/9.0.597.98 Safari/534.13
Build Identifier: 3.6.13

When there are several contentEditable divs on a page, typing is incredibly slow when typing in one div, but only on the first line.  Typing takes forever, but if you hit enter to go to the next line within the contentEditable div the typing speed is normal.  It seems that it goes slowly only on the first line.

Reproducible: Always

Steps to Reproduce:
1.Create many contentEditable div objects on the screen (100 should be sufficient)
2.Place the cursor inside one of the div objects to begin typing
3.Typing on the first line goes really slowly.  Hit enter to go to the next line.  Notice that the speed of typing is normal.
Actual Results:  
Slow typing

Expected Results:  
Normal typing speed within a contentEditable div object when there are a lot of contentEditable div objects.

The typing speed should not be any different no matter how many contentEditable div objects there are.  This type of issue is not seen in IE nor Chrome.

Comment 1

[Ed Morley [:emorley]]

Could you attach a reduced html testcase please.

Does this occur in the latest Firefox 4 beta?

Comment 2

[Art Geigel]

You can view the bug by visiting: http://www.groundbooth.com/ff_div_issue.php

Scroll down on the page until you see the "Add Case" section.  Click within the contentEditable div associated with "Case Name" (or whatever) and note how slow the typing runs on the first line.  Hit enter to go to the next and see how typing runs at a normal speed.

I will dl 4beta and test now...

AG3

Comment 3

[Art Geigel]

The problem exists in 4beta also.  I just confirmed.

AG3

Comment 4

[(mostly gone) XtC4UaLL [:xtc4uall]]

Confirmed using Mozilla/5.0 (Windows NT 5.1; rv:2.0b12pre) Gecko/20110220 Firefox/4.0b12pre ID:20110220030351.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Hmm.  I can't reproduce this on the testcase in comment 2 (on Mac, with a current nightly)...

Comment 6

[Chris]

Is there any updates on this bug?

I'm using version 7.0.1 and I'm getting the same problem. My page has over 8000 DOM elements and only one contentEditable div is set when a users edits a section (removed afterwards on save/cancel). I get the slow typing on the first line depending on how far in the DOM is my contentEditable div set. Towards the end of my document it's very slow while setting a contentEditable div at the beginning is fast.
Note that I cannot reproduce the problem in IE or Chrome.

Thanks

Comment 7

[Boris Zbarsky [:bzbarsky]]

The latest update is comment 5:  I tried to reproduce the problem and could not.

If you have a testcase that shows the problem, please attach it to the bug.  That would be very much appreciated.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Chris pointed me to a testcase that he unfortunately can't make public.

What I see on his testcase is that when I first see the contenteditable area there's some lag and right after I click in it there's a few seconds of lag.  After that, typing is fast for me.  If I blur the contenteditable field I get the lag again.  If I focus it after that I get it again.  Chris, is that what you see as well?

On a hunch, I logged spellcheck events, and when the contenteditable area is shown or when I click in it or when I blur it there are three such events posted.  Each one takes about 2 seconds to process.  So you get 6 seconds of lag.  No idea where the "three" comes from, offhand.

Now the good news is that with the patches in bug 684638 there is only one spellcheck event instead of 3, and it takes us 20ms to handle it, not 2 seconds.  At that point I'm not seeing much lag at all.  If people are interested in trying the builds with those patches, they're available at <http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5a1c9caa43d8>.  I'd really appreciate knowing whether those solve the problems people are seeing!

Comment 9

[Chris]

The lag that I see is on the keydown in the contenteditable div area. The problem is not on the inital focusin, since no matter how long I wait or type the keys are showing very slowly.

Again just like the initial post, the problem is on the first line. If I add a line feed and type on the second line or more it's always fast and if I type again on the first line the characters are rendered slowly.

What is more confusing is that I never get this problem with a contentEditable div at the start of my document. If I have one that the rendering is slow and move the DOM object to the beginning of my document the rendering will be normal.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Hmm.  Ok, I definitely can't reproduce that behavior...

I'd really appreciate you trying the builds linked to in comment 8 and reporting back whether you still see the problem in those.

Comment 11

[Boris Zbarsky [:bzbarsky]]

One other thing worth checking:  Do you see the issue in safe mode?

Comment 12

[Chris]

I tried the nightly build you provided above and the issue wasn't there. Everything was working fine. No lag whatsoever.
Safe Mode didn't change anything.

Comment 13

[Alice0775 White]

Attachment: sample.html

This sample contains 9000div and contentEdittable.


[STR]
1. Open sample html
2. Press "k" and hold keydown. 

3. Input Enter key so that the caret go to 2nd line.
4. Press "k" and hold keydown. 

5. Compare speed of the key repeat of step 2 and step 4.


[Actual]
Key in speed of Step2 is slower than Step4.

Comment 14

[Alice0775 White]

And the tryserver build in comment 8 does not seem to help this problem.

Comment 15

[Boris Zbarsky [:bzbarsky]]

OK.  So maybe we have two separate issues here.

Just to check... Chris, if you try a build from http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2011/11/2011-11-01-03-11-08-mozilla-central/ does _that_ show the problem you saw?  That's just this morning's nightly build without any spellchecker fixes.

Alice0775, I'll look at your testcase tomorrow.

Comment 16

[Chris]

No, it's the same issue. The provided file by Alice0775 generates the behaviour that I'm getting. However in my case holding keydown will make Firefox Unresponsive. The rendering of characters on the first line is much slower than in the provided file. But we can still see in the file that Alice0775 provided that keys are rendered much slower on the first line.

I tried to create a much bigger file with some actual content, but at this point either the file is not big enough or structured in a too simple format to generate the "Page Unresponsive" or my file is so big that editing the contentEditable makes the browser crash.

- Build in comment 8 solves the problem for me.
- Build in comment 15 solves the problem, but now the first key of each line is rendered slowly. Holding keydown takes literally over 1 second, before the first key is shown and then the behaviour is normal.
- Safe Mode doesn't change anything to problem found in 7.0.1

Comment 17

[Boris Zbarsky [:bzbarsky]]

OK.  So given comment 16, I think we have the following:

1)  The main issue Chris is seeing is just fixed on trunk already.
2)  The remaining issue (first char on each line) is fixed as of this morning.

Both of those will ship in Firefox 10.

On Alice's testcase, I do see the problem if I squint enough, or if I throttle my CPU back enough.  A profile shows that it's a problem similar to bug 684638 but in editor, not spellcheck.  More details once my debug build finishes.  This should not be particularly hard to fix now that I can reproduce it.  The key is the display:none content, which the groundbooth site also has...

Comment 18

[Boris Zbarsky [:bzbarsky]]

So what happens in Alice0775's testcase is that nsHTMLEditRules::WillInsert and nsHTMLEditRules::CheckInterlinePosition (called from nsHTMLEditRules::AfterEditInner) call nsHTMLEditor::GetPriorHTMLNode which calls nsEditor::GetPriorNode.

Now nsEditor::GetPriorNode has three problems:

1)  It gets computed style.  This is expensive for display:none elements.  Furthermore, it just wants to know whether the display is "none", which is easier to check by checking for a frame, I think.  In fact it's better that way, because display might be none on some ancestor.  And right now it uses a non-flushing computed style lookup, so we wouldn't even need to Flush_Frames.

2)  nsIDOMNode everywhere.

3)  It walks right out of the contenteditable area.

#1 is trivial to fix.  #2 is not that hard.  Thinking about #3.

Comment 19

[Boris Zbarsky [:bzbarsky]]

OK, patches coming up.  Without them the time spent under GetPriorNode is about 65% of the time needed to process the keypress for me.  The first patch (addressing #1 above) cuts down that number to about 20%.  The next 3 patches (addressing #2 above) cut it down to about 6%.  The last patch (addressing #3 above) cuts it down to under 0.1% for sure; hard to say more exactly at that point because the random sampling noise probably dominates the data.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Nix the slow IsElementVisible and replace it with a direct primary frame check.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Add nsINode overloads of various editor helper methods.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Switch GetPriorNodeImpl and GetNextNodeImpl to working with nsINode.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Switch GetNextNode and GetPriorNode to working with nsINode.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Pass an optional argument to GetPriorNode and GetNextNode to limit the search.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 5 for real

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 5 with silly whitespace issue fixed too

Comment 27

[Boris Zbarsky [:bzbarsky]]

Ehsan, you probably want to wait on the reviews a bit; the tests are triggering at least some of the asserts I added.  I'll look into that on Monday.

Comment 28

[Mozilla RelEng Bot]

Try run for 22b56a583a98 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=22b56a583a98
Results (out of 192 total builds):
    success: 163
    warnings: 29
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-22b56a583a98

Comment 29

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1 with the necessary IsElement check reinstated so we pass the browserscope editor tests

Comment 30

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 merged to the part 1 changes

Comment 31

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 5 with a necessary check on entry into this code to prevent assertions triggering

Comment 32

[Boris Zbarsky [:bzbarsky]]

That last set of patches should not pass all the tests.  I pushed them to try to make sure.

Comment 33

[(no longer active)]

Comment on attachment 572656 [details] [diff] [review]
Part 2 merged to the part 1 changes

Having to add these overloads is really sad, but I'm not going to ask you to fix the entire editor code.  r=me.

(Nit: please use for(;;) instead of do/while(1), and adjust the assertion comment following those two occurrences.)

Comment 34

[(no longer active)]

Comment on attachment 572158 [details] [diff] [review]
part 3.  Switch GetPriorNodeImpl and GetNextNodeImpl to working with nsINode.

Nit: please pass goForward as an argument to FindNextLeafNode and make it non-template.

Carrying forward the do/while nit.

Also please file a follow-up to get nsHTMLEditor::IsRootNode overrides checking against the active editable area root.

Comment 35

[(no longer active)]

Comment on attachment 572159 [details] [diff] [review]
part 4.  Switch GetNextNode and GetPriorNode to working with nsINode.

Carrying forward the boolean template argument nit.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1 updated to work with LazyFC, as discussed on irc

Comment 37

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 updated to comments

Comment 38

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3 updated to comments

Filed bug 700538

Comment 39

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 4 updated to comments

Comment 40

[Boris Zbarsky [:bzbarsky]]

I was updating part 5, when I realized that my previous try run still had two oranges.

I looked at those, and found that:

1)  editor/libeditor/base/crashtests/633709.xhtml ends up with two <input> elements.  Then
    it calls nsHTMLEditor::GetPriorHTMLNode with |inParent| being one of the <input>s 
    while activeEditingHost is the _other_ <input>.  That seems pretty broken, and
    triggers asserts about bogus aCurrentNode not descending from editing root arguments.
2)  editor/libeditor/html/crashtests/582138-1.xhtml calls the (parent, offset) version of
    GetPriorNode with that parent being the parent of our editable root and the offset
    being 0 (which is the offset of the editable root in that parent).  This ends up
    calling the (currrent-node) version of GetPriorNode with that parent as the argument,
    which asserts.

I suppose I could deal with both cases by checking that the input node is a descendant of the root in the (current-node) version of GetPriorNode.  I'll do that for now.

Comment 41

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 5 with those changes

Comment 42

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff for part 5

Comment 43

[Mozilla RelEng Bot]

Try run for 7418264b40fb is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=7418264b40fb
Results (out of 214 total builds):
    success: 189
    warnings: 25
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-7418264b40fb

Comment 44

[Mozilla RelEng Bot]

Try run for 95712fff53d1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=95712fff53d1
Results (out of 216 total builds):
    success: 195
    warnings: 20
    failure: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-95712fff53d1

Comment 45

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 572720 [details] [diff] [review]
Part 4 updated to comments

>--- a/editor/libeditor/base/nsEditor.cpp
>+++ b/editor/libeditor/base/nsEditor.cpp
>-  nsCOMPtr<nsIDOMNode> candidate =
>-    do_QueryInterface(FindNextLeafNode(currentNode, false, bNoBlockCrossing));

>-    nsCOMPtr<nsIDOMNode> notEditableNode = do_QueryInterface(candidate);

\o/

Comment 46

[(no longer active)]

Comment on attachment 572715 [details] [diff] [review]
Part 1 updated to work with LazyFC, as discussed on irc

Review of attachment 572715 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 47

[(no longer active)]

Comment on attachment 572732 [details] [diff] [review]
Interdiff for part 5

Review of attachment 572732 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 48

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6adf89aa521
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4614bcc4ad8
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc4a60869275
https://hg.mozilla.org/integration/mozilla-inbound/rev/4af87ec88918
https://hg.mozilla.org/integration/mozilla-inbound/rev/68ed3842525d

Comment 49

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/c6adf89aa521
https://hg.mozilla.org/mozilla-central/rev/b4614bcc4ad8
https://hg.mozilla.org/mozilla-central/rev/fc4a60869275
https://hg.mozilla.org/mozilla-central/rev/4af87ec88918
https://hg.mozilla.org/mozilla-central/rev/68ed3842525d