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

RESOLVED FIXED in mozilla11

Status

()

Core
Editor
P1
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Art Geigel, Assigned: bz)

Tracking

({perf, testcase})

Trunk
mozilla11
x86
Windows 7
perf, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(7 attachments, 10 obsolete attachments)

896.67 KB, text/html
Details
3.47 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
19.45 KB, patch
Details | Diff | Splinter Review
11.18 KB, patch
Details | Diff | Splinter Review
5.40 KB, patch
Details | Diff | Splinter Review
16.16 KB, patch
Details | Diff | Splinter Review
1.97 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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

6 years ago
Could you attach a reduced html testcase please.

Does this occur in the latest Firefox 4 beta?
(Reporter)

Comment 2

6 years ago
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
(Reporter)

Comment 3

6 years ago
The problem exists in 4beta also.  I just confirmed.

AG3
Component: General → Editor
Product: Firefox → Core
QA Contact: general → editor
Confirmed using Mozilla/5.0 (Windows NT 5.1; rv:2.0b12pre) Gecko/20110220 Firefox/4.0b12pre ID:20110220030351.
Severity: major → normal
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
Version: unspecified → Trunk
Assignee: nobody → ehsan
Keywords: perf
Whiteboard: [post-2.0]
Hmm.  I can't reproduce this on the testcase in comment 2 (on Mac, with a current nightly)...

Comment 6

6 years ago
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
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.
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/bzbarsky@mozilla.com-5a1c9caa43d8>.  I'd really appreciate knowing whether those solve the problems people are seeing!
Depends on: 684638

Comment 9

6 years ago
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.
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.
One other thing worth checking:  Do you see the issue in safe mode?

Comment 12

6 years ago
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

6 years ago
Created attachment 571861 [details]
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

6 years ago
And the tryserver build in comment 8 does not seem to help this problem.
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

6 years ago
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
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...
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.
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.
Created attachment 572156 [details] [diff] [review]
part 1.  Nix the slow IsElementVisible and replace it with a direct primary frame check.
Attachment #572156 - Flags: review?(ehsan)
Created attachment 572157 [details] [diff] [review]
part 2.  Add nsINode overloads of various editor helper methods.
Attachment #572157 - Flags: review?(ehsan)
Created attachment 572158 [details] [diff] [review]
part 3.  Switch GetPriorNodeImpl and GetNextNodeImpl to working with nsINode.
Attachment #572158 - Flags: review?(ehsan)
Created attachment 572159 [details] [diff] [review]
part 4.  Switch GetNextNode and GetPriorNode to working with nsINode.
Attachment #572159 - Flags: review?(ehsan)
Created attachment 572160 [details] [diff] [review]
part 5.  Pass an optional argument to GetPriorNode and GetNextNode to limit the search.
Attachment #572160 - Flags: review?(ehsan)
Created attachment 572161 [details] [diff] [review]
Part 5 for real
Attachment #572161 - Flags: review?(ehsan)
Attachment #572160 - Attachment is obsolete: true
Attachment #572160 - Flags: review?(ehsan)
Created attachment 572162 [details] [diff] [review]
Part 5 with silly whitespace issue fixed too
Attachment #572162 - Flags: review?(ehsan)
Attachment #572161 - Attachment is obsolete: true
Attachment #572161 - Flags: review?(ehsan)
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

6 years ago
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/bzbarsky@mozilla.com-22b56a583a98
Created attachment 572651 [details] [diff] [review]
Part 1 with the necessary IsElement check reinstated so we pass the browserscope editor tests
Attachment #572651 - Flags: review?(ehsan)
Attachment #572156 - Attachment is obsolete: true
Attachment #572156 - Flags: review?(ehsan)
Created attachment 572656 [details] [diff] [review]
Part 2 merged to the part 1 changes
Attachment #572656 - Flags: review?(ehsan)
Attachment #572157 - Attachment is obsolete: true
Attachment #572157 - Flags: review?(ehsan)
Created attachment 572659 [details] [diff] [review]
Part 5 with a necessary check on entry into this code to prevent assertions triggering
Attachment #572659 - Flags: review?(ehsan)
Attachment #572162 - Attachment is obsolete: true
Attachment #572162 - Flags: review?(ehsan)
That last set of patches should not pass all the tests.  I pushed them to try to make sure.
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.)
Attachment #572656 - Flags: review?(ehsan) → review+
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.
Attachment #572158 - Flags: review?(ehsan) → review+
Comment on attachment 572159 [details] [diff] [review]
part 4.  Switch GetNextNode and GetPriorNode to working with nsINode.

Carrying forward the boolean template argument nit.
Attachment #572159 - Flags: review?(ehsan) → review+
Attachment #572659 - Flags: review?(ehsan) → review+
Assignee: ehsan → bzbarsky
Whiteboard: [post-2.0]
Created attachment 572715 [details] [diff] [review]
Part 1 updated to work with LazyFC, as discussed on irc
Attachment #572715 - Flags: review?(ehsan)
Attachment #572651 - Attachment is obsolete: true
Attachment #572651 - Flags: review?(ehsan)
Created attachment 572716 [details] [diff] [review]
Part 2 updated to comments
Attachment #572656 - Attachment is obsolete: true
Blocks: 700538
Created attachment 572718 [details] [diff] [review]
Part 3 updated to comments

Filed bug 700538
Attachment #572158 - Attachment is obsolete: true
Created attachment 572720 [details] [diff] [review]
Part 4 updated to comments
Attachment #572159 - Attachment is obsolete: true
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.
Created attachment 572731 [details] [diff] [review]
Part 5 with those changes
Attachment #572659 - Attachment is obsolete: true
Created attachment 572732 [details] [diff] [review]
Interdiff for part 5
Attachment #572732 - Flags: review?(ehsan)

Comment 43

6 years ago
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/bzbarsky@mozilla.com-7418264b40fb

Comment 44

6 years ago
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/bzbarsky@mozilla.com-95712fff53d1
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 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
Attachment #572715 - Flags: review?(ehsan) → review+
Comment on attachment 572732 [details] [diff] [review]
Interdiff for part 5

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

r=me
Attachment #572732 - Flags: review?(ehsan) → review+
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
Flags: in-testsuite?
Priority: -- → P1
Target Milestone: --- → mozilla11
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
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Depends on: 713427
Depends on: 795708
You need to log in before you can comment on or make changes to this bug.