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

RichText/Input Interaction: use ZWNBSP as padding #14846

Merged
merged 6 commits into from Jul 26, 2019

Conversation

@ellatrix
Copy link
Member

commented Apr 5, 2019

Description

<br> elements as padding for rich text elements proved to be no good.

Screenshot 2019-07-24 at 17 30 02

  • Using line breaks as padding can cause some subtle range-to-rectangle bugs. See #14804 (comment). The following code tries to account for that, but doesn't fully succeed in some cases.

/**
* Get the rectangle of a given Range.
*
* @param {Range} range The range.
*
* @return {DOMRect} The rectangle.
*/
export function getRectangleFromRange( range ) {
// For uncollapsed ranges, get the rectangle that bounds the contents of the
// range; this a rectangle enclosing the union of the bounding rectangles
// for all the elements in the range.
if ( ! range.collapsed ) {
return range.getBoundingClientRect();
}
const { startContainer } = range;
// Correct invalid "BR" ranges. The cannot contain any children.
if ( startContainer.nodeName === 'BR' ) {
const { parentNode } = startContainer;
const index = Array.from( parentNode.childNodes ).indexOf( startContainer );
range = document.createRange();
range.setStart( parentNode, index );
range.setEnd( parentNode, index );
}
let rect = range.getClientRects()[ 0 ];
// If the collapsed range starts (and therefore ends) at an element node,
// `getClientRects` can be empty in some browsers. This can be resolved
// by adding a temporary text node with zero-width space to the range.
//
// See: https://stackoverflow.com/a/6847328/995445
if ( ! rect ) {
const padNode = document.createTextNode( '\u200b' );
range.insertNode( padNode );
rect = range.getClientRects()[ 0 ];
padNode.parentNode.removeChild( padNode );
}
return rect;
}

If we use zero width spaces as padding, the calculation is more reliable, and Firefox doesn't have any issues.

It also fixes an issue in the list block, where to browser moves the selection from an empty text element between a br and nested list to the start of the nested list.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@ellatrix ellatrix referenced this pull request Apr 8, 2019
5 of 5 tasks complete

@ellatrix ellatrix force-pushed the try/zwnbsp-padding branch 2 times, most recently from eac10df to 8c2699b Apr 19, 2019

@ellatrix ellatrix changed the title RichText/Input Interaction: try ZWNBSP as padding RichText/Input Interaction: use ZWNBSP as padding Apr 24, 2019

@aduth
Copy link
Member

left a comment

The build failure appears to be legitimate. I can also reproduce locally that when pressing ArrowDown from a bottom-padded paragraph, the caret is not placed in the text field (though the <p contenteditable> does become the active element).

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

@aduth Yeah, I know, I haven't had the chance to look at it yet.

@ellatrix ellatrix force-pushed the try/zwnbsp-padding branch from 8c2699b to 9be2f6f May 14, 2019

@ellatrix ellatrix referenced this pull request May 14, 2019
5 of 5 tasks complete
@ellatrix

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

I can't reproduce the error locally. :/

@aduth

This comment has been minimized.

Copy link
Member

commented May 14, 2019

Testing again, I still observe issues, though in my case it works for the ArrowDown from the padded field, but the caret becomes lost when pressing ArrowUp to try to get back to the first field.

I created a screen recording in case it helps provide any useful context: https://cloudup.com/crRofrsTvMJ

@aduth

This comment has been minimized.

Copy link
Member

commented May 14, 2019

In debugging, I notice that in placeCaretAtVerticalEdge, it computes a new range using hiddenCaretRangeFromPoint, where the range object produced by this function is anchored to a button in the block contextual toolbar:

image

Unclear if this might explain the issue (it's trying to move the selection into the hidden toolbar button?). I'm assuming the caretRangeFromPoint might be misidentifying some hidden elements.

If you're having trouble reproducing, it may be that you're configured to use the Fixed toolbar?

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

@aduth I don't see a toolbar in the video you shared. Not sure how it would catch the selection...

@ellatrix ellatrix force-pushed the try/zwnbsp-padding branch from 9be2f6f to bc2dee1 Jul 24, 2019

@ellatrix ellatrix requested review from daniloercoli and etoledom as code owners Jul 24, 2019

@ellatrix ellatrix requested a review from SergioEstevao as a code owner Jul 24, 2019

@@ -407,20 +407,6 @@ export function placeCaretAtVerticalEdge( container, isReverse, rect, mayUseScro
return;
}

// Check if the closest text node is actually further away.
// If so, attempt to get the range again with the y position adjusted to get the right offset.

This comment has been minimized.

Copy link
@ellatrix

ellatrix Jul 24, 2019

Author Member

I have no idea what this code is for. Apparently I wrote it! :) Since we're moving from a br element to a text element for an empty rich text, this calculation makes should navigate contenteditable with padding fail.

Since I don't understand what this is for, and there are no failing e2e tests when removing the code, I think it's safe to remove. If we end up needing it again, we should comment it better and add e2e tests.

This comment has been minimized.

Copy link
@ellatrix

ellatrix Jul 24, 2019

Author Member

@aduth This appeared to be the culprit of the e2e test failure.

@ellatrix ellatrix added this to the Gutenberg 6.2 milestone Jul 24, 2019

@ellatrix ellatrix requested a review from aduth Jul 24, 2019

@aduth

aduth approved these changes Jul 25, 2019

@@ -3,3 +3,4 @@
*/
export const LINE_SEPARATOR = '\u2028';
export const OBJECT_REPLACEMENT_CHARACTER = '\ufffc';
export const ZWNBSP = '\ufeff';

This comment has been minimized.

Copy link
@aduth

aduth Jul 25, 2019

Member

It could help to have some documentation about what these special characters are intended to be used for.

@ellatrix ellatrix merged commit bf8e942 into master Jul 26, 2019

1 of 2 checks passed

Milestone It Milestone It
Details
Travis CI - Pull Request Build Passed
Details

@ellatrix ellatrix deleted the try/zwnbsp-padding branch Jul 26, 2019

@ellatrix

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2019

Thanks @aduth!

sbardian added a commit to sbardian/gutenberg that referenced this pull request Jul 29, 2019

RichText/Input Interaction: use ZWNBSP as padding (WordPress#14846)
* RichText: try ZWNBSP as padding

* Add e2e test

* Fix e2e test

* Add list e2e test

* Try removing some mysterious code

* Add docs for special characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.