Skip to content
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
66 changes: 59 additions & 7 deletions blocks/rich-text/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,10 @@ import 'element-closest';
/**
* WordPress dependencies
*/
import { createElement, Component, renderToString, Fragment } from '@wordpress/element';
import { keycodes, createBlobURL, isHorizontalEdge, getRectangleFromRange } from '@wordpress/utils';
import { createElement, Component, renderToString, Fragment, compose } from '@wordpress/element';
import { keycodes, createBlobURL, isHorizontalEdge, getRectangleFromRange, getScrollContainer } from '@wordpress/utils';
import { withSafeTimeout, Slot, Fill } from '@wordpress/components';
import { withSelect } from '@wordpress/data';

/**
* Internal dependencies
Expand Down Expand Up @@ -415,11 +416,11 @@ export class RichText extends Component {
* absolutely position the toolbar. It does this by finding the closest
* relative element.
*
* @param {DOMRect} position Caret range rectangle.
*
* @return {{top: number, left: number}} The desired position of the toolbar.
*/
getFocusPosition() {
const position = getRectangleFromRange( this.editor.selection.getRng() );

getFocusPosition( position ) {
// Find the parent "relative" or "absolute" positioned container
const findRelativeParent = ( node ) => {
const style = window.getComputedStyle( node );
Expand Down Expand Up @@ -529,6 +530,40 @@ export class RichText extends Component {
if ( keyCode === BACKSPACE ) {
this.onChange();
}

// `scrollToRect` is called on `nodechange`, whereas calling it on
// `keyup` *when* moving to a new RichText element results in incorrect
// scrolling. Though the following allows false positives, it results
// in much smoother scrolling.
if ( this.props.isViewportSmall && keyCode !== BACKSPACE && keyCode !== ENTER ) {
this.scrollToRect( getRectangleFromRange( this.editor.selection.getRng() ) );
}
}

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

if ( ! container ) {
return;
}

// 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 = 100;

// Avoid pointless scrolling by establishing a threshold under
// which scrolling should be skipped;
const epsilon = 10;
const delta = caretTop - graceOffset;

if ( Math.abs( delta ) > epsilon ) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

container.scrollTo(
Copy link
Member

@aduth aduth Apr 2, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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:

android emulator

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:

browser

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

container.scrollLeft,
container.scrollTop + delta,
);
}
}

/**
Expand Down Expand Up @@ -632,8 +667,17 @@ export class RichText extends Component {
return accFormats;
}, {} );

const focusPosition = this.getFocusPosition();
const rect = getRectangleFromRange( this.editor.selection.getRng() );
const focusPosition = this.getFocusPosition( rect );

this.setState( { formats, focusPosition, selectedNodeId: this.state.selectedNodeId + 1 } );

if ( this.props.isViewportSmall ) {
// Originally called on `focusin`, that hook turned out to be
// premature. On `nodechange` we can work with the finalized TinyMCE
// instance and scroll to proper position.
this.scrollToRect( rect );
}
}

updateContent() {
Expand Down Expand Up @@ -838,4 +882,12 @@ RichText.defaultProps = {
formatters: [],
};

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Buuuuuuump.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggestions cannot be applied while the pull request is closed.

😞

Copy link
Member

Choose a reason for hiding this comment

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

I approve 😄

return {
isViewportSmall: isViewportMatch( '< small' ),
};
} ),
withSafeTimeout,
] )( RichText );
2 changes: 1 addition & 1 deletion components/higher-order/with-filters/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,4 @@ function MyCustomElement() {
export default withFilters( 'MyCustomElement' )( MyCustomElement );
```

`withFilters` expects a string argument which provides a hook name. It returns a function which can then be used in composing your component. The hook name allows plugin developers to customize or completely override the component passed to this higher-order component using `wp.utils.addFilter` method.
`withFilters` expects a string argument which provides a hook name. It returns a function which can then be used in composing your component. The hook name allows plugin developers to customize or completely override the component passed to this higher-order component using `wp.hooks.addFilter` method.
13 changes: 9 additions & 4 deletions edit-post/components/layout/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,18 @@
min-height: 100%;
flex-direction: column;

// on mobile the main content area has to scroll
// otherwise you can invoke the overscroll bounce on the non-scrolling container, causing (ノಠ益ಠ)ノ彡┻━┻
// Pad the scroll box so content on the bottom can be scrolled up.
padding-bottom: 50vh;
@include break-small {
overflow-y: auto;
-webkit-overflow-scrolling: touch;
padding-bottom: 0;
}

// On mobile the main content area has to scroll otherwise you can invoke
// the overscroll bounce on the non-scrolling container, causing
// (ノಠ益ಠ)ノ彡┻━┻
overflow-y: auto;
-webkit-overflow-scrolling: touch;

.edit-post-visual-editor {
flex-basis: 100%;
}
Expand Down