Make pads with long lines usable with chrome. #1833
Conversation
- Do not use incorpIfQuick on keyup because it's not ever quick on chrome. Calling incorpIfQuick calls incorporateUserChanges which sets a flag on the current callstack state indicating that the selection has changed (since something was typed). Whenever this flag is set, the event handler will run code to update the selection (and possibly scroll the view as well), which is a very costly operation in webkit browsers. Instead let the user changes be incorporated by the idle worker, scheduling it to run ASAP on keyup. This isn't a perfect solution, but may make pads running on webkit browsers more usable with otherwise fairly unnoticeable changes in the UI.
|
Related to #1763. This isn't a perfect fix, but it should make etherpads with a lot of lines more usable on webkit browsers like chrome. This patch essentially prevents the really slow webkit selection and node-querying code from running on every keyup event. Instead, the code will run whenever the idle worker runs, which is scheduled to run immediately after each keyup event. This translates into a maximum of every 4ms in most browsers (as mandated by HTML5). |
|
Without looking at the code I'm impressed: seems like a pretty elegant |
|
Any chance we can provide a test to cover this? |
|
@JohnMcLear, I'll try to adapt the test you created here: https://github.com/ether/etherpad-lite/blob/slow-chrome/tests/frontend/specs/responsiveness.js I'm pretty sure that test isn't actually simulating sending keyup events. Editing is still slow with that many lines on the screen (with the above patch) in chrome, but it isn't completely unusable as it is currently. Profiling also no longer reveals that the browser is waiting on browser selection functions, instead it just places all of the time in "(program)" ... which isn't particularly helpful. I think a major redesign (or bug fixes to webkit browsers) are necessary to fully resolve this issue. The above patch is just meant to help alleviate some of the pain. It's also more helpful when there are fewer characters per line (but still many lines), which is probably a more common case than what the test is currently doing. In any event, I'll try to make some changes and get a reasonable test working that at least demonstrates the improvement. |
- The test now ensures that all three key events are fired when sending keys. Previously, only the 'keypress' event was sent, which failed to trigger very slow code on webkit browsers (as it is triggered by 'keyup'). All three events should really be sent whenever sending keys to the browser to ensure that we're adequately testing real behavior. See the 'sendkeys' plugin for more; it only sends 'keypress'.
|
So on my test machine (Intel(R) Core(TM) i7-3770 CPU @ 3.40GHz), that new test sends 100 simulated key strokes (with all three key events, 'keyup', 'keypress', and 'keydown') to a pad with a lot of lines and does so in under 200ms (sometimes around 100ms). Without the patch, it takes over 1000ms. If you change it to do 1000 key strokes, it takes a little over 300ms with the patch and over 8000ms without. So there is clear improvement here, but, like I said above, it's non-optimal. |
|
Okay I should be able to test this tonight |
…down Make pads with long lines usable with chrome.
|
Thanks! :) |
|
@JohnMcLear, sure! :) |
|
awesome. thank you! |
|
The tests fail in IE: https://travis-ci.org/ether/etherpad-lite/builds/9387991#L357 |
on chrome. Calling incorpIfQuick calls incorporateUserChanges
which sets a flag on the current callstack state indicating
that the selection has changed (since something was typed).
Whenever this flag is set, the event handler will run code
to update the selection (and possibly scroll the view as
well), which is a very costly operation in webkit browsers.
Instead let the user changes be incorporated by the idle worker,
scheduling it to run ASAP on keyup. This isn't a perfect
solution, but may make pads running on webkit browsers more
usable with otherwise fairly unnoticeable changes in the UI.