RichText: Keep caret visible when typing on mobile #5769

mcsf · 2018-03-23T19:49:37Z

Description

A smaller iteration of #4775. Partly fixes #4731.

This PR is a spin-off of #4775 that focuses on keeping in sight the area into which a user is typing. The secondary goal is to leave a small footprint and reducing exceptions (e.g. per viewport width, per user agent).

A notable departure from #4775 is that this PR doesn't just cover Enter presses, but also line wraps — i.e., when enough text is typed that it is placed on a new line in the editor. This aggressive change warrants some extra testing, but, in my own testing, is a welcome addition.

How Has This Been Tested?

Make sure the experience of editing content on a desktop device is unchanged: (un)focusing blocks, typing in RichText elements (type enough characters so as to experience line wrap), pressing Enter in multiline blocks (e.g. Quote) and in non-multiline (e.g. Paragraph).
Test the mobile experience, ideally also on non-iOS devices (of which I currently have none). Try the same things as for desktop.

Screenshots (jpeg or gifs if applicable):

Types of changes

Checklist:

My code is tested.
My code follows the WordPress code style.
My code has proper inline documentation.

mcsf · 2018-03-23T21:14:57Z

blocks/rich-text/index.js

+			window.scrollTo(
+				window.pageXOffset,
+				window.pageYOffset + caretHeight - toolbarOffset
+			);


scrollIntoView was behaving quite awkwardly for me, scrolling a bit unpredictably. I'd still rather use whatever our preferred interface is, at least one that looks at scrollable containers. For the sake of having something demoable I pushed this anyway (and there's something nice about just calling window.scrollTo).

mcsf · 2018-03-23T21:23:49Z

edit-post/components/visual-editor/style.scss

@@ -28,6 +28,7 @@
 	// Collapse to minimum height of 50px, to fully occupy editor bottom pad.
 	height: 50px;
 	width: $content-width;
+	max-width: 100%;


Don't let this grow beyond the viewport, thereby avoiding the following:

Rebasing this. Seems addressed in #5945. Dropping this line. Correct me if I'm wrong.

I misunderstood Miguel's initial comment here, but yes, changing to move the fixed value from the width to max-width should be better here. I.e.

width: 100%; max-width: $content-width;

should be right.

jasmussen · 2018-03-24T08:05:29Z

Holy guacamole, thank you so much for working on this.

Holy moly guacamole IT'S WORKING IT'S WORKING

https://media.giphy.com/media/9K2nFglCAQClO/200.gif

GIF:

This cannot be merged in fast enough. SHIP IT SHIP IT!

I can't believe you two did it. Ella proved it was possible and nearly fixed it. This takes it home. While I imagine future improvements can be made, none of them feel critical at all to me, as this is working so well for me.

👏 👏 👏 👏 👏 👏 👏 👏 🔥

aduth · 2018-03-24T20:26:17Z

blocks/rich-text/index.js

+		const caretRect = this.getEditorSelectionRect();
+		const caretHeight = caretRect.y;
+		if ( caretHeight !== this.caretHeight ) {
+			const toolbarOffset = 100;


Which toolbar does this refer to? And should the RichText component have built-in knowledge of said toolbar's height? When we redesign the toolbar, who will know to update this value accordingly?

And should the RichText component have built-in knowledge of said toolbar's height?

It shouldn't. :)

Which toolbar does this refer to?

I grabbed the name from a comment in the parent PR, but I actually think of it independently of any toolbars. It's more of a "grace offset" so that we don't scroll to place the caret at the very top of the viewport: by leaving some empty space, we can incidentally accommodate toolbars, but it's more about scrolling to position the caret in a more central place, and perhaps revealing the end of the previous block to provide context.

When we redesign the toolbar, who will know to update this value accordingly?

From my previous reasoning, there's no imperative to synchronize the values. That said, it is a magic constant, and is disconnected from the toolbar. Other ways of addressing include computing the offset dynamically, e.g. 1/3 · viewportHeight.

Until further feedback, I'll rename and comment to clarify the intent.

aduth · 2018-03-24T20:27:10Z

blocks/rich-text/index.js

+
+	scrollToCaret() {
+		const caretRect = this.getEditorSelectionRect();
+		const caretHeight = caretRect.y;


Is it accurate to label this as a height value? Based on .y, I'd think it's a point offset.

Actually, what isn't accurate is to grab .y, since

[1] IE and Edge support the non-standard MSDN: ClientRect which does not define x and y properties (#)

in non-IE, .y and .top hold the same value* for caretRect, so I'll just switch the property. Thanks for asking.

*: Returns the top coordinate value of the DOMRect (has the same value as y, or y + height if height is negative.)

Edit: I'd written height when I meant top 🙈 . Updated.

aduth · 2018-03-24T20:28:56Z

blocks/rich-text/index.js

+	scrollToCaret() {
+		const caretRect = this.getEditorSelectionRect();
+		const caretHeight = caretRect.y;
+		if ( caretHeight !== this.caretHeight ) {


This compares to an instance value, meaning on its first invocation, it will always call window.scrollTo. Do we need the instance value, or can we just compare against the desired pageYOffset to see if we need to perform a scroll?

Good point. Actually, one thing we can do is:

if ( caretHeight !== this.caretHeight ) { const delta = caretTop - toolbarOffset; if ( Math.abs( delta ) > epsilon ) { window.scrollTo( window.pageXOffset, window.pageYOffset + delta ); } }

where epsilon is as an arbitrarily small number, e.g. 10.

This solves the problem of pointless scrolling more generally.

mcsf · 2018-03-26T13:04:27Z

16023be addresses Andrew's feedback and makes RichText more conservative about scrolling. d99d1ac, I find, greatly helps with the scrolling stutter experienced when pressing Enter into a new block.

aduth · 2018-03-26T12:55:09Z

blocks/rich-text/index.js

+			if ( Math.abs( delta ) > epsilon ) {
+				window.scrollTo(
+					window.pageXOffset,
+					window.pageYOffset + caretTop - graceOffset


Minor: You could reuse delta here, window.pageYOffset + delta

I was wondering why I was seeing unpredictable behavior on larger viewports. It seems specific to the fact that we assume window is the overflow area we want to scroll within. In desktop, the editor canvas is the scrollable region. Worse yet, desktop still tries to scroll (at least at shorter viewports where a scroll exists), but does so on the wrong element.

Should we use utils.dom.getScrollContainer ?

Yeah, I meant to circle back to getScrollContainer before merging, sorry it wasn't clear.

Minor: You could reuse delta here, window.pageYOffset + delta

D'oh, that was the idea.

aduth · 2018-03-26T13:12:29Z

blocks/rich-text/index.js

+	scrollToCaret() {
+		const caretRect = this.getEditorSelectionRect();
+		const caretTop = caretRect.top;
+		if ( caretTop !== this.caretTop ) {


Still not clear why we need this condition or the this.caretTop instance variable.

My original thinking was that, when typing in a line, this would be a cheaper test than computing delta from window.pageYOffset. However, now reading the documentation for window.scrollY, of which pageYOffset is an alias, there's no mention of any incurred cost of reading this property. In fact, it reads:

Use this property to check that the document hasn't already been scrolled when using relative scroll functions such as scrollBy(), scrollByLines(), or scrollByPages().

Which is enough to convince me to drop this eq check.

aduth · 2018-03-26T13:14:29Z

blocks/rich-text/index.js

@@ -558,6 +558,38 @@ export class RichText extends Component {
 		if ( keyCode === BACKSPACE ) {
 			this.onChange();
 		}
+
+		// `scrollToCaret` is called on `nodechange`, whereas calling it on
+		// `keyup` *when* moving to a new RichText element results in incorrect


Why do we assume backspace or enter would result in a new RichText element? Doesn't this depend on presence of onSplit and onMerge props?

Wondering also if this is a behavior that ought to be handled by individual instances of RichText, vs. a global handler (WritingFlow?)

Doesn't this depend on presence of onSplit and onMerge props?

It does; I've been trying out different heuristics but wanted to maintain momentum. Not sure what to converge on. In practice, I'm seeing good behaviors on paragraph, quote and list, which cover a decent set of multiline / onSplit / onMerge combinations.

WritingFlow?

On one hand, it makes architectural sense; on the other, RichText may be the best entity to track the caret in its own editor, so I'm not sure. It'd be great to have this in the next release, though, which is why I followed the approach of the parent PR.

mcsf · 2018-03-26T14:56:14Z

blocks/rich-text/index.js

+		// When scrolling, avoid positioning the caret at the very top of
+		// the viewport, providing some "air" and some textual context for
+		// the user, and avoiding toolbars.
+		const graceOffset = this.props.isViewportSmall ? 100 : 300;


I wasn't very with reading clientHeight on every key release, and also with the fact that it was blindly grabbing a third of the height when we don't know what the device actually looks like, so I went with this.

I wasn't very with reading clientHeight on every key release

Perhaps I'm just uninformed, but what's the reason to think this is not a trivial property access?

I was trying to find a reference, but maybe I dreamt the whole thing. I thought I recalled Paul Lewis from Google, who often talks about performance, bringing up certain read interfaces of the DOM that were more expensive than one would suspect. I was looking here but didn't find anything. Maybe it's totally fine.

Anyway, per my other comment about rolling this out for mobile only, do you think it's still worth having clientHeight / 3.0, or would the magic 100 be enough?

mcsf · 2018-03-26T15:01:05Z

One limitation right now is that there's no scrolling when a user presses shift+enter. This is because, though it would be straightforward to allow scrolling for enter if shift is also down, the caret computed at that instant points to the top of the RichText element and not where the caret really is.

I'd rather look at that later. The primary purpose of this PR was to improve the mobile experience, where by default no external keyboards are used and thus where shift + enter is not an expected input.

jasmussen · 2018-03-26T15:08:01Z

I'd rather look at that later. The primary purpose of this PR was to improve the mobile experience, where by default no external keyboards are used and thus where shift + enter is not an expected input.

This is super key, and thanks again for working on this.

aduth · 2018-03-26T17:32:53Z

blocks/rich-text/index.js

+		// When scrolling, avoid positioning the caret at the very top of
+		// the viewport, providing some "air" and some textual context for
+		// the user, and avoiding toolbars.
+		const graceOffset = this.props.isViewportSmall ? 100 : 300;


I wasn't very with reading clientHeight on every key release

Perhaps I'm just uninformed, but what's the reason to think this is not a trivial property access?

aduth · 2018-03-26T17:34:58Z

edit-post/components/layout/style.scss

-		-webkit-overflow-scrolling: touch;
-	}
+	// Pad the scroll box so content on the bottom can be scrolled up.
+	padding-bottom: 50vh;


This padding causes an unexpected shift of collapsed meta boxes in desktop:

D'oh, thanks. I tried placing the padding in other places, but they end up affecting mobile scrolling (though not this PR's), presumably due to the addition of a scrolling container. Since I'd like to retain this feature for mobile only and I'm short of ideas, I've pushed f1a4108.

aduth · 2018-03-26T17:36:50Z

blocks/rich-text/index.js

+		const epsilon = 10;
+		const delta = caretTop - graceOffset;
+
+		if ( Math.abs( delta ) > epsilon ) {


Should we check to see whether the desired location is already within viewport, even if it's not necessarily at the "top"? It's odd when clicking around on blocks in desktop that the viewport jumps constantly, even when the blocks are already visible in screen.

Yesterday I got lost in a rabbit hole with this, discovering strange edge cases, issues on desktop. I was initially happy as I thought I'd nicely add this to all devices, but now I'm thinking we should retain the is-mobile-device filter from the parent PR...

Even if it may be fine to have also on desktop, it would be nice to wrap an interim version of this PR because the improvements it means for mobile are so huge. If it means disabling it for desktop and potentially revisiting for desktop in the future, that's okay I feel.

mcsf · 2018-04-02T12:49:32Z

Let's get this in and let folks test ahead of release. What do you say, @aduth?

aduth · 2018-04-02T14:33:57Z

blocks/rich-text/index.js

+		const delta = caretTop - graceOffset;
+
+		if ( Math.abs( delta ) > epsilon ) {
+			container.scrollTo(


I'm finding this is not very durable, or at least I'm finding some browser inconsistencies. The container here will be document.body. If you open your browser's Developer Tools console right now (on GitHub.com) and enter:

document.body.scrollTo( 0, 500 );

I expect nothing will happen.

... That is, of course, unless you're running Safari or, more precisely, iOS Safari (though desktop works just as well), where it will scroll.

I'm not familiar with why this is different, though I suspect that when there is no explicit scroll container, Chrome and Firefox expect scrolls to be made via window.scrollTo (which does scroll).

Though the fact that getScrollContainer is returning document.body could itself be concerning since, as mentioned, it's not an explicit scroll container. It does qualify for document.body.scrollHeight > document.body.clientHeight. Maybe it's because there's an overflow-x: hidden ? I've had some previous experience with issues where even one direction of overflow has effects in both directions.

If we expect to call scrollTo on the result of getScrollableContainer, then I think we ought to normalize so that if the browser must call window.scrollTo on a non-explicit container, then getScrollableContainer should return window. This means either improving the detection of a scrollable container, or returning window when the result would otherwise be document.body.

I'm finding this is not very durable […] That is, of course, unless you're running Safari or, more precisely, iOS Safari (though desktop works just as well), where it will scroll.

Thanks for digging. The fact is I haven't tested this on Android yet, as I currently don't own any such device.

If we expect to call scrollTo on the result of getScrollableContainer, […]

All that sounds good, I'll see how much we can get out of the body->window normalization.

Apologies for not testing this sooner — I only tested on iPhone and in the browser and it worked fine there.

I retested, and in the Android emulator it's working pretty damn well for me:

I also tested using the Chrome "mobile simulator" (which is just desktop chrome with a small viewport and fancy mobile scrollbars, so it's not accurate), but I got a few JS errors there:

Let me know how I can help with any other testing.

ellatrix · 2018-04-05T13:18:11Z

blocks/rich-text/index.js

+			return;
+		}
+
+		const { top: caretTop } = this.getEditorSelectionRect();


I think getFocusPosition is now equivalent to getEditorSelectionRect? So we could pass that in nodeChange?

I'm a bit tied up today, but if you're interested and have spare time, I'd be grateful if you had a go. :)

ellatrix · 2018-04-05T13:39:00Z

blocks/rich-text/index.js

+		// scrolling. Though the following allows false positives, it results
+		// in much smoother scrolling.
+		if ( keyCode !== BACKSPACE && keyCode !== ENTER ) {
+			this.scrollToCaret();


Do we really want to run this stuff on every keystroke, down the line: https://gist.github.com/paulirish/5d52fb081b3570c81e3a#range.

Or maybe this is fine if there are no layout changes.

That's the list I was looking for the other week!

Yeah but the layout is not invalidated... so this seems fine. I don't see any issues atm.

ellatrix · 2018-04-05T13:47:56Z

blocks/rich-text/index.js

+
+		const { top: caretTop } = this.getEditorSelectionRect();
+
+		const container = getScrollContainer( this.editor.getBody() );


Could this be cached?

I'd say yes, but what would invalidate it? Resizing? Viewport breakpoints? Or do we assume the container to be constant throughout RichText's lifetime?

Oh, right, I somehow thought it would remain constant.

* Change graceOffset to a third of the viewport height, to see how it feels.

ellatrix · 2018-04-05T15:42:44Z

Rebased and addressed #5769 (comment).

While testing on iOS Safari I run into some strange issues... When I focus the text all text sometimes goes blank... Can't reproduce consistently and probably unrelated to this but just in case someone else experiences this...

jasmussen · 2018-04-05T16:58:20Z

Gave it a spin and my previous issues are gone.

Wasn't able to easily test in the simulator, but assuming that hasn't regressed since I tested earlier today, this is 👍 👍 to ship.

The chief reason to ship this is that it brings huge improvements to iOS which frankly is in a somewhat sorry state right now. Even if it turns out that there are some aspects of this PR that aren't perfect, it might still be worth shipping this and fixing separately.

THANK YOU for working on this, Ella and Miguel. It's sooo much better than before.

jasmussen · 2018-04-05T17:02:01Z

🔥 🔥 🔥 🔥 🔥 🔥

WOOOOOOOOOOO

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

(ノ^v^)ノ彡┻━┻

aduth · 2018-07-10T18:24:10Z

blocks/rich-text/index.js

-export default withSafeTimeout( RichText );
+export default compose( [
+	withSelect( ( select ) => {
+		const { isViewportMatch = identity } = select( 'core/viewport' ) || {};


What are the fallbacks here for? Why = identity ? Why || {} ?

What are the fallbacks here for? Why = identity ? Why || {} ?

Buuuuuuump.

I can't think of a good reason. We should remove that. Trying the new Suggestion feature!

Suggested change

const { isViewportMatch = identity } = select( 'core/viewport' ) || {};

const { isViewportMatch } = select( 'core/viewport' );

Suggestions cannot be applied while the pull request is closed.

😞

I approve 😄

mcsf added Mobile Web Viewport sizes for mobile and tablet devices [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... labels Mar 23, 2018

mcsf requested review from jasmussen, aduth and ellatrix March 23, 2018 19:49

mcsf commented Mar 23, 2018

View reviewed changes

aduth reviewed Mar 24, 2018

View reviewed changes

aduth mentioned this pull request Mar 26, 2018

Generalise and comment on DOMRect calculation and storage #5411

Merged

3 tasks

aduth reviewed Mar 26, 2018

View reviewed changes

mcsf commented Mar 26, 2018

View reviewed changes

aduth requested changes Mar 26, 2018

View reviewed changes

mcsf force-pushed the try/fix-ios-caret-visibility branch from 9b258f7 to 411d187 Compare March 27, 2018 13:55

jasmussen mentioned this pull request Apr 2, 2018

Typewriter linebreak behavior #353

Closed

aduth reviewed Apr 2, 2018

View reviewed changes

mcsf added this to the 2.6 milestone Apr 5, 2018

ellatrix reviewed Apr 5, 2018

View reviewed changes

mcsf added 6 commits April 5, 2018 17:15

RichText: Keep caret visible when typing on mobile

31b28b8

Avoid pointless scrolling

ccfaac0

Fix scroll stutter when pressing Enter

11876e6

Scroll within the right container

174900b

* Change graceOffset to a third of the viewport height, to see how it feels.

Pick graceOffset from 2 per-viewport values

c2d5abd

Only scroll on narrow viewports

433d556

mcsf and others added 2 commits April 5, 2018 17:18

Don't add bottom padding on desktop

ff5bdf6

Reuse getRectangleFromRange value

cf1729b

ellatrix force-pushed the try/fix-ios-caret-visibility branch from f1a4108 to cf1729b Compare April 5, 2018 15:40

youknowriad merged commit 8361a9c into master Apr 5, 2018

youknowriad deleted the try/fix-ios-caret-visibility branch April 5, 2018 17:01

This was referenced Apr 5, 2018

Warning on pressing ENTER fast #6021

Closed

Keep mobile keyboard on ENTER: delay focus until MCE init #4775

Closed

ellatrix mentioned this pull request Apr 18, 2018

Enter in iOS Safari hides keyboard #2601

Closed

mcsf mentioned this pull request Jul 3, 2018

RichText: Remove focusPosition state #7651

Merged

aduth reviewed Jul 10, 2018

View reviewed changes

mcsf mentioned this pull request Oct 17, 2018

Clean-up: rm unused variable fallbacks in RichText #10688

Merged

4 tasks

ellatrix mentioned this pull request Nov 16, 2018

RichText: remove iOS scroll adjusting #11971

Merged

4 tasks


		const { top: caretTop } = this.getEditorSelectionRect();

		const container = getScrollContainer( this.editor.getBody() );

	const { isViewportMatch = identity } = select( 'core/viewport' ) \|\| {};
	const { isViewportMatch } = select( 'core/viewport' );

RichText: Keep caret visible when typing on mobile #5769

RichText: Keep caret visible when typing on mobile #5769

Conversation

mcsf commented Mar 23, 2018

Description

How Has This Been Tested?

Screenshots (jpeg or gifs if applicable):

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmussen commented Mar 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcsf Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcsf commented Mar 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcsf commented Mar 26, 2018

jasmussen commented Mar 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcsf commented Apr 2, 2018

aduth Apr 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Apr 5, 2018

jasmussen commented Apr 5, 2018

jasmussen commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcsf Mar 26, 2018 •

edited

Loading

aduth Apr 2, 2018 •

edited

Loading