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

Improving edge detection in blocks #3015

Closed
wants to merge 9 commits into from
5 changes: 3 additions & 2 deletions blocks/editable/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import 'element-closest';
*/
import { createElement, Component, renderToString } from '@wordpress/element';
import { keycodes } from '@wordpress/utils';
import { isAtCursorStart, isAtCursorEnd } from '../../editor/utils/dom';

/**
* Internal dependencies
Expand Down Expand Up @@ -298,7 +299,7 @@ export default class Editable extends Component {

isStartOfEditor() {
const range = this.editor.selection.getRng();
if ( range.startOffset !== 0 || ! range.collapsed ) {
if ( ! isAtCursorStart( range.startContainer, range.startOffset ) || ! range.collapsed ) {
Copy link
Member

Choose a reason for hiding this comment

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

Could we replace these methods entirely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As is use isEdge( editor.getBody(), true ) for isStart and isEdge( editor.getBody(), false ) for end ? I think you're right. Nice catch :)

Copy link
Contributor Author

@ephox-mogran ephox-mogran Oct 13, 2017

Choose a reason for hiding this comment

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

In terms of avoiding the need to understand that boolean, are you OK with me exporting LEFT_EDGE = true and RIGHT_EDGE = false as constants, and then using them in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or alternatively exposing isLeftEdge and isRightEdge?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe isEdge( { container: Element, start: Boolean } )?

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'll defer to your preference :) I guess it matches the name in the range API for collapsing, which is most likely what you're basing it on. I've always found start a very strange name for a boolean, but that's just person taste.

I'm working on this now, so are you OK with the idea of exposing constants for the second argument or would you prefer passing an object like your last suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

I think an object might be better for now, since it's just adding the braces. :) But that's just my opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used object notation as recommended.

return false;
}
const start = range.startContainer;
Expand All @@ -316,7 +317,7 @@ export default class Editable extends Component {

isEndOfEditor() {
const range = this.editor.selection.getRng();
if ( range.endOffset !== range.endContainer.textContent.length || ! range.collapsed ) {
if ( ! isAtCursorEnd( range.endContainer, range.endOffset ) || ! range.collapsed ) {
return false;
}
const start = range.endContainer;
Expand Down
51 changes: 49 additions & 2 deletions editor/utils/dom.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,50 @@
const NODE_TEXT_TYPE = 3;
const NODE_ELEMENT_TYPE = 1;
Copy link
Member

Choose a reason for hiding this comment

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

In other modules, we've been using:

/**
 * Browser dependencies
 */
const { ELEMENT_NODE, TEXT_NODE } = window.Node

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I was hoping it would be somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


function getCursorStart( /* node */ ) {
// NOTE: In the future, we may want to skip some non-inhabitable positions in some nodes
return 0;
}

function getCursorEnd( node ) {
// NOTE: In the future, this might be a place to ignore special characters that browsers ignore
if ( node.nodeType === NODE_TEXT_TYPE ) {
return node.nodeValue.length;
}

// We may need exceptions for other empty tags as well (e.g. hr, br, input)
if ( node.nodeType === NODE_ELEMENT_TYPE && node.nodeName.toLowerCase() === 'img' ) {
return 1;
}

// Non text nodes have a last cursor position of their number of children
return node.childNodes.length;
}

/**
* Check whether the offset is at the start of the node
*
* @param {Element} node the DOM element
* @param {Integer} offset the offset
* @return {Boolean} whether or not the offset is at the first cursor position in node
*/
export function isAtCursorStart( node, offset ) {
const nodeStart = getCursorStart( node );
return nodeStart === offset;
}

/**
* Check whether the offset is at the end of the node
*
* @param {Element} node the DOM element
* @param {Integer} offset the offset
* @return {Boolean} whether or not the offset is at the last cursor position in node
*/
export function isAtCursorEnd( node, offset ) {
Copy link
Member

Choose a reason for hiding this comment

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

Might it make sense to only introduce one method (isAtCursorEnd) for now? In more sure if it's useful to create 4 functions right now. Additionally, if editable uses the rest of the logic in this file, we might not need to export it.

Copy link
Member

Choose a reason for hiding this comment

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

You would, for the tests, but maybe we can test isEdge as a whole instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With your above change to just use isEdge outside instead of needing to call an internal method, my preference would be to keep the methods for isAtCursorStart, but only expose isEdge. How does that sound?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The major reason is it just increases the likelihood that lots of code won't have to change if it turns out that you have to start ignoring some offsets for start (like empty text nodes). It can get fairly gnarly with text node fragmentation. Having the cursor calculations in one spot helps in the long run. But yep, if we are able to use isEdge the other place I changed, then definitely only expose that one and just test it.

Copy link
Member

@ellatrix ellatrix Oct 13, 2017

Choose a reason for hiding this comment

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

With your above change to just use isEdge outside instead of needing to call an internal method, my preference would be to keep the methods for isAtCursorStart, but only expose isEdge. How does that sound?

That's exactly what I mean. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only isEdge is exposed now.

Copy link
Member

Choose a reason for hiding this comment

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

What I also meant though: maybe we can only keep only getCursorEnd if the other one is just a stub for now, and isAtCursorEnd is just an equality check? Why create so many functions?

const nodeEnd = getCursorEnd( node );
return nodeEnd === offset;
}

/**
* Check whether the selection touches an edge of the container
*
Expand Down Expand Up @@ -34,11 +81,11 @@ export function isEdge( container, start = false ) {
return false;
}

if ( start && offset !== 0 ) {
if ( start && ! isAtCursorStart( node, offset ) ) {
return false;
}

if ( ! start && offset !== node.textContent.length ) {
if ( ! start && ! isAtCursorEnd( node, offset ) ) {
return false;
}

Expand Down
75 changes: 74 additions & 1 deletion editor/utils/test/dom.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* Internal dependencies
*/
import { isEdge, placeCaretAtEdge } from '../dom';
import { isEdge, placeCaretAtEdge, isAtCursorStart, isAtCursorEnd } from '../dom';

describe( 'DOM', () => {
let parent;
Expand Down Expand Up @@ -91,4 +91,77 @@ describe( 'DOM', () => {
expect( isEdge( input, false ) ).toBe( true );
} );
} );

describe( 'isAtCursorStart', () => {
it( 'should consider 0 to be the start of an empty text node', () => {
const node = document.createTextNode( '' );
expect( isAtCursorStart( node, 0 ) ).toBe( true );
expect( isAtCursorStart( node, 1 ) ).toBe( false );
} );

it( 'should consider 0 to be the start of a non-empty text node', () => {
const node = document.createTextNode( 'not-empty' );
expect( isAtCursorStart( node, 0 ) ).toBe( true );
expect( isAtCursorStart( node, 1 ) ).toBe( false );
} );

it( 'should consider 0 to be the start of an empty div', () => {
const node = document.createElement( 'div' );
expect( isAtCursorStart( node, 0 ) ).toBe( true );
expect( isAtCursorStart( node, 1 ) ).toBe( false );
} );

it( 'should consider 0 to be the start of an non-empty div', () => {
const node = document.createElement( 'div' );
node.innerHTML = 'text';
expect( isAtCursorStart( node, 0 ) ).toBe( true );
expect( isAtCursorStart( node, 1 ) ).toBe( false );
} );

it( 'should consider 0 to be the start of a img', () => {
const node = document.createElement( 'img' );
expect( isAtCursorStart( node, 0 ) ).toBe( true );
expect( isAtCursorStart( node, 1 ) ).toBe( false );
} );
} );

describe( 'isAtCursorEnd', () => {
it( 'should consider 0 to be the end of an empty text node', () => {
const node = document.createTextNode( '' );
expect( isAtCursorEnd( node, 0 ) ).toBe( true );
expect( isAtCursorEnd( node, 1 ) ).toBe( false );
} );

it( 'should consider "not-empty".length to be the end of a non-empty text node', () => {
const node = document.createTextNode( 'not-empty' );
expect( isAtCursorEnd( node, 'not-empty'.length ) ).toBe( true );
expect( isAtCursorEnd( node, 1 ) ).toBe( false );
} );

it( 'should consider 0 to be the end of an empty div', () => {
const node = document.createElement( 'div' );
expect( isAtCursorEnd( node, 0 ) ).toBe( true );
expect( isAtCursorEnd( node, 1 ) ).toBe( false );
} );

it( 'should consider 1 to be the end of an non-empty div with a single text node', () => {
const node = document.createElement( 'div' );
node.innerHTML = 'text';
expect( isAtCursorEnd( node, 1 ) ).toBe( true );
expect( isAtCursorEnd( node, 'text'.length ) ).toBe( false );
} );

it( 'should consider 3 to be the end of an non-empty div with a three spans', () => {
const node = document.createElement( 'div' );
node.innerHTML = '<span>one</span><span>two</span><span>three</span>';
expect( isAtCursorEnd( node, [ 'span', 'span', 'span' ].length ) ).toBe( true );
expect( isAtCursorEnd( node, 'onetwothree'.length ) ).toBe( false );
} );

it( 'should consider 1 to be the end of a img', () => {
const node = document.createElement( 'img' );
expect( isAtCursorEnd( node, 0 ) ).toBe( false );
expect( isAtCursorEnd( node, 1 ) ).toBe( true );
} );
} );
} );