Skip to content
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

Merged
merged 11 commits into from Aug 20, 2019

Conversation

@ellatrix
Copy link
Member

commented Jul 8, 2019

Description

This change maintains the caret (insertion point) position while the user is typing.

Text wrapping and Enter.

typewriter

iOS (there's still movement, but MUCH better). Apologies, Simulator doesn't correctly set the correct keyboard layout...

m-type

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
  • Text wrapping (when text wraps to a new line, the position should update).

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@ellatrix ellatrix requested review from gziolo, talldan and youknowriad as code owners Jul 8, 2019

@ellatrix ellatrix requested review from jasmussen and mtias Jul 8, 2019

@mtias mtias added the Writing Flow label Jul 8, 2019

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

I like this a lot.

I like that:

  • The iOS editing experience is VASTLY improved (going from broken to usable) in this PR.
  • I like that the solution works the same across mobile and web.
  • I like that the Enter behavior works regardless of where you are in the document.

Here's a GIF of the iOS 12 simulator with this:

iphone

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, 👍 👍 👍 👍

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2019

@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. :)

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

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.

@ellatrix ellatrix requested review from aduth, ajitbohra, nerrad and ntwb as code owners Jul 8, 2019

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2019

@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.

@ellatrix ellatrix added this to the Gutenberg 6.1 milestone Jul 8, 2019

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2019

@jasmussen You're right, when you use the arrow icons to tab through the blocks, I observe the same jumpiness.

@ellatrix ellatrix force-pushed the try/typewriter branch from 8b9f954 to a6ebbaf Jul 9, 2019

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

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. :)

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

Strange that the tests I created are again successful locally but fail on Travis...

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2019

The only thing left here is figuring out while the newly added tests are failing with Travis (and pass locally).

@jorgefilipecosta jorgefilipecosta force-pushed the try/typewriter branch from e3a8c63 to 357b53d Jul 18, 2019

@jorgefilipecosta
Copy link
Member

left a comment

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.

Jul-18-2019 13-12-25
(after backspace the last visible block should be the 60)

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

@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
Copy link
Contributor

left a comment

This is looking way better. Two last remarks from me.

  • When used with metaboxes, there's a lot of empty space, I wonder if we should disable this if we have metaboxes.
  • I still feel like we may want to keep the animation.
@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

When used with metaboxes, there's a lot of empty space, I wonder if we should disable this if we have metaboxes.

I can remove just the space when there are meta boxes. Looks like the meta boxes themselves will give enough space.

I still feel like we may want to keep the animation.

Other opinions on the animation? I just don't like it at all on typing. 🤷‍♀️

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

I tend to agree with no animation on typing, simply because I can't recall it happening in any other editor I've used. However I think it would be fine to test this out in a separate PR, so we can sort of compare the two side by side.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

In some occasions, I scroll and start typing, the scroll position reverts to the previous one (before scrolling). It doesn't happen consistently though, I reproduce it 50% of the time.?

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

In some occasions, I scroll and start typing, the scroll position reverts to the previous one (before scrolling). It doesn't happen consistently though, I reproduce it 50% of the time.?

Could you make a GIF or video?

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

scroll type

Notice in the gif above, it doesn't happen consistently. (The jumps) It happens twice in that video.

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

@youknowriad Is that in Chrome or Firefox?

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

It's Chrome

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

@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.

@ellatrix ellatrix force-pushed the try/typewriter branch from a337362 to 3d7e9e1 Aug 20, 2019

@youknowriad
Copy link
Contributor

left a comment

LGTM 👍

Did you try running the performance tests and compare with master? I don't really expect a change but you never know.

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

There's no noticeable difference between master and this branch.

Ellas-MacBook-Pro:gutenberg ella$ git checkout master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
Ellas-MacBook-Pro:gutenberg ella$ npm run test-performance

> gutenberg@6.3.0 test-performance /Users/ella/gutenberg
> wp-scripts test-e2e --config packages/e2e-tests/jest.performance.config.js

 PASS  packages/e2e-tests/specs/performance.test.js (105.638s)
  Performance
    ✓ 1000 paragraphs (101351ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        106.125s
Ran all test suites.

Average time to load: 4721ms
Average time to DOM content load: 4432ms
Average time to type character: 53.99ms
Slowest time to type character: 128ms
Fastest time to type character: 47ms
		
Ellas-MacBook-Pro:gutenberg ella$ git checkout -
Switched to branch 'try/typewriter'
Your branch is up to date with 'origin/try/typewriter'.
Ellas-MacBook-Pro:gutenberg ella$ npm run test-performance

> gutenberg@6.3.0 test-performance /Users/ella/gutenberg
> wp-scripts test-e2e --config packages/e2e-tests/jest.performance.config.js

 PASS  packages/e2e-tests/specs/performance.test.js (104.662s)
  Performance
    ✓ 1000 paragraphs (101565ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        104.739s, estimated 106s
Ran all test suites.

Average time to load: 4331ms
Average time to DOM content load: 4028ms
Average time to type character: 53.84ms
Slowest time to type character: 133ms
Fastest time to type character: 48ms
		
@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

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?

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

Thanks for the reviews everyone! Let's expose this more and iterate if there's any bugs found.

@ellatrix ellatrix merged commit d63306d into master Aug 20, 2019

4 checks passed

Filter opened
Details
Filter opened
Details
Milestone It
Details
Travis CI - Pull Request Build Passed
Details

@ellatrix ellatrix deleted the try/typewriter branch Aug 20, 2019

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

:keanu-whoah:

ƪ(˘⌣˘)┐ ƪ(˘⌣˘)ʃ ┌(˘⌣˘)ʃ

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

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.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

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.

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

@youknowriad I'm not talking about the CSS animations, but about the animation frame requested during scroll. :)

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

😅

@ellatrix ellatrix added this to the Gutenberg 6.4 milestone Aug 22, 2019

@mapk

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

Beautiful work, @ellatrix! 👏

@SchneiderSam

This comment has been minimized.

Copy link

commented Aug 26, 2019

I just tested the RC-1 and it's a really cool feature. Thanks ;-)

donmhico added a commit to donmhico/gutenberg that referenced this pull request Aug 27, 2019
Typewriter experience (WordPress#16460)
* 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
gziolo added a commit that referenced this pull request Aug 29, 2019
Typewriter experience (#16460)
* 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
gziolo added a commit that referenced this pull request Aug 29, 2019
Typewriter experience (#16460)
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.