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

ellatrix · 2018-01-31T12:14:04Z

Description

This PR fixes #2601 and part of #4731. It delays moving the focus an editable that is still initialising, which fixes the keyboard hiding on ENTER.

How Has This Been Tested?

Open Gutenberg in iOS with an on screen keyboard. Keyboard should not hide on ENTER.

Checklist:

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

jasmussen · 2018-02-01T10:35:43Z

Here's what I'm seeing in this branch: https://cloudup.com/cj4vQAOSyir

Are you seeing the same?

ellatrix · 2018-02-01T13:04:40Z

@jasmussen So another issue seems to we the keyboard overlapping the target when it first pops up... I'll try to fix.

ellatrix · 2018-02-01T15:03:14Z

It seems Gutenberg is in general suffering from a few scroll issues. When I open the inserter, the whole screen scrolls to the bottom.

ellatrix · 2018-02-01T19:17:10Z

For our records, something similar happens with just plain contenteditables: https://codepen.io/iseulde/full/qxOoGd/

ellatrix · 2018-02-01T20:27:21Z

Okay, I think this is a lot better now. We'll get the scroll container dynamically (changes based on viewport width at 600px). The scroll container must be padded so we can scroll into view when there is little content. Maybe there's a better solution here.

jasmussen · 2018-02-02T07:42:07Z

YOU DID IT! This experience is amazing. Not only did you fix the issue when creating new paragraphs, you fixed the bounce too, I didn't think that was possible.

You deserve a literal medal. These emoji don't do justice 🏅🥇🎖

jasmussen · 2018-02-02T08:12:32Z

Screen recordings. iOS looks pretty much perfect:

Android seems to have a bit much padding at the bottom, it'd be nice if you could just see the previous paragraph above.

I know it may not be polished yet, but how you solved this in two days boggles my mind.

ellatrix · 2018-02-02T10:35:18Z

The only thing that bothers me still is that sometimes, when you focus some text that will later be covered by the keyboard, it won't scroll up.

jasmussen · 2018-02-02T10:39:05Z

The only thing that bothers me still is that sometimes, when you focus some text that will later be covered by the keyboard, it won't scroll up.

On mobile Safari, the viewport doesn't change height when the soft keyboard shows up. It does on Android. I don't know how much there is we can do here.

ellatrix · 2018-02-02T12:22:32Z

The only thing that bothers me still is that sometimes, when you focus some text that will later be covered by the keyboard, it won't scroll up.

This issue is now fixed btw. :)

Android seems to have a bit much padding at the bottom, it'd be nice if you could just see the previous paragraph above.

That's because the publish toolbar hides on iOS, but it does not on Android. Not sure how to fix ATM or what the mechanism that hides it on iOS is.

youknowriad · 2018-02-02T12:33:07Z

blocks/rich-text/index.js

+					// the position can be scrolled to in the next focussed
+					// instance.
+					if ( window.innerWidth < 784 ) {
+						scrollPosition = this.editor.getBody().getBoundingClientRect();


This is probably a left over?

What do you mean with left over? :)

Nevermind, I thought it was a local variable

youknowriad · 2018-02-02T12:34:21Z

blocks/rich-text/index.js

@@ -212,6 +219,30 @@ export default class RichText extends Component {
 	}

 	onFocus() {
+		// For small screens (virtual keyboard), always scroll the focussed
+		// editor into view. Unfortunately we cannot detect virtual keyboards.
+		if ( window.innerWidth < 784 ) {


Maybe we could extract this to a module const to avoid repetition?

Yeah, that's probably best. To utils?

Hm, I just tried the iPad horizontally, and this won't cover it. We need a better way to detect virtual keyboards.

youknowriad · 2018-02-02T12:35:45Z

blocks/rich-text/index.js

@@ -884,6 +933,7 @@ export default class RichText extends Component {
 					{ ...ariaProps }
 					className={ className }
 					key={ key }
+					onMount={ this.onTinyMCEMount }


Should we move these scrolling behaviors to the TinyMCE component instead of adding this Prop?

I did, but then we need to pass the scrollPosition variable which is tied to this component. To me it looked cleaner this way, but I'm fine either way.

True, I missed the global variable

Global sounds bad :) I'd prefer to call it module variable ;)

youknowriad · 2018-02-02T12:36:21Z

edit-post/components/layout/style.scss

@@ -70,6 +70,7 @@

 .edit-post-layout__content {
 	position: relative;
+	padding-bottom: 100vh;


What's the purpose of this change?

This could use a comment as well. :) To pad the scroll box if there is not enough content to scroll.

youknowriad · 2018-02-02T12:38:51Z

editor/components/block-list/block.js

@@ -504,6 +480,7 @@ const mapStateToProps = ( state, { uid } ) => ( {
 	isFirstMultiSelected: isFirstMultiSelectedBlock( state, uid ),
 	isHovered: isBlockHovered( state, uid ) && ! isMultiSelecting( state ),
 	focus: getBlockFocus( state, uid ),
+	shouldFocusNext: shouldBlockFocusNext( state, uid ),


So if I understand properly this new prop is useful because there's a new dispatch that will be performed later to move the focus? Why can't we batch these two dispatches together?

Maybe a unique actions

Using a store enhancer or a middleware (I believe these things already exist)

Yes, we let the editor that needs focus call onFocus by itself later. There's no newly added dispatch here, we are just moving focus too early. I'm not sure how an enhancer or middleware would help.

youknowriad · 2018-02-02T12:39:52Z

Added some reviewers as this kind of change is always tricky :)

youknowriad · 2018-02-02T13:10:44Z

editor/store/reducer.js

-				focus: {},
-				isMultiSelecting: false,
+				// We must wait for Editable to initialise.
+				next: action.blocks[ 0 ].uid,


Not sure I understand here, you're adding a next saying that this block will be focused next but for me focus: {} means the same thing?

So wondering why the focus prop is not passed at the same moment to the block without this change?

The focus props for the previous block will be taken away, which will cause the area to blur.

So you want to focus before blur right? Maybe we can just delay the .blur call?

How do we do that across instances? I guess we could maybe use something like scrollPosition. scrollPosition || blur.

Actually, I wonder if this .blur called is needed at all, might be good to check the commit adding it.

The .blur() call was added here #683 (comment) where the reason seems based on "logic" than on a use case (cc @aduth). If removing the blur fixes the mobile issue, I'd vote for that :)

Calling blur might be bad in any case, because the focus should always move somewhere else. This will just move it to the body element.

ellatrix · 2018-02-02T13:58:16Z

I removed the blur call entirely, and while this seems to work on iPhone, the keyboard still hides on enter on iPad... Looking...

ellatrix · 2018-02-02T14:01:00Z

This does not happen with the last version d1966db... What is happing here... 😭

ellatrix · 2018-02-02T15:25:34Z

@youknowriad It seems that it just sometimes works, sometimes it does not, because focus is first moved to the block wrapper:

gutenberg/editor/components/block-list/block.js

Lines 125 to 128 in 56e192c

    
           componentDidMount() { 
        
           	if ( this.props.focus ) { 
        
           		this.node.focus(); 
        
           	}

With the next trick in the reducer, this focus call was skipped too. Somehow we need to remove this call if an editable is present?

ellatrix · 2018-02-02T15:27:00Z

I'm also wondering why the autofocus is needed at all. I don't immediately see regressions taking it away. Having a look at the history here.

ellatrix · 2018-02-07T09:55:27Z

Rebased.

ellatrix · 2018-02-07T10:51:45Z

Figuring out how to fix the e2e test.

ellatrix · 2018-02-07T11:22:04Z

@youknowriad It seems the e2e tests just start typing too fast after click/enter. I always see the first character splitting off from the rest. Unsure how to fix at this time. I tried different selectors and delay.

youknowriad · 2018-02-07T12:40:20Z

ad part of #4872 I noticed that one of the tests was a false positive in master and the issue was surfaced by my changes (and I suspect it surfaced here as well). And it looks like there's no way to fix it at the moment so I commented and added a comment to explain the reasons.

Maybe if you rebase the tests will pass.

mcsf

Nice work. :) Left some questions.

mcsf · 2018-02-07T16:45:27Z

blocks/rich-text/index.js

@@ -592,6 +616,11 @@ export default class RichText extends Component {
 				if ( event.shiftKey || ! this.props.onSplit ) {
 					this.editor.execCommand( 'InsertLineBreak', false, event );
 				} else {
+					// For type writing offect, save the root node offset so it
+					// can the position can be scrolled to in the next focussed
+					// instance.


Copy errors here.

mcsf · 2018-02-07T16:47:43Z

blocks/rich-text/index.js

@@ -212,6 +219,23 @@ export default class RichText extends Component {
 	}

 	onFocus() {
+		// For virtual keyboards, always scroll the focussed editor into view.
+		// Unfortunately we cannot detect virtual keyboards, so we check UA.


I don't have an external keyboard right now, but what happens if I use Gutenberg with a mobile device with an external keyboard? Most systems hide the virtual keyboard whenever an external keyboard is connected; any chance the auto-scrolling can be detrimental in those cases?

It wouldn't be super annoying I think. The focussed field won't go out of view, and in these cases, we're still dealing with a relatively small screen. The bigger the screen, the worse this gets. While not ideal, this is the best I can come up with. I've played with other approaches as well, e.g. trying to detect touch (vs clicks), but failed to make it work. The order of which touch, click, and focus events are fired makes it seems impossible to do.

mcsf · 2018-02-07T16:48:03Z

blocks/rich-text/index.js

@@ -212,6 +219,23 @@ export default class RichText extends Component {
 	}

 	onFocus() {
+		// For virtual keyboards, always scroll the focussed editor into view.
+		// Unfortunately we cannot detect virtual keyboards, so we check UA.
+		if ( /iPad|iPhone|iPod|Android/i.test( window.navigator.userAgent ) ) {


Is it worth making a isMobile util?

Maybe. Would be nice if there are more uses. In this specific case we're not really trying to detect "mobile" devices, but rather devices with virtual keyboards.

Agreed. My thinking was also something like:

isMobile = () => regex.test( … ) hasVirtualKeyboard = isMobile // best approximation

ellatrix · 2018-02-07T23:21:16Z

I'm trying to rebase, but the last focus changes are causing some bugs here. Haven't figured out why yet, I may need to revisit this tomorrow.

mcsf · 2018-02-09T10:31:37Z

I'm trying to rebase, but the last focus changes are causing some bugs here. Haven't figured out why yet, I may need to revisit this tomorrow.

Let us know when/if you'd like a last look, once that's resolved.

ellatrix · 2018-02-12T16:34:14Z

Looking at this again. Sometimes instead of scrolling down on enter, it scrolls up... I can't remember this before the rebase. Trying to debug but not sure what's going on.

ellatrix · 2018-02-12T17:20:51Z

Got a bursting headache from this issue

jorgefilipecosta · 2018-02-27T18:12:28Z

There were some changes in parallel to this PR so conflicts happened. I rebased handled the conflicts and I did a new set of tests.
I think the work done here is awesome @iseulde! It's totally new experience writing on iOS after this PR. I did not find regressions on iOS there and the problem of scrolling up on enter never happened (maybe the changes in parallel solved it).
The only regression I found is on the desktop, using arrows to scroll the editable paragraph may become out of view:

It looks like it is the only issue remaining I will try to find the reason for this behavior.

youknowriad · 2018-02-28T07:34:59Z

editor/components/block-list/block.js

+		// If there is a contenteditable element, it will move focus by itself.
+		if ( ! target.querySelector( '[contenteditable="true"]' ) ) {
+			target.focus();
+		}


This is the only change that's bothering me in this PR because it introduces a special case in how blocks focus is handled.

I'm thinking all this behavior focusTabbable when a new block is selected could be moved into a separate component and also handle the usecase we need for this PR: Focus the first tabbable element at the right position :)

Anyway, might not be that easy to do, so maybe for now, just consolidate the test of the presence of a TinyMCE editor to match the test already done here https://github.com/WordPress/gutenberg/pull/4775/files#diff-bc66d5701a2fea1111bb8f1bf0150910R244 const editor = tinymce.get( target.getAttribute( 'id' ) );

Also, I believe this assumes that the TinyMCE will focus itself which I believe is not always what we want. Imagine inserting a block containing a plainText followed by the richText. TinyMCE won't be focused in that case.

I believe this is only necessary if we use the onSplit behavior of the RichText element.

youknowriad · 2018-02-28T07:40:53Z

blocks/rich-text/index.js

+					// For type writing offect, save the root node offset so it
+					// can the position can be scrolled to in the next focussed
+					// instance.
+					offsetTop = rootNode.getBoundingClientRect().top;


While this works at the moment, nothing guarantee that it will work in the future, imagine if the onSplit callback of the RichText component inserts something else that a RichText, say a PlainText or another unrelated block, how this would work? would it be a problem?

I don't have an alternative implementation at the moment but it seems fragile to rely on a global variable like this and assume a certain behavior in parent blocks.

jasmussen · 2018-02-28T08:17:06Z

Thank you for helping with this PR. It's going to improve the user experience so much, and it feels like it's nearly there, experience wise.

notnownikki · 2018-03-07T09:44:36Z

I found a similar (but slightly different) issue to #4775 (comment)

If I create a post and keep inserting paragraphs, then the space at the end of the post keeps growing and I can scroll the post out of view, whether I use the arrows or not.

This is in Chrome 61.

notnownikki · 2018-03-07T09:58:20Z

There's the same space at the bottom of posts on android too, you can swipe up and lose the post off the top of the screen (although that's probably the same thing Joen reports near the beginning :) )

jasmussen · 2018-03-21T11:03:24Z

Ella has done such a great job on this PR, and deserves multiple medals for so drastically improving the writing behavior on phones, notably the iPhone.

It would be a great thing if we were to pull together and ship this PR as soon as we can, as the way this fixes the phone experience would be great to test in master for a bit. It's also definitely clear that this is a technically difficult PR with many different moving pieces. How can we help ship this together, so it's not only on Ella's shoulders?

Does it make sense to look at the individual pieces that make up this PR and ship them as smaller PRs separately? Is there a way we reduce this PR to a smaller, half-way step, and then pull together to ship the remaining pieces afterwards? CC: @aduth @youknowriad @gziolo @karmatosed @jorgefilipecosta

ellatrix · 2018-04-05T18:33:21Z

Done in #5769.

ellatrix force-pushed the try/fix-ios-keyboard-hide branch from 37053b2 to eb6ee66 Compare January 31, 2018 12:36

This was referenced Jan 31, 2018

Enter in iOS Safari hides keyboard #2601

Closed

Improve soft-keyboard handling on mobile #4731

Closed

ellatrix force-pushed the try/fix-ios-keyboard-hide branch 2 times, most recently from dd96f0e to 6e75cd6 Compare February 1, 2018 17:05

ellatrix requested a review from youknowriad February 2, 2018 12:27

youknowriad reviewed Feb 2, 2018

View reviewed changes

youknowriad requested review from mcsf and aduth February 2, 2018 12:39

youknowriad reviewed Feb 2, 2018

View reviewed changes

ellatrix force-pushed the try/fix-ios-keyboard-hide branch from 310ebe7 to 4bf3b8e Compare February 2, 2018 13:56

ellatrix force-pushed the try/fix-ios-keyboard-hide branch from 7a4f87c to 2d1c883 Compare February 7, 2018 09:54

ellatrix requested a review from a team February 7, 2018 09:55

mtias added the [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... label Feb 7, 2018

mcsf reviewed Feb 7, 2018

View reviewed changes

mtias added the Mobile Web Viewport sizes for mobile and tablet devices label Feb 9, 2018

ellatrix force-pushed the try/fix-ios-keyboard-hide branch from 2d1c883 to 1e62288 Compare February 12, 2018 16:32

jasmussen mentioned this pull request Feb 26, 2018

Block List: Extract scroll preservation as non-visual component #5085

Merged

Delay focus until MCE init (squashed 11 commits)

f281688

jorgefilipecosta force-pushed the try/fix-ios-keyboard-hide branch from 1e62288 to f281688 Compare February 27, 2018 17:44

youknowriad reviewed Feb 28, 2018

View reviewed changes

jasmussen mentioned this pull request Mar 13, 2018

Writing in the bottom of the screen #5580

Closed

jorgefilipecosta mentioned this pull request Mar 22, 2018

iOS - cursor hidden when editing on screen keyboard appears #3360

Closed

2 tasks

mcsf mentioned this pull request Mar 23, 2018

RichText: Keep caret visible when typing on mobile #5769

Merged

3 tasks

ellatrix closed this Apr 5, 2018

ellatrix deleted the try/fix-ios-keyboard-hide branch April 5, 2018 18:33

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

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

Conversation

ellatrix commented Jan 31, 2018 • edited Loading

Description

How Has This Been Tested?

Checklist:

jasmussen commented Feb 1, 2018

ellatrix commented Feb 1, 2018

ellatrix commented Feb 1, 2018

ellatrix commented Feb 1, 2018

ellatrix commented Feb 1, 2018

jasmussen commented Feb 2, 2018

jasmussen commented Feb 2, 2018

ellatrix commented Feb 2, 2018

jasmussen commented Feb 2, 2018

ellatrix commented Feb 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

youknowriad Feb 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

youknowriad commented Feb 2, 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

youknowriad Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Feb 2, 2018

ellatrix commented Feb 2, 2018

ellatrix commented Feb 2, 2018

ellatrix commented Feb 2, 2018

ellatrix commented Feb 7, 2018

ellatrix commented Feb 7, 2018

ellatrix commented Feb 7, 2018

youknowriad commented Feb 7, 2018

mcsf left a 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 Feb 7, 2018

mcsf commented Feb 9, 2018

ellatrix commented Feb 12, 2018

ellatrix commented Feb 12, 2018

jorgefilipecosta commented Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmussen commented Feb 28, 2018

notnownikki commented Mar 7, 2018

notnownikki commented Mar 7, 2018 • edited Loading

jasmussen commented Mar 21, 2018 • edited Loading

ellatrix commented Apr 5, 2018

ellatrix commented Jan 31, 2018 •

edited

Loading

youknowriad Feb 2, 2018 •

edited

Loading

youknowriad Feb 2, 2018 •

edited

Loading

youknowriad Feb 2, 2018 •

edited

Loading

jorgefilipecosta commented Feb 27, 2018 •

edited

Loading

notnownikki commented Mar 7, 2018 •

edited

Loading

jasmussen commented Mar 21, 2018 •

edited

Loading