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

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 50 additions & 1 deletion blocks/rich-text/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,13 @@ import {
} from 'lodash';
import { nodeListToReact } from 'dom-react';
import 'element-closest';
import scrollIntoView from 'dom-scroll-into-view';

/**
* WordPress dependencies
*/
import { createElement, Component, renderToString } from '@wordpress/element';
import { keycodes, createBlobURL, isHorizontalEdge } from '@wordpress/utils';
import { keycodes, createBlobURL, getScrollContainer, isHorizontalEdge } from '@wordpress/utils';
import { withSafeTimeout, Slot, Fill } from '@wordpress/components';

/**
Expand All @@ -37,6 +38,11 @@ import { EVENTS } from './constants';

const { BACKSPACE, DELETE, ENTER } = keycodes;

/**
* Holds the offset of the root node, to use across instances when needed.
*/
let offsetTop;

export function createTinyMCEElement( type, props, ...children ) {
if ( props[ 'data-mce-bogus' ] === 'all' ) {
return null;
Expand Down Expand Up @@ -139,6 +145,8 @@ export class RichText extends Component {
this.onPastePreProcess = this.onPastePreProcess.bind( this );
this.onPaste = this.onPaste.bind( this );
this.onCreateUndoLevel = this.onCreateUndoLevel.bind( this );
this.onTinyMCEMount = this.onTinyMCEMount.bind( this );
this.onFocus = this.onFocus.bind( this );

this.state = {
formats: {},
Expand Down Expand Up @@ -182,6 +190,7 @@ export class RichText extends Component {
} );

editor.on( 'init', this.onInit );
editor.on( 'focusin', this.onFocus );
editor.on( 'NewBlock', this.onNewBlock );
editor.on( 'nodechange', this.onNodeChange );
editor.on( 'keydown', this.onKeyDown );
Expand Down Expand Up @@ -244,6 +253,25 @@ export 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 ) ) {
const rootNode = this.editor.getBody();
const rootRect = rootNode.getBoundingClientRect();
const caretRect = this.editor.selection.getRng().getClientRects()[ 0 ];
const offset = caretRect ? caretRect.top - rootRect.top : 0;

scrollIntoView( rootNode, getScrollContainer( rootNode ), {
// Give enough room for toolbar. Must be top.
// Unfortunately we cannot scroll to bottom as the virtual
// keyboard does not change the window size.
offsetTop: 100 - offset,
alignWithTop: true,
} );
}
}

/**
* Handles an undo event from tinyMCE.
*
Expand Down Expand Up @@ -541,6 +569,11 @@ export 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 link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy errors here.

offsetTop = rootNode.getBoundingClientRect().top;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


this.splitContent();
}
}
Expand Down Expand Up @@ -770,6 +803,21 @@ export class RichText extends Component {
this.props.onSplit( before, after, ...blocks );
}

onTinyMCEMount( node ) {
if ( ! offsetTop ) {
return;
}

// When a new instance is created, scroll the root node into the
// position of the root node that captured ENTER.
scrollIntoView( node, getScrollContainer( node ), {
offsetTop,
alignWithTop: true,
} );

offsetTop = null;
}

render() {
const {
tagName: Tagname = 'div',
Expand Down Expand Up @@ -829,6 +877,7 @@ export class RichText extends Component {
{ ...ariaProps }
className={ className }
key={ key }
onMount={ this.onTinyMCEMount }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I missed the global variable

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/>
{ isPlaceholderVisible &&
<Tagname
Expand Down
4 changes: 4 additions & 0 deletions blocks/rich-text/tinymce.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ import { diffAriaProps, pickAriaProps } from './aria';
const IS_PLACEHOLDER_VISIBLE_ATTR_NAME = 'data-is-placeholder-visible';
export default class TinyMCE extends Component {
componentDidMount() {
if ( this.props.onMount ) {
this.props.onMount( this.editorNode );
}

this.initialize();
}

Expand Down
2 changes: 2 additions & 0 deletions edit-post/components/layout/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@

.edit-post-layout__content {
position: relative;
// Pad the scroll box so content on the bottom can be scrolled up.
padding-bottom: 100vh;

// on mobile the main content area has to scroll
// otherwise you can invoke the overscroll bounce on the non-scrolling container, causing (ノಠ益ಠ)ノ彡┻━┻
Expand Down
5 changes: 4 additions & 1 deletion editor/components/block-list/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,10 @@ export class BlockListBlock extends Component {
return;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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' ) );

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// In reverse case, need to explicitly place caret position.
if ( isReverse ) {
Expand Down