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
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
6 changes: 6 additions & 0 deletions packages/block-editor/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,12 @@ _Returns_

- `Array`: converted rules.

<a name="Typewriter" href="#Typewriter">#</a> **Typewriter**

Ensures that the text selection keeps the same vertical distance from the
ellatrix marked this conversation as resolved.
Show resolved Hide resolved
viewport during keyboard events within this component. The vertical distance
can vary. It is the last clicked or scrolled to position.

<a name="URLInput" href="#URLInput">#</a> **URLInput**

_Related_
Expand Down
6 changes: 5 additions & 1 deletion packages/block-editor/src/components/block-list/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ export default compose( [
getMultiSelectedBlockClientIds,
hasMultiSelection,
getGlobalBlockCount,
isTyping,
} = select( 'core/block-editor' );

const { rootClientId } = ownProps;
Expand All @@ -276,7 +277,10 @@ export default compose( [
selectedBlockClientId: getSelectedBlockClientId(),
multiSelectedBlockClientIds: getMultiSelectedBlockClientIds(),
hasMultiSelection: hasMultiSelection(),
enableAnimation: getGlobalBlockCount() <= BLOCK_ANIMATION_THRESHOLD,
enableAnimation: (
! isTyping() &&
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally like seeing the animation while hitten "Enter" or using the slash inserter. Let's have more opinions here. @jasmussen @kjellr @mapk @mtias

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 personally don't like it on Enter :D It really distracts to writing and you kind of expect it to be instant.

getGlobalBlockCount() <= BLOCK_ANIMATION_THRESHOLD
),
};
} ),
withDispatch( ( dispatch ) => {
Expand Down
1 change: 1 addition & 0 deletions packages/block-editor/src/components/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export { default as NavigableToolbar } from './navigable-toolbar';
export { default as ObserveTyping } from './observe-typing';
export { default as PreserveScrollInReorder } from './preserve-scroll-in-reorder';
export { default as SkipToSelectedBlock } from './skip-to-selected-block';
export { default as Typewriter } from './typewriter';
export { default as Warning } from './warning';
export { default as WritingFlow } from './writing-flow';

Expand Down
254 changes: 254 additions & 0 deletions packages/block-editor/src/components/typewriter/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,254 @@
/**
* WordPress dependencies
*/
import { Component, createRef } from '@wordpress/element';
import { computeCaretRect, getScrollContainer } from '@wordpress/dom';
import { withSelect } from '@wordpress/data';
import { UP, DOWN, LEFT, RIGHT } from '@wordpress/keycodes';

const isIE = window.navigator.userAgent.indexOf( 'Trident' ) !== -1;
const arrowKeyCodes = new Set( [ UP, DOWN, LEFT, RIGHT ] );
const initialTriggerPercentage = 0.75;

class Typewriter extends Component {
Copy link
Member

Choose a reason for hiding this comment

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

As a new component, I might have been curious to see if it could have been implemented as a function component using hooks (could simplify especially event binding with useEffect and handling with useCallback, cancellations on unmount, etc).

https://reactjs.org/docs/hooks-reference.html#cleaning-up-an-effect

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'm not sure how you'd convert the properties that I'm setting. E.g. this.caretRect. Using state here adds extra rerenders.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this something blocking for this feature? I ask because it feels as a great UX improvement and it could always be refactored later in a separare issue/PR, right?

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 don't think this is blocking, but something nice to have. I'm just unsure how to do it properly. Might need some help there :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Properties are easily replaced with useRef

Copy link
Member Author

Choose a reason for hiding this comment

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

How would that work?

Copy link
Contributor

Choose a reason for hiding this comment

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

const  myProperty = useRef( 'initial value' );

// Using the value
console.log( myProperty.current );

// Assigning a new value
myProperty.current =  'some value';

Copy link
Member Author

Choose a reason for hiding this comment

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

🤯 Didn't realise this could, or is designed to, be used for state as well.

constructor() {
super( ...arguments );

this.ref = createRef();
this.onKeyDown = this.onKeyDown.bind( this );
this.addSelectionChangeListener = this.addSelectionChangeListener.bind( this );
this.computeCaretRectOnSelectionChange = this.computeCaretRectOnSelectionChange.bind( this );
this.maintainCaretPosition = this.maintainCaretPosition.bind( this );
this.computeCaretRect = this.computeCaretRect.bind( this );
this.onScrollResize = this.onScrollResize.bind( this );
this.isSelectionEligibleForScroll = this.isSelectionEligibleForScroll.bind( this );
}

componentDidMount() {
// When the user scrolls or resizes, the scroll position should be
// reset.
window.addEventListener( 'scroll', this.onScrollResize, true );
window.addEventListener( 'resize', this.onScrollResize, true );
}

componentWillUnmount() {
window.removeEventListener( 'scroll', this.onScrollResize, true );
window.removeEventListener( 'resize', this.onScrollResize, true );
document.removeEventListener( 'selectionchange', this.computeCaretRectOnSelectionChange );

if ( this.onScrollResize.rafId ) {
window.cancelAnimationFrame( this.onScrollResize.rafId );
}

if ( this.onKeyDown.rafId ) {
window.cancelAnimationFrame( this.onKeyDown.rafId );
}
}

/**
* Resets the scroll position to be maintained.
*/
computeCaretRect() {
if ( this.isSelectionEligibleForScroll() ) {
this.caretRect = computeCaretRect();
}
}

/**
* Resets the scroll position to be maintained during a `selectionchange`
* event. Also removes the listener, so it acts as a one-time listener.
*/
computeCaretRectOnSelectionChange() {
document.removeEventListener( 'selectionchange', this.computeCaretRectOnSelectionChange );
this.computeCaretRect();
}

onScrollResize() {
if ( this.onScrollResize.rafId ) {
return;
}

this.onScrollResize.rafId = window.requestAnimationFrame( () => {
this.computeCaretRect();
delete this.onScrollResize.rafId;
} );
}

/**
* Checks if the current situation is elegible for scroll:
* - There should be one and only one block selected.
* - The component must contain the selection.
* - The active element must be contenteditable.
*/
isSelectionEligibleForScroll() {
return (
this.props.selectedBlockClientId &&
this.ref.current.contains( document.activeElement ) &&
document.activeElement.isContentEditable
);
}

isLastEditableNode() {
const editableNodes = this.ref.current.querySelectorAll(
'[contenteditable="true"]'
);
const lastEditableNode = editableNodes[ editableNodes.length - 1 ];
return lastEditableNode === document.activeElement;
}

/**
* Maintains the scroll position after a selection change caused by a
* keyboard event.
*
* @param {SyntheticEvent} event Synthetic keyboard event.
*/
maintainCaretPosition( { keyCode } ) {
if ( ! this.isSelectionEligibleForScroll() ) {
return;
}

const currentCaretRect = computeCaretRect();

if ( ! currentCaretRect ) {
return;
}

// If for some reason there is no position set to be scrolled to, let
// this be the position to be scrolled to in the future.
if ( ! this.caretRect ) {
this.caretRect = currentCaretRect;
return;
}

// Even though enabling the typewriter effect for arrow keys results in
// a pleasant experience, it may not be the case for everyone, so, for
// now, let's disable it.
if ( arrowKeyCodes.has( keyCode ) ) {
// Reset the caret position to maintain.
this.caretRect = currentCaretRect;
return;
}

const diff = currentCaretRect.y - this.caretRect.y;

if ( diff === 0 ) {
return;
}

const scrollContainer = getScrollContainer( this.ref.current );

// The page must be scrollable.
if ( ! scrollContainer ) {
return;
}

const windowScroll = scrollContainer === document.body;
const scrollY = windowScroll ?
window.scrollY :
scrollContainer.scrollTop;
const scrollContainerY = windowScroll ?
0 :
scrollContainer.getBoundingClientRect().y;
const relativeScrollPosition = windowScroll ?
this.caretRect.y / window.innerHeight :
( this.caretRect.y - scrollContainerY ) /
( window.innerHeight - scrollContainerY );

// If the scroll position is at the start, the active editable element
// is the last one, and the caret is positioned within the initial
// trigger percentage of the page, do not scroll the page.
// The typewriter effect should not kick in until an empty page has been
// filled with the initial trigger percentage or the user scrolls
// intentionally down.
if (
scrollY === 0 &&
relativeScrollPosition < initialTriggerPercentage &&
this.isLastEditableNode()
) {
// Reset the caret position to maintain.
this.caretRect = currentCaretRect;
return;
}

const scrollContainerHeight = windowScroll ?
window.innerHeight :
scrollContainer.clientHeight;

// Abort if the target scroll position would scroll the caret out of
// view.
if (
// The caret is under the lower fold.
this.caretRect.y + this.caretRect.height >
scrollContainerY + scrollContainerHeight ||
// The caret is above the upper fold.
this.caretRect.y < scrollContainerY
) {
// Reset the caret position to maintain.
this.caretRect = currentCaretRect;
return;
}

if ( windowScroll ) {
window.scrollBy( 0, diff );
} else {
scrollContainer.scrollTop += diff;
}
}

/**
* Adds a `selectionchange` listener to reset the scroll position to be
* maintained.
*/
addSelectionChangeListener() {
document.addEventListener( 'selectionchange', this.computeCaretRectOnSelectionChange );
}

onKeyDown( event ) {
event.persist();

// Ensure the any remaining request is cancelled.
if ( this.onKeyDown.rafId ) {
window.cancelAnimationFrame( this.onKeyDown.rafId );
}

// Use an animation frame for a smooth result.
this.onKeyDown.rafId = window.requestAnimationFrame( () => {
this.maintainCaretPosition( event );
delete this.onKeyDown.rafId;
} );
}

render() {
// There are some issues with Internet Explorer, which are probably not
// worth spending time on. Let's disable it.
if ( isIE ) {
return this.props.children;
}

// Disable reason: Wrapper itself is non-interactive, but must capture
// bubbling events from children to determine focus transition intents.
/* eslint-disable jsx-a11y/no-static-element-interactions */
return (
<div
ref={ this.ref }
onKeyDown={ this.onKeyDown }
onKeyUp={ this.maintainCaretPosition }
onMouseDown={ this.addSelectionChangeListener }
onTouchStart={ this.addSelectionChangeListener }
>
{ this.props.children }
</div>
);
/* eslint-enable jsx-a11y/no-static-element-interactions */
}
}

/**
* Ensures that the text selection keeps the same vertical distance from the
* viewport during keyboard events within this component. The vertical distance
* can vary. It is the last clicked or scrolled to position.
*/
export default withSelect( ( select ) => {
const { getSelectedBlockClientId } = select( 'core/block-editor' );
return { selectedBlockClientId: getSelectedBlockClientId() };
} )( Typewriter );
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/writing-flow/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,7 @@ class WritingFlow extends Component {
aria-hidden
tabIndex={ -1 }
onClick={ this.focusLastTextField }
className="wp-block editor-writing-flow__click-redirect block-editor-writing-flow__click-redirect"
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for this change?

I seem to recall some need for this to imitate BlockListBlock:

// The wp-block className is important for editor styles.
// Generate the wrapper class names handling the different states of the block.
const wrapperClassName = classnames(
'wp-block editor-block-list__block block-editor-block-list__block',

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if it makes much difference. Without the class, clicks anywhere under the block list will redirect. With the class, only clicks vertically under it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't the old behaviour make more sense?

className="editor-writing-flow__click-redirect block-editor-writing-flow__click-redirect"
/>
</div>
);
Expand Down
2 changes: 0 additions & 2 deletions packages/block-editor/src/components/writing-flow/style.scss
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
.block-editor-writing-flow {
height: 100%;
display: flex;
flex-direction: column;
}

.block-editor-writing-flow__click-redirect {
flex-basis: 100%;
cursor: text;
}
Loading