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
Merged

Typewriter experience #16460

merged 11 commits into from
Aug 20, 2019

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix 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 added the [Type] Enhancement A suggestion for improvement. label Jul 8, 2019
@ellatrix ellatrix requested review from jasmussen and mtias July 8, 2019 11:53
@mtias mtias added the [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... label Jul 8, 2019
@jasmussen
Copy link
Contributor

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
Copy link
Member Author

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

jasmussen 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
Copy link
Member Author

ellatrix 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
Copy link
Member Author

ellatrix 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
Copy link
Member Author

ellatrix 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
Copy link
Member Author

ellatrix commented Jul 9, 2019

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

@ellatrix
Copy link
Member Author

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

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a 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.

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

@ellatrix
Copy link
Member Author

@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

youknowriad 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
Copy link
Member Author

@youknowriad Is that in Chrome or Firefox?

@youknowriad
Copy link
Contributor

It's Chrome

@ellatrix
Copy link
Member Author

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

Copy link
Contributor

@youknowriad youknowriad left a 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.

@ellatrix
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
@ellatrix ellatrix deleted the try/typewriter branch August 20, 2019 12:16
@jasmussen
Copy link
Contributor

:keanu-whoah:

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

@ellatrix
Copy link
Member Author

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

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
Copy link
Member Author

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

@youknowriad
Copy link
Contributor

😅

@ellatrix ellatrix added this to the Gutenberg 6.4 milestone Aug 22, 2019
@mapk
Copy link
Contributor

mapk commented Aug 23, 2019

Beautiful work, @ellatrix! 👏

@SchneiderSam
Copy link

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

donmhico pushed a commit to donmhico/gutenberg that referenced this pull request Aug 27, 2019
* 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 pushed a commit that referenced this pull request Aug 29, 2019
* 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 pushed a commit that referenced this pull request Aug 29, 2019
* 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
Labels
[Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.