1448818 - textarea: long text - input cursor positioning slow

Status: NEW

(Core :: Layout: Text and Fonts, defect, P3)

Description

[jonas.zeiger]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180322152034

Steps to reproduce:

1. Open MoinMoin wiki page edit (raw text)
2. Enter text into the provided <textarea> (no special event handlers attached)
3. Ensure there are at least 200 lines of varying length (0 to 150 characters) present
4. Jump close to the end of the text
5. Navigate around in the text using keyboard right-arrow, left-arrow or using direct mouse click



Actual results:

A delay of multiple seconds between keypress and actual cursor position change is observed.

The delay is long enough to make efficient editing impossible.


Expected results:

The delay between keypress/click and actual cursor position change should be smaller or equal to 33ms.

Comment 1

[jonas.zeiger]

Any page that contains a <textarea> can be used, site doesn't matter.

The machine the issue was observed on is a Core i5 with 32GB of RAM.

Comment 2

[jonas.zeiger]

Maybe related: https://bugzilla.mozilla.org/show_bug.cgi?id=1446300

Comment 3

[Gingerbread Man]

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
20180326132207

(In reply to jonas.zeiger from comment #1)
> Any page that contains a <textarea> can be used, site doesn't matter.

1. https://en.wikipedia.org/w/index.php?title=List_of_Falcon_9_and_Falcon_Heavy_launches&action=edit
2. Click the "Start editing" button when the prompt pops up.
3. Keep the Right Arrow key held down for a few moments.
4. Keep the Left Arrow key held down for a few moments.
5. Keep the Down Arrow key held down for a few moments.
6. Keep the Up Arrow key held down for a few moments.
7. Press Ctrl+End.
8. Keep the Up Arrow key held down for a few moments.
9. Keep the Down Arrow key held down for a few moments.

Actual results:
3 & 4. Extremely sluggish caret movement.
5 & 6. Normal caret movement.
8 & 9. Extremely sluggish caret movement.

Comment 4

[Makoto Kato [:m_kato]]

This is spellchecker issue.  When I profile this step, mozInlineSpellChecker::ResumeCheck spends 98% during this steps.

Comment 5

[Makoto Kato [:m_kato]]

Workaround is layout.display-list.retain=false

Comment 6

[jonas.zeiger]

The workaround above fixes the issue for me, just disabling "spell check as you type" did not.

Comment 7

[Makoto Kato [:m_kato]]

Bug 1419519 doesn't resolve by layout.display-list.retain=false, so reopen this issue.

Comment 8

[Makoto Kato [:m_kato]]

layout.display-list.retain=true
https://perfht.ml/2pGATGa

layout.display-list.retain=false
https://perfht.ml/2pIlzZB

Comment 9

[Makoto Kato [:m_kato]]

When turning on layout.display-list.retain, spellchecker spends a lot of times.  If off, it finishes soon...

Comment 10

[Alice0775 White]

Regression window(on win10, set layout.display-list.retain to true. spellcheck is default English enabled):

1st regression:
Text lines with caret in the line will disappear periodically.
Caret movement is super laggy. Caret movement skips several characters.
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ce1a86d3b4db161c95d1147676bbed839d7a4732&tochange=b39904cff06b5b74798214ff8057a0ed2833fb0b


2nd regression(progression):
Text line no longer disappear.
But, Caret movement is still super laggy. Caret movement skips several characters.
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=64adf2b4d0177593194501b17993cb2e6fe935b8&tochange=8b76e07b21bc2b462dd93d40fdb1162cdd7dcdf6

Regressed by: Bug 1404181 and Bug 1411856

Comment 11

[Matt Woodrow (:mattwoodrow)]

I can't reproduce this on OSX, might only happen on Windows/Linux.

Looking at the profiles in comment 8, they seem very similar except that with display-list.retain=false, we frequently get multiple paints in a row without running spellcher. With display-list.retain=true, then we're running the spellcheck every frame, and the frame rate is much lower.

The actual time spent doing spellcheck appears to be identical, almost entirely building text runs (why do we need to do this for a caret move?).

Jonathan, do you have any idea why this might be happening?

Retained display lists would mean that we likely won't call BuildDisplayList on all the frames when we paint, but nsTextFrame::BuildDisplayList doesn't appear to do anything that would matter.

Comment 12

[Jonathan Kew [:jfkthame]]

I'm suspicious this may be related to the code in nsTextFrame::SetSelectedRange (used whenever we refresh the range(s) marked as misspellings), which can trigger a reflow of the frame:

https://searchfox.org/mozilla-central/rev/028cd8342899357d80fafba2d528a0fd5819e316/layout/generic/nsTextFrame.cpp#7686-7690

This is more likely to be an issue on Linux/Windows, where spell-checking uses a wavy underline, than on macOS where it is a straight dotted underline (because the wavy underline, obviously, is more likely to overflow the frame's normal rect).

It seems unfortunate that we mark the frame as requiring reflow here (so we completely re-do line breaking, etc.), because AFAIK the styles that can be applied to a selection are only things like color or text-decoration that do not affect metrics in any way, and therefore will not actually change the layout.

I wonder if we can avoid calling FrameNeedsReflow here, and instead just update the frame's overflow areas? ISTM that might save us a LOT of (redundant) work.

Comment 13

[Matt Woodrow (:mattwoodrow)]

Given that basically all the time here is recreating text runs, that seems like it would make a massive difference.

Who knows this code well enough to give it a shot?

Comment 14

[Makoto Kato [:m_kato]]

This occurs on OSX as long as I test this on my MacBook (not PRO).

Spellchecker removes a lot of spellchecker ranges by Selection::RemoveRange, then RemoveRange calls SelectFrames(.., true) and CombineSelectionUnderlineRect spends a lot of time.  If calling SelectFrames(.., true) is less, performance might be improved.

But didHaveOverflowingSelection seems to be true at a lot of case, so it is unnecessary to call CombineSelectionUnderlineRect  in SetSelectedRange when didHaveOverflowingSelection is true.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sounds like that spellchecker removes existing range first, then, add same range again in most cases. However, looks like that it's not so easy to optimize mozInlineSpellChecker::DoSpellCheck() for preventing redundant range modifications.

One possible quick hack is, making nsTextFrame::SetSelectedRange() ignore didHaveOverflowingSelection. That means that if an nsTextFrame is overflown by spellchecker or IME, it may keep overlapped with next line's nsTextFrame even after it loses all special selections. I think that it may be rare case that this demerit becomes actual performance issue.

Anyway, I don't understand why every selection change causes spell checking again, though.

Comment 16

[Makoto Kato [:m_kato]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4172f9a586afe92d88acb860fc71a299c6f32eec

Comment 17

[Makoto Kato [:m_kato]]

This seems to be some issues
- When turning on spellchecker, spellchecker spends a lot of time (it can be fixed by comment #16's patch)
- When turning off spellchecker, moving caret isn't smooth.

  1. open https://en.wikipedia.org/w/index.php?title=List_of_Falcon_9_and_Falcon_Heavy_launches&action=edit
  2. click the "Start editing" button 
  3. scroll bottom of textbox and set caret to the last line ([[Category:Articles containing video clips]])
  4. Repeat [left arrow]

  - result
  caret by moving isn't rendered per character.  If retained display list is off, it renders per character

Comment 18

[Makoto Kato [:m_kato]]

(In reply to Makoto Kato [:m_kato] from comment #17)
> This seems to be some issues
> - When turning on spellchecker, spellchecker spends a lot of time (it can be
> fixed by comment #16's patch)

I filed as bug 1449828 for this issue.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Wow, even only in Selection::RemoveRange(), Selection::SelectFrames() are called redundantly...
https://searchfox.org/mozilla-central/rev/7e663b9fa578d425684ce2560e5fa2464f504b34/dom/base/Selection.cpp#2372,2404,2416

Comment 20

[Jonathan Kew [:jfkthame]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #13)
> Given that basically all the time here is recreating text runs, that seems
> like it would make a massive difference.
> 
> Who knows this code well enough to give it a shot?

I've pushed a try job at https://treeherder.mozilla.org/#/jobs?repo=try&revision=d30092da3218b49e0bd3697c9237f8840e4bef6d that has a very naïve shot at this; curious to see how it goes.

(I'm suspicious that there may be cases where it won't repaint selection decorations properly, if a changed visual overflow area needs to be propagated to ancestors of the textframe. But it may at least give an idea of the potential impact a change here could have.)

Comment 21

[Alice0775 White]

Attachment: longtext.html

scroll to bottom, move caret with arrow key.... extremely slow if retain-dl enabled :(

Comment 23

[Makoto Kato [:m_kato]]

(In reply to Alice0775 White from comment #21)
> Created attachment 8964237 [details]
> longtext.html
> 
> scroll to bottom, move caret with arrow key.... extremely slow if retain-dl
> enabled :(

This bug and it might be fixed by bug 1450360 according to mozregression.  Does this still occurs on the latest Nightly?

Comment 24

[Alice0775 White]

(In reply to Makoto Kato [:m_kato] from comment #23)
> (In reply to Alice0775 White from comment #21)
> > Created attachment 8964237 [details]
> > longtext.html
> > 
> > scroll to bottom, move caret with arrow key.... extremely slow if retain-dl
> > enabled :(
> 
> This bug and it might be fixed by bug 1450360 according to mozregression. 
> Does this still occurs on the latest Nightly?

The caret lag problem is no longer reproduced on Nightly61.0a1. And I can also confirm the Bug 1450360 fixes this.

Fixed by: 
  d5135503f825	Matt Woodrow — Bug 1450360 - Reland chunk that got accidentally removed during a branch merge. r=mattwoodrow


However, the problem is still reproduce on 60.0b9(next ESR).
@:mattwoodrow,
Could you uplift the patch to 60beta?

Comment 25

[Gingerbread Man]

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
20180405104009

(In reply to Makoto Kato [:m_kato] from comment #23)
> Does this still occurs on the latest Nightly?

Going through the STR at comment 3, there's notable improvement. Maybe enough to consider this fixed, though Left/Right arrow movement is still a little choppy and Up/Down arrow movement actually seems a bit worse than before. It's particularly noticeable when comparing with Vivaldi, which just breezes through the text.

Comment 26

[TGOS]

(In reply to Makoto Kato [:m_kato] from comment #23)
> This bug and it might be fixed by bug 1450360 according to mozregression. 

Bug 1450360 does not really fix anything, it just disables retaining as that is not possible in combination with the dark scrollbars. 

https://hg.mozilla.org/integration/autoland/rev/08c9beb155d5233f1e57073b44e9175ef6231de3

That disabling retaining works is already what you said in comment #5. But that's not what I would call a fix as if no overlay scrollbars are used, retaining is still enabled by default and the cursor will still be sluggish.

A fix is to either kick out retaining completely again and live happy without it or fixing the speed issue caused by it, isn't it?

Comment 27

[Matt Woodrow (:mattwoodrow)]

(In reply to TGOS from comment #26)
> (In reply to Makoto Kato [:m_kato] from comment #23)
> > This bug and it might be fixed by bug 1450360 according to mozregression. 
> 
> Bug 1450360 does not really fix anything, it just disables retaining as that
> is not possible in combination with the dark scrollbars. 
> 
> https://hg.mozilla.org/integration/autoland/rev/
> 08c9beb155d5233f1e57073b44e9175ef6231de3
> 
> That disabling retaining works is already what you said in comment #5. But
> that's not what I would call a fix as if no overlay scrollbars are used,
> retaining is still enabled by default and the cursor will still be sluggish.
> 
> A fix is to either kick out retaining completely again and live happy
> without it or fixing the speed issue caused by it, isn't it?

Right, skipping retaining here doesn't solve the underlying issue.

I think Jonathan's approach here is still what we want.

Comment 28

[Jonathan Kew [:jfkthame]]

Could you please try the experimental build from https://treeherder.mozilla.org/#/jobs?repo=try&revision=cee60a8980318efb0dc7f1115c7a310ceb59e917 and let me know whether it shows any improvement? This should confirm whether the idea in comment 12 is actually relevant and offers hope for a solution.

Comment 29

[Alice0775 White]

(In reply to Jonathan Kew (:jfkthame) from comment #28)
> Could you please try the experimental build from
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=cee60a8980318efb0dc7f1115c7a310ceb59e917 and let me
> know whether it shows any improvement? This should confirm whether the idea
> in comment 12 is actually relevant and offers hope for a solution.

Test with Windows 2012 x64 pgo build, Text disappears temporarily when moving the caret(Especially moving caret upward).
And sometimes, when you click on the text, Text after the click position continuously blinks.


STR:
Open attachment 8964237 [details] and scroll to bottom
Click on the last line to put caret
Keydown [ArrowUp] key and keep hold it

AR:
Text disappears temporarily

Comment 30

[Jonathan Kew [:jfkthame]]

Interesting (though not entirely what I expected) - thanks.

Actually, that sounds like it might be a symptom of bug 1451688, which I think was present in the tree when I did that try push. :( So I've submitted a fresh try build with an updated base revision: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e22a181457b051c9ac614c77c4a068a96dd5aa2. Let's see if that helps.

Comment 31

[TGOS]

The cee60a8980318efb0dc7f1115c7a310ceb59e917 build was fine on macOS 10.13.4, speed was okay, no matter if left/right or up/down but I also saw the disappearing text lines (looks like they are flickering).

The 7e22a181457b051c9ac614c77c4a068a96dd5aa2 build also shows good performance and doesn't show the text flickering anymore.

Comment 32

[Alice0775 White]

(In reply to Jonathan Kew (:jfkthame) from comment #30)
> Interesting (though not entirely what I expected) - thanks.
> 
> Actually, that sounds like it might be a symptom of bug 1451688, which I
> think was present in the tree when I did that try push. :( So I've submitted
> a fresh try build with an updated base revision:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=7e22a181457b051c9ac614c77c4a068a96dd5aa2. Let's see
> if that helps.

Test with Windows 2012 x64 pgo build. it works without any problem.

Comment 33

[Gingerbread Man]

(In reply to Jonathan Kew (:jfkthame) from comment #28)
> Could you please try the experimental build from
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=cee60a8980318efb0dc7f1115c7a310ceb59e917 and let me
> know whether it shows any improvement? This should confirm whether the idea
> in comment 12 is actually relevant and offers hope for a solution.

(In reply to Jonathan Kew (:jfkthame) from comment #30)
> I've submitted a fresh try build with an updated base revision:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=7e22a181457b051c9ac614c77c4a068a96dd5aa2. Let's see
> if that helps.

I've never downloaded try builds before. I didn't notice any download links either there or in the user guides. I'd appreciate some guidance for future reference.
Here's how I went about it: chose Run Single Build in mozregression-gui, build type: pgo, repo: try, build from: cee60a898031, then 7e22a181457b.

Both builds seemed fine to me. I made screen recordings so you can be the judge. I tried attaching them here but that failed. Hopefully you can get to the download link before it automatically expires in 24 hours:
https://send.firefox.com/download/38a17112d8/#v-X63rEJnBTVuKsoH9WxuQ

Comment 34

[TGOS]

(In reply to Gingerbread Man from comment #33)
> I've never downloaded try builds before. I didn't notice any download links
> either there or in the user guides.

When I go to a build link, like

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e22a181457b051c9ac614c77c4a068a96dd5aa2

It shows the performed jobs to the right. Builds jobs have a capital B. So in my case, I need the macOS build, I click the green "B" next to "OS X Cross Compiled opt". Once clicked, it shows "Job Details" at the bottom and these will list "artifact uploaded" tasks; artifacts are files that were produced by running that job and are stored as job results, in case of a build job, this contains the actually build Firefox binary. In my case that would be "target.dmg" that contains the build Firefox.app bundle.

A Windows user would either click the "B" or "Bs" (Bs is a signed build) next to "Windows 2012 x64 pgo" and then download "target.zip" from the build artifacts or "target.installer.exe" to get the installer.

Comment 35

[Gingerbread Man]

(In reply to TGOS from comment #34)
> A Windows user would either click the "B" or "Bs" (Bs is a signed build)
> next to "Windows 2012 x64 pgo" and then download "target.zip"

Thank you. No difference from comment 33. Also, about:buildconfig shows the same "build from" links, so it looks like I got the correct builds via mozregression-gui.
Built from https://hg.mozilla.org/try/rev/cee60a8980318efb0dc7f1115c7a310ceb59e917
Built from https://hg.mozilla.org/try/rev/7e22a181457b051c9ac614c77c4a068a96dd5aa2

Comment 36

[Jonathan Kew [:jfkthame]]

(In reply to Gingerbread Man from comment #33)
> Both builds seemed fine to me. I made screen recordings so you can be the
> judge. I tried attaching them here but that failed. Hopefully you can get to
> the download link before it automatically expires in 24 hours:
> https://send.firefox.com/download/38a17112d8/#v-X63rEJnBTVuKsoH9WxuQ

Thanks, I looked at the screen recordings, and they seemed pretty reasonable to me. I don't know how Nightly (without the experimental patch) looks on your system in comparison, but from your description it sounds like this is a worthwhile improvement.

Comment 37

[afiskon]

I can confirm this issue reproduces with not so large (8.3 Kb of text) blog posts in WordPress on Arch Linux x64 with default binary package of Firefox 61.0.1. The workaround `layout.display-list.retain=false` worked for me. I'm using two spellcheckers (Russian and English) and usually my blog posts contain a lot of "errors" (actually code snippets). I had a similar issue not a long time ago, may be related: https://bugzilla.mozilla.org/show_bug.cgi?id=1444742