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.

Sign up for GitHub

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

Rich Text: Detect handled horizontal navigation by preventDefault #7644

Merged
merged 1 commit into from
Jul 2, 2018
Merged

Rich Text: Detect handled horizontal navigation by preventDefault #7644

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
83 changes: 33 additions & 50 deletions editor/components/rich-text/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,21 +212,6 @@ export class RichText extends Component {
this.editor.shortcuts.add( rawShortcut.primary( 'z' ), '', 'Undo' );
this.editor.shortcuts.add( rawShortcut.primaryShift( 'z' ), '', 'Redo' );

// Bind directly to the document, since properties assigned to TinyMCE
// events are lost when accessed during bubbled handlers. This must be
// captured _before_ TinyMCE's internal handling of arrow keys, which
// would otherwise already have moved the caret position. This is why
// it is bound to the capture phase.
//
// Note: This is ideally a temporary measure, only needed so long as
// TinyMCE inaccurately prevents default around inline boundaries.
// Ideally we rely on the defaultPrevented property to stop WritingFlow
// from transitioning.
//
// See: https://github.com/tinymce/tinymce/issues/4476
// See: WritingFlow#onKeyDown
this.editor.dom.doc.addEventListener( 'keydown', this.onHorizontalNavigationKeyDown, true );

// Remove TinyMCE Core shortcut for consistency with global editor
// shortcuts. Also clashes with Mac browsers.
this.editor.shortcuts.remove( 'meta+y', '', 'Redo' );
Expand Down Expand Up @@ -467,53 +452,46 @@ export class RichText extends Component {
}

/**
* Handles a horizontal navigation key down event to stop propagation if it
* can be inferred that it will be handled by TinyMCE (notably transitions
* out of an inline boundary node).
* Handles a horizontal navigation key down event to handle the case where
* TinyMCE attempts to preventDefault when on the outside edge of an inline
* boundary when arrowing _away_ from the boundary, not within it. Replaces
* the TinyMCE event `preventDefault` behavior with a noop, such that those
* relying on `defaultPrevented` are not misinformed about the arrow event.
*
* If TinyMCE#4476 is resolved, this handling may be removed.
*
* @see https://github.com/tinymce/tinymce/issues/4476
*
* @param {KeyboardEvent} event Keydown event.
* @param {tinymce.EditorEvent<KeyboardEvent>} event Keydown event.
*/
onHorizontalNavigationKeyDown( event ) {
const { keyCode } = event;
const isHorizontalNavigation = keyCode === LEFT || keyCode === RIGHT;
if ( ! isHorizontalNavigation ) {
return;
}

const { focusNode, focusOffset } = window.getSelection();
const { focusNode } = window.getSelection();
const { nodeType, nodeValue } = focusNode;

if ( nodeType !== Node.TEXT_NODE ) {
return;
}

const isReverse = event.keyCode === LEFT;

// Look to previous character for ZWSP if navigating in reverse.
let offset = focusOffset;
if ( isReverse ) {
offset--;
if ( nodeValue.length !== 1 || nodeValue[ 0 ] !== TINYMCE_ZWSP ) {
return;
}

// Workaround: In a new inline boundary node, the zero-width space
// wrongly lingers at the beginning of the node, rather than following
// the caret. If we are at the extent of the inline boundary, but the
// ZWSP exists at the beginning, consider as though it were to be
// handled as a transition outside the inline boundary.
//
// Since this condition could also be satisfied in the case that we're
// on the right edge of an inline boundary -- where the ZWSP exists as
// as an otherwise empty focus node -- ensure that the focus node is
// non-empty.
//
// See: https://github.com/tinymce/tinymce/issues/4472
if ( ! isReverse && focusOffset === nodeValue.length &&
nodeValue.length > 1 && nodeValue[ 0 ] === TINYMCE_ZWSP ) {
offset = 0;
}
const { keyCode } = event;

if ( nodeValue[ offset ] === TINYMCE_ZWSP ) {
event._navigationHandled = true;
// Consider to be moving away from inline boundary based on:
//
// 1. Within a text fragment consisting only of ZWSP.
// 2. If in reverse, there is no previous sibling. If forward, there is
// no next sibling (i.e. end of node).
const isReverse = keyCode === LEFT;
const edgeSibling = isReverse ? 'previousSibling' : 'nextSibling';
if ( ! focusNode[ edgeSibling ] ) {
// Note: This is not reassigning on the native event, rather the
// "fixed" TinyMCE copy, which proxies its preventDefault to the
// native event. By reassigning here, we're effectively preventing
// the proxied call on the native event, but not otherwise mutating
// the original event object.
event.preventDefault = noop;
}
}

Expand Down Expand Up @@ -555,6 +533,11 @@ export class RichText extends Component {
event.stopImmediatePropagation();
}

const isHorizontalNavigation = keyCode === LEFT || keyCode === RIGHT;
if ( isHorizontalNavigation ) {
this.onHorizontalNavigationKeyDown( event );
}

// If we click shift+Enter on inline RichTexts, we avoid creating two contenteditables
// We also split the content and call the onSplit prop if provided.
if ( keyCode === ENTER ) {
Expand Down
10 changes: 3 additions & 7 deletions editor/components/writing-flow/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,13 +188,9 @@ class WritingFlow extends Component {
onKeyDown( event ) {
const { hasMultiSelection, onMultiSelect, blocks } = this.props;

// If navigation has already been handled (e.g. TinyMCE inline
// boundaries), abort. Ideally this uses Event#defaultPrevented. This
// is currently not possible because TinyMCE will misreport an event
// default as prevented on outside edges of inline boundaries.
//
// See: https://github.com/tinymce/tinymce/issues/4476
if ( event.nativeEvent._navigationHandled ) {
// Aobrt if navigation has already been handled (e.g. TinyMCE inline
// boundaries).
if ( event.nativeEvent.defaultPrevented ) {
return;
}

Expand Down
15 changes: 14 additions & 1 deletion test/e2e/specs/writing-flow.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,20 @@ describe( 'adding blocks', () => {
await page.keyboard.up( 'Shift' );
await pressWithModifier( 'mod', 'b' );

// Arrow left from selected bold should traverse into first.
// Arrow left from selected bold should collapse to before the inline
// boundary. Arrow once more to traverse into first paragraph.
//
// See native behavior example: http://fiddle.tinymce.com/kvgaab
//
// 1. Select all of second paragraph, end to beginning
// 2. Press ArrowLeft
// 3. Type
// 4. Note that text is not bolded
//
// This is technically different than how other word processors treat
// the collapse while a bolded segment is selected, but our behavior
// is consistent with TinyMCE.
await page.keyboard.press( 'ArrowLeft' );
await page.keyboard.press( 'ArrowLeft' );
await page.keyboard.type( 'After' );

Expand Down