Amp-wp: Lag when dragging elements

[swissspidy]

Bug Description

In #3060 it was mentioned:

• It seems to start lagging after adding a few blocks, and generally behaves so that the usability is not the best, it became not possible to drag.
• I have seen that for a while actually. It's in develop too and have been for about a week
• But when there's multiple elements on the page, you can't select-and-drag in one click (not always at least). You have to click to select first, then drag.

The last part might be covered by #3455, not sure though.

Expected Behaviour

Consistently smooth interactions without lagging. No issues with selecting and dragging blocks.

Steps to reproduce

1. Go to '...'
2. Click on '....'
3. Scroll down to '....'
4. See error

Screenshots

Additional context

• WordPress version: 5.3 Beta
• Plugin version: current develop
• Gutenberg plugin version (if applicable): 6.6
• OS: macOS
• Browser: Chrome

---

_Do not alter or remove anything below. The following sections will be managed by moderators only._

Acceptance criteria

• AC1: When dragging a block, the block should move, along with the cursor, in real-time, so no lag is experienced on the screen.

Implementation brief

Two optimizations are recommended for getting smoother snap line display:

• _Debounce_ setSnapLines by 10 ms.
• We currently make way more snap lines than "normal" tools (using Google Slides for comparison here). We currently allow _edge-to-edge_, _center-to-center_, _edge-to-center_ and _center-to-edge_ snap lines. We should reduce this by only allowing _edge-to-edge_ and _center-to-center_ snap lines.
• 
  
  • The easiest way to do this is to split snap targets in two for each direction, e.g. separate horizontalSnaps in horizontalEdgeSnaps and horizontalCenterSnaps in with-snap-target.js. And consequently use these separately for calculations when finding optimal snap lines in getBestSnapLines.
  
  
• 
  
  • Do note however, that snapping _edge-to-center-of-page_ is normal. It's only snapping _edge-to-center-of-other-object_, that's weird. Snapping _center-to-edge-of-page_ is still weird though, so centers should only ever snap to centers of other things.
  
  

QA testing instructions

Verify smooth drag without lagging:

• Add several elements to a page.
• Drag any one around and see no delay while snap lines are calculated.

Verify proper snap lines between element and page:

• Add an image to a page.
• Drag it so that the left edge matches page left edge and see a (very narrow) snap line.
• Similarly for right, top and bottom edge at page ditto.
• Drag it so that the left edge matches page center and see a snap line.
• Similarly for right, top and bottom edge at page center.
• Drag it so that image center is over center of page in both direction and see both snap lines displayed.
• Drag it so that image center is over left edge of page and see no snap line.
• Similarly for image center at right, top and bottom edge of page.

Verify proper snap lines between two elements:

• Add two images, A and B, to a page.
• Drag image A so that left edge matches left edge of B and see a snap line.
• Similarly for left-to-right, right-to-right and right-to-left.
• Drag image A so that left edge matches center of image B and see no snap line.
• Similarly for right-to-center.
• Drag image A so that vertical center matches center of image B and see a snap line.
• Drag image A so that vertical center matches left edge of image B and see no snap line.
• Similarly for center-to-right.
• Repeat for up-down and see snap lines between edge-to-edge and center-to-center and no snap lines between edge-to-center and center-to-edge.

Demo

• https://youtu.be/HlaS6AzwknE

Addressed by

• #3514

Changelog entry

• Improve performance when dragging elements

[spacedmonkey]

Also having some mega lag issues. Created a video that shows the lags.
https://youtu.be/msGZkNMrU3Y

[miina]

Starting looking into this (Implementation Brief).

[miina]

I'm looking into this and when I disable Snapping then the lagging does not happen. So it still seems to me as if this is related to Snapping.

@barklund You mentioned that you had seen the lagging for a while already, already pre-snapping. Do you happen to remember if you saw this with dragging or somewhere else as well?

[barklund]

@barklund You mentioned that you had seen the lagging for a while already, already pre-snapping. Do you happen to remember if you saw this with dragging or somewhere else as well?

I don't remember the circumstances, sorry.

[miina]

Looks like setSnapLines is the thing that makes it lag.

Q: Perhaps we could detect if the cursor is moving slowly or has stopped, and only then do the snapping thing? It's likely that the cursor (user) would slow down when trying to position, or WDYT?

It might make the user experience better as well, right now there are so many lines moving around that it's quite confusing (to me at least).

Thoughts?

[miina]

Additionally, I'm trying to think of a reasonable way to call setSnapLines less frequently, perhaps if the cursor has moved only 1px meanwhile then we shouldn't call setSnapLines again? This could bring inconsistencies though.

Not sure what could be a reasonable logic for setting the state less frequently. Perhaps even do it every X ms instead?

Any ideas/thoughts are welcome!

[barklund]

snapLines bleeds through to the div element, maybe that's the problem. None of the withSnapLines components actually use this prop, so it can be removed safely from the HoC (and even from the context provider). Maybe that helps?

If think that here were no problems with this early on, when the components actually used this property?

[swissspidy]

snapLines bleeds through to the div element, maybe that's the problem.

Also reported in #3462. If it's not needed, removing sounds good to me.

[miina]

@barklund If I understood correctly then you meant removing this, is that correct?
https://github.com/ampproject/amp-wp/blob/a9b50d973e089290ae5ed5795717f277051ac076/assets/src/stories-editor/components/contexts/snapping/index.js#L28

If that's correct, then I tried removing this, it doesn't seem to make any change, still lagging.
It seems like just calling setSnapLines, even if I try with always setting it to [] instead of the actual snap lines, makes the dragging lag.

Q: Is it generally a "normal" use-case to use useState hundreds of times per second?

[spacedmonkey]

I have updated the PR at #3484 so that the fix suggested by Miina.

As noted, this change doesn't improve performance noticeably. But it is still a valid change, as does fix the notice error.

[spacedmonkey]

Q: Is it generally a "normal" use-case to use useState hundreds of times per second?

I would say no. Calling the store should be seen as slow / unreliable, depending the size of the store. Is there anyway to limit the number of calls to store? Either with batching or a simple timeout?

[miina]

Is there anyway to limit the number of calls to store? Either with batching or a simple timeout?

Also had a similar and some additional ideas, in this and this comment.

Would like to know first if calling the useState hundreds of times per second is the issue here or if there might be something else :)

@barklund Since you probably have more experience with hooks, then .. Based on your experience, do you think that just calling useState that often might cause the lag, or is it a common use-case?

[barklund]

In the initial implementation I made, there was the same updating of the local state inside the context on every mouse move and it had no noticeable lag.

Updating an internal state locally in a component is cheap. Updating the global registry is expensive, but I don't think it's doing that.

Have you tried using the built-in profiler to hunt for the problematic functions?

[miina]

Have you tried using the built-in profiler to hunt for the problematic functions?

No, I haven't, I've so far hunted the problematic functions manually and detected that setSnapLines makes it lag.
I've tested the following:

1. Instead of using setSnapLines with the actual value (and removing getBestSnapLines at all, setting it to [] -- still lagging.
2. Using setSnapLines with [] and additionally _not using the set value for anything_ -- still lagging
3. Not using setSnapLines at all but doing everything else (getBestSnapLines, etc.) -- lagging is gone!

So, from that, I concluded that setSnapLines is the culprit.

Not sure what else (in the plugin) might influence this.

Have you tried using the built-in profiler to hunt for the problematic functions?

Do you mean the Chrome Profiler for example? _Will check now_

[swissspidy]

Do you mean the Chrome Profiler for example? Will check now

I think he meant the one in React dev tools

[miina]

Tried the built-in (not React) profile and looks like it's the setSnapLines (dispatchAction) if I debugged correctly.

[barklund]

I figured it out. Debounce helped a lot, but I also recommend making less potential snap lines as described above. Please review @swissspidy (you probably understand the current code the best).

[spacedmonkey]

Debounce setSnapLines by 10 ms.

Why 10ms? Just interested how you came to that number. Why not 15 or 20?

[swissspidy]

IB looks good to me. The 10ms question is valid though, but I assume you got to that by trial and error. We can play with it and tweak it as necessary when doing the actual implementation. Important is that we're doing the debouncing in the first place.

[swissspidy]

Moved to the execution board now 👍

[miina]

It needs an estimation first.

[swissspidy]

🤦‍♂ I got too excited here I guess

[spacedmonkey]

Does this really need estimations? The work is basically done.

[miina]

Yep, everything needs an estimation. Also, the Implementation Brief has more than just adding debounce -- it's also for snapLines.

If it would be basically done then it could have XS as the estimation, however, the estimation should also include reducing the amount of snaplines.

[barklund]

Does this really need estimations? The work is basically done.

The second part about splitting into edge and center snap lines is not done, only described. It's only a few hours though, but require updating helper functions and unit tests for said functions, so XS or even S is probably appropriate.

[miina]

Added an estimation and sending it to the execution board now :)

[barklund]

Debounce setSnapLines by 10 ms.

Why 10ms? Just interested how you came to that number. Why not 15 or 20?

Trial-and-error in deed. 5ms was still a bit laggy when moving quickly around. 20ms felt too delayed. But feel free to play around.

[spacedmonkey]

Trial-and-error in deed. 5ms was still a bit laggy when moving quickly around. 20ms felt too delayed. But feel free to play around.

Fair enough. I trust you picked the best value. Just wondered.

[swissspidy]

See #2992 for general snapping screencast.

[csossi]

still lagging

video - https://drive.google.com/file/d/1Nlj9UzKYckZNIKoqRIsj4zbhXbPKTglE/view
win10/chrome - Version 77.0.3865.120 (Official Build) (64-bit)

[miina]

@csossi Could you test this again just in case? Not seeing the issue, tried both locally and in the test environment.

If you're still seeing the issue, could you please link the Story you were testing with as well?

Thank you!

[swissspidy]

Also interesting would be if the lagging continues when holding down the alt/option key while dragging.

[csossi]

Verified in QA