Improving edge detection in blocks #3015

ephox-mogran · 2017-10-13T06:17:30Z

Description

Added some cursor position APIs to help with edge detection

How Has This Been Tested?

I can't add any tests to test the only exported method (isDom) because it requires window.getSelection to be in the test runner.

Types of changes

Bug fix / improvement.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows has proper inline documentation.

codecov · 2017-10-13T06:17:33Z

Codecov Report

Merging #3015 into master will increase coverage by 0.01%.
The diff coverage is 21.42%.

@@            Coverage Diff             @@
##           master    #3015      +/-   ##
==========================================
+ Coverage   34.54%   34.55%   +0.01%     
==========================================
  Files         261      261              
  Lines        6710     6699      -11     
  Branches     1225     1229       +4     
==========================================
- Hits         2318     2315       -3     
+ Misses       3704     3692      -12     
- Partials      688      692       +4

Impacted Files	Coverage Δ
blocks/editable/index.js	`11.07% <0%> (+0.83%)`	⬆️
editor/utils/dom.js	`11.76% <23.07%> (-3.44%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 38d0c74...d50f6c6. Read the comment docs.

ellatrix · 2017-10-13T08:48:28Z

blocks/editable/index.js

@@ -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 ) {


Could we replace these methods entirely?

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

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?

Or alternatively exposing isLeftEdge and isRightEdge?

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

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?

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

Used object notation as recommended.

ellatrix · 2017-10-13T08:50:48Z

editor/utils/dom.js

+ * @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 ) {


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.

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

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?

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.

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

Only isEdge is exposed now.

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?

ellatrix · 2017-10-13T08:55:23Z

editor/utils/dom.js

@@ -1,3 +1,50 @@
+const NODE_TEXT_TYPE = 3;
+const NODE_ELEMENT_TYPE = 1;


In other modules, we've been using:

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

Ah, I was hoping it would be somewhere.

…al functions are no longer exported

ellatrix · 2017-10-13T11:12:26Z

Thanks for the PR, I'll test in Classic Text a bit.

ellatrix · 2017-10-13T11:19:10Z

Works well with an image in Classic Test. I'm seeing a regression though. In the demo content there's an image without a caption. I select it, then put the caret in the caption. Right arrow doesn't do anything. Looks like the node is the editor, but startOffset and endOffset stays 0.

ephox-mogran · 2017-10-13T11:22:39Z

What's the DOM structure? Also, whereabouts on the demo page?

ellatrix · 2017-10-13T11:23:36Z

<figcaption contenteditable="true" class="blocks-editable__tinymce mce-content-body" aria-label="Write caption…" id="mce_36" data-is-placeholder-visible="false"><br data-mce-bogus="1"></figcaption>

ephox-mogran · 2017-10-13T11:24:10Z

And the parent of that node? (Ignore, I now see it's content-editable itself)

ellatrix · 2017-10-13T11:25:01Z

A non editable div. Not sure why that matters? :)

ephox-mogran · 2017-10-13T11:28:27Z

Yeah, I see what's happening here. It looks like tiny is injecting a <br> tag into the figcaption, which means that its child nodes are 1. The problem is that the browser doesn't seem to like selecting past the br, and tends to make your selection before it. Therefore, it's , 0. This worked before because figcaption had no text content, so 0 was the right offset. Did it definitely work before when there was something in the caption, out of curiosity ?

ephox-mogran · 2017-10-13T11:29:11Z

So I think what we'll have to do is strip any BR tags from the end of the child nodes when counting the length. This is where the edge-case part begins.

ellatrix · 2017-10-13T11:31:23Z

Why not always return true when requesting if it's the edge, and the range is at the content editable container?

ellatrix · 2017-10-13T11:32:54Z

	if ( ! range || ! range.collapsed ) {
		return false;
	}

	if ( node.isContentEditable ) {
		return true;
	}

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

	if ( ! start && ! isAtCursorEnd( node, offset ) ) {
		return false;
	}

ellatrix · 2017-10-13T11:33:51Z

In that case we also don't need to bother checking parentNodes.

ephox-mogran · 2017-10-13T11:33:54Z

Because the content could be like this:

<div><p>First paragraph</p><br /><br /><p>Second paragraph</p></div>

with the selection being (div, 2) (i.e. just before the 2nd br). Unfortunately, you can't make generalisations like that in complex content. The browser will report the selection being at the root any time it doesn't have a text node to reference.

ellatrix · 2017-10-13T11:37:39Z

True. Then it should be this?

if ( node.isContentEditable && offset === 0 ) {
	return true;
}

ephox-mogran · 2017-10-13T11:39:43Z

You could have that shortcut, yes. But the problem at the moment is that it isn't detecting the right edge, because there's a <br> there.

ephox-mogran · 2017-10-13T11:55:28Z

So I think I have a solution for that particular case (filtering out trailing br tags and checking again), but it's going to need a lot more work as you go forward. You'll identify nodes which don't create valid cursor positions for the browser but do occupy positions in the DOM. Empty text and formatting nodes and zero width cursors are particularly painful here.

gziolo · 2018-06-10T05:18:02Z

There was no activity in this PR for over 6 months. It looks like it was abandoned. Let’s close it. We can always reopen and rebase it with master branch to align with the current state of Gutenberg.

ephox-mogran added 2 commits October 13, 2017 15:48

cursor detection functions

ddc6924

dom testing

c19d314

ephox-mogran requested review from youknowriad, aduth and ellatrix October 13, 2017 06:17

ephox-mogran mentioned this pull request Oct 13, 2017

Improving edge detection for blocks #3014

Closed

ellatrix reviewed Oct 13, 2017

View reviewed changes

ephox-mogran added 2 commits October 13, 2017 20:34

changed api of isEdge

5d49715

removing tests because we do not have window selection and the intern…

d397895

…al functions are no longer exported

ephox-mogran added 3 commits October 13, 2017 21:56

cursor is at end if it is the last node or it is the last non br node

0440939

remove image special-casing ... probably not needed for this case

b155f83

inlining functions

f6b8364

ephox-mogran mentioned this pull request Nov 16, 2017

Run emoji script correctly in editable #3520

Closed

ephox-mogran added 2 commits November 17, 2017 15:52

merged with master

6202575

removing outdated comment

d50f6c6

ellatrix mentioned this pull request Jan 8, 2018

Formatted content causes unexpected keyboard navigation behavior. #4300

Closed

gziolo added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Mar 8, 2018

gziolo closed this Jun 10, 2018

gziolo deleted the fix/cursor-end-for-elements branch June 10, 2018 05:18

snyk-bot mentioned this pull request Jun 24, 2021

[Snyk] Security upgrade node-sass from 4.14.1 to 6.0.1 Verdinjoshua26/gutenberg#13

Open

This was referenced Aug 4, 2021

[Snyk] Security upgrade node-sass from 4.14.1 to 5.0.0 Verdinjoshua26/gutenberg#15

Open

[Snyk] Security upgrade node-sass from 4.14.1 to 5.0.0 Verdinjoshua26/gutenberg#16

Open

snyk-bot mentioned this pull request Sep 1, 2021

[Snyk] Security upgrade node-sass from 4.14.1 to 5.0.0 Verdinjoshua26/gutenberg#17

Open

Verdinjoshua26 mentioned this pull request Jun 20, 2023

[Snyk] Fix for 1 vulnerabilities Verdinjoshua26/gutenberg#71

Open

This was referenced Mar 22, 2024

[Snyk] Security upgrade node-sass from 4.14.1 to 5.0.0 Verdinjoshua26/gutenberg#110

Open

[Snyk] Fix for 2 vulnerabilities Verdinjoshua26/gutenberg#111

Open

		@@ -1,3 +1,50 @@
		const NODE_TEXT_TYPE = 3;
		const NODE_ELEMENT_TYPE = 1;

Improving edge detection in blocks #3015

Improving edge detection in blocks #3015

Conversation

ephox-mogran commented Oct 13, 2017 • edited Loading

Description

How Has This Been Tested?

Types of changes

Checklist:

codecov bot commented Oct 13, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ephox-mogran Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Oct 13, 2017

ellatrix commented Oct 13, 2017

ephox-mogran commented Oct 13, 2017 • edited Loading

ellatrix commented Oct 13, 2017

ephox-mogran commented Oct 13, 2017 • edited Loading

ellatrix commented Oct 13, 2017

ephox-mogran commented Oct 13, 2017 • edited Loading

ephox-mogran commented Oct 13, 2017

ellatrix commented Oct 13, 2017

ellatrix commented Oct 13, 2017

ellatrix commented Oct 13, 2017

ephox-mogran commented Oct 13, 2017 • edited Loading

ellatrix commented Oct 13, 2017

ephox-mogran commented Oct 13, 2017 • edited Loading

ephox-mogran commented Oct 13, 2017

gziolo commented Jun 10, 2018

ephox-mogran commented Oct 13, 2017 •

edited

Loading

codecov bot commented Oct 13, 2017 •

edited

Loading

ephox-mogran Oct 13, 2017 •

edited

Loading

ellatrix Oct 13, 2017 •

edited

Loading

ephox-mogran commented Oct 13, 2017 •

edited

Loading

ephox-mogran commented Oct 13, 2017 •

edited

Loading

ephox-mogran commented Oct 13, 2017 •

edited

Loading

ephox-mogran commented Oct 13, 2017 •

edited

Loading

ephox-mogran commented Oct 13, 2017 •

edited

Loading