Fix block & block toolbar vibration on block move #22640
Conversation
ellatrix
requested review from
ajitbohra,
chrisvanpatten,
jorgefilipecosta and
youknowriad
as code owners
|
Btw, this does NOT fix the scroll displacement that sometimes happens on arrow up. Let's look into that separately. I think it has to do with the toolbar being inside thee scroll container. |
|
Size Change: -82 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
ZebulanStanphill
added
the
[Type] Bug
|
This is pretty wild and incredibly incredible. There is zero vibration, the block is locked in place while moving. Let me pause for a brief applause here. Well done. I'm seeing one issue, though. It's a little hard to explain, so here's a GIF:
As best I can tell, here's how to reproduce:
What appears to happen is that the block toolbar is attached to another block than the one you selected, and when you mouse over the block toolbar to show the mover, the block toolbar seems to jump, thinking it's still attached to the one you actually selected. As I said, it's hard to explain, but try it out and be sure to scroll down the page before you select a block. It's only the first block you select after having scrolled, which has this issue. If you select other blocks after having scrolled, things work fine. I'm not seeing that issue in master. Other than that, this seems super duper solid, impressive work! |
packages/block-editor/src/components/use-moving-animation/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/use-moving-animation/index.js
Outdated
Show resolved
Hide resolved
|
@jasmussen Does the bug appear with the demo content? Are you using Chrome? I tried this in Chrome but I don't see any scrolling on select. |
Chrome 83 stable, Mac, happened regardless of content. However I can no longer reproduce 🎉 So it's very possible that it was a ghost on my side, or a bad npm install, or something else. I will return if I find a way to reproduce, however since I can't do that anymore, all this needs is a code review it seems! 👍 👍 |
packages/block-editor/src/components/use-moving-animation/index.js
Outdated
Show resolved
Hide resolved
|
Aside from the issue @jasmussen pointed out, this feels really good. Hoping to see this finished and merged soon! |
packages/block-editor/src/components/use-moving-animation/index.js
Outdated
Show resolved
Hide resolved
|
@youknowriad That's the essence of it. :) |
ellatrix
force-pushed
the
try/scroll-animation
branch
from
c898728 to
ab2ca39
Compare
|
Restored observing in the popover for the horizontal animation of the block and toolbar. There's something broken with that animation, but I see the same in master, so let's look into that separately. |
ellatrix
force-pushed
the
try/scroll-animation
branch
from
ab2ca39 to
b07cdee
Compare
| moverDirection === 'horizontal' && node | ||
| } | ||
| // Observe movement for block animations (especially horizontal). | ||
| __unstableObserveElement={ node } |
There was a problem hiding this comment.
Seems like this is the same thing as anchorRef. I wonder if we should instead observe anchor ref when it's a DOM element. I also wonder whether this means the interval inside Popover is useless.
Description
Fixes #20793.
The trick here is to only animate with whole pixels, because scrolling doesn't allow sub pixel positions. This also allows us to simplify popover code.
Best is to test for yourself on a retina screen.
Before:
After:
How has this been tested?
Screenshots
Types of changes
Checklist: