-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Typewriter experience #16460
Typewriter experience #16460
Conversation
I like this a lot. I like that:
Here's a GIF of the iOS 12 simulator with this: Even though the above is a little jumpy, it is VASTLY better than what is in master now, which is nigh unusable. Thanks so much for working on this, mobile is critical path. I am in the process of installing the beta version of Xcode so I can test in iOS 13. Some changes have been made to how scrolling containers work there, and I want to test whether the jumpiness is the same or not in that. I'll return with more info. But for now, 👍 👍 👍 👍 |
packages/block-editor/src/components/block-list/moving-animation.js
Outdated
Show resolved
Hide resolved
@jasmussen Great! I tried a lot of different thing today to try to get rid of the jumpiness on iOS (which was there previously as well, and also, nowhere else do I see this problem), but unfortunately I find nothing that fixes it. Something I tried was scroll-locking the container at key and releasing it at keyup, but that doesn't change anything. It would also not be reliable to do that; the key down event can be cancelled resulting it to be locked forever. I'd appreciate other ideas for the iOS issue. Anyway, this is already much better. :) |
I strongly agree that what you have here is so much better! I think the jumpiness comes from the soft keyboard imperceptibly quickly opening and closing. The soft keyboard on iOS is notoriously hard to work with. When the caret is in text, it stays open, when something that isn't text has focus, it closes. My theory is that because the caret jumps from one text block to a new empty richtext field, in the transition there is a brief period where focus isn't in text, starting the soft keyboard closing animation. |
@jasmussen Ah, that's a good point... I wonder if we can somehow trick it into thinking it stays in a text field. :) Probably not. |
@jasmussen You're right, when you use the arrow icons to tab through the blocks, I observe the same jumpiness. |
Changed it now so that the scroll adjustment a lot smoother. With that I mean for everything except iOS. Before you might have sometimes seen the screen flicker. This should now be gone. :) |
Strange that the tests I created are again successful locally but fail on Travis... |
The only thing left here is figuring out while the newly added tests are failing with Travis (and pass locally). |
e3a8c63
to
357b53d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a commit that removes the usage useSelect inside useMovingAnimation hook.
In my tests, I found a small regression.
If we add many blocks (more than the visible area), and then we multiple select a large chunk of blocks and delete them by pressing backspace, the last blocks available blocks become invisible.
@jorgefilipecosta The multi-select issue should be resolved. The last thing left here seems to be getting the new e2e tests to work on Travis. |
@youknowriad Is that in Chrome or Firefox? |
It's Chrome |
@youknowriad It looks like it's caused by the 100ms debounce. The caret position will not have updated during scrolling (and continued smooth scrolling), and not until 100ms after it ends. It seems that this is debouncing is just not the way to go here. I'll rewrite this part with continuous animation frame requests. |
a337362
to
3d7e9e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Did you try running the performance tests and compare with master? I don't really expect a change but you never know.
There's no noticeable difference between
|
It's interesting to see how varying the loading times are... almost a 10% faster with this branch, but probably because of some random network or CPU thing? |
Thanks for the reviews everyone! Let's expose this more and iterate if there's any bugs found. |
:keanu-whoah: ƪ(˘⌣˘)┐ ƪ(˘⌣˘)ʃ ┌(˘⌣˘)ʃ |
I'm surprised to see the e2e tests pass locally and on Travis without the need to manually wait the next animation frame after a scroll event. If there turn out to be intermittent failures, it's probably related to the scrolling and can be fixed easily. |
CSS animations are disabled in e2e tests both locally and in travis. Spring-js animations are also disabled on travis and you disable them locally by using an env variable. |
@youknowriad I'm not talking about the CSS animations, but about the animation frame requested during scroll. :) |
😅 |
Beautiful work, @ellatrix! 👏 |
I just tested the RC-1 and it's a really cool feature. Thanks ;-) |
* Typewriter XP * Address feedback * Do not maintain scroll position when it's out of view * Reset scroll position when aborting * Set initial trigger % * Add test for viewport * Clean up * Adjust doc * Return children for IE * Hide bottom space when there are metaboxes * Debounce scroll with RAF
* Typewriter XP * Address feedback * Do not maintain scroll position when it's out of view * Reset scroll position when aborting * Set initial trigger % * Add test for viewport * Clean up * Adjust doc * Return children for IE * Hide bottom space when there are metaboxes * Debounce scroll with RAF
* Typewriter XP * Address feedback * Do not maintain scroll position when it's out of view * Reset scroll position when aborting * Set initial trigger % * Add test for viewport * Clean up * Adjust doc * Return children for IE * Hide bottom space when there are metaboxes * Debounce scroll with RAF
Description
This change maintains the caret (insertion point) position while the user is typing.
Text wrapping and
Enter
.iOS (there's still movement, but MUCH better). Apologies, Simulator doesn't correctly set the correct keyboard layout...
How has this been tested?
Make sure there is enough content so that the page is scrollable.
On various devices test:
Enter
Shift+Enter
Backspace
Screenshots
Types of changes
Checklist: