Fix rich text value for nested lists by ellatrix · Pull Request #10799 · WordPress/gutenberg

ellatrix · 2018-10-19T14:20:04Z

Description

Fixes List block: list breaks when a new subitem is added using the keyboard. #10701. Adjusts the rich text value for lists to have separators for nested lists items. This is needed to be able to split nested list items.
Improves the handling of multiline values in general on split and merge by manipulating the value ourselves rather than relying on browser/TinyMCE behaviour.
Fixes transforms to other blocks when a list has nested list items.

A big part of the added lines are unit and e2e tests.

How has this been tested?

See #10701.

Screenshots

Types of changes

Bug fix.

Checklist:

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

aduth

Is there anywhere we've said that the multi line support is only intended for lists? Based on the prop name multilineTag, I would expect that no, it's not just for lists, but any tag. However, everything we build around it seems to be specifically only to support lists. Why is that?

aduth · 2018-10-19T17:04:06Z

packages/rich-text/src/to-dom.js

 	return node.parentNode.removeChild( node );
 }

+function createBogusBR( doc ) {


If 'br' is an abbreviation of "break" (unsure), correct capitalization would be createBogusBr

aduth · 2018-10-19T17:05:48Z

packages/rich-text/src/to-dom.js

+	return br;
+}
+
+function pad( node ) {


What is being padded, and with what. This isn't obvious.

packages/rich-text/src/create.js

packages/rich-text/src/to-tree.js

aduth · 2018-10-19T17:14:15Z

packages/rich-text/src/to-tree.js

+				if ( format.type === 'ol' || format.type === 'ul' ) {
+					accumulator.push( format );
+					accumulator.push( multilineFormat );
+				} else if ( character !== '\u2028' ) {


Can we give this magic string a named constant? Out of context, it means nothing to me.

test/e2e/specs/blocks/list.test.js

aduth · 2018-10-19T17:17:44Z

packages/rich-text/src/to-dom.js


+function createBogusBR( doc ) {
+	const br = doc.createElement( 'br' );
+	br.setAttribute( 'data-mce-bogus', '1' );


I'm sure it's known / been discussed before, but this is quite the implied dependency to TinyMCE.

Moved to RichText.

Is the idea of a "line padding" something which is broadly applicable to support rich text editing? Still seems a bit oriented to TinyMCE specifically, even if not directly by name in the rich-text package.

Is it something that could be contained within the createEmpty procedure?

Is the idea of a "line padding" something which is broadly applicable to support rich text editing?

Yes, it's not specific to TinyMCE. It should be part of the output to edit so you can always place the caret correctly in empty lines.

aduth · 2018-10-19T17:18:08Z

packages/rich-text/src/to-dom.js

+function pad( node ) {
+	const length = node.childNodes.length;
+
+	// Optimise for speed.


As opposed to?

mcsf

I need to do a better review of this later. It's a very domain-specific change, which makes it a bit harder to get into.

mcsf · 2018-10-19T17:11:41Z

packages/rich-text/src/create.js

 	unwrapNode,
 	filterString,
 	removeAttribute,
+	startSeparator = false,


New arg missing in JSDoc

mcsf · 2018-10-19T17:21:26Z

packages/rich-text/src/create.js


 		// Multiline value text should be separated by a double line break.
-		if ( index !== 0 ) {
+		if ( index !== 0 || startSeparator === true ) {


Perhaps shouldPrependSeparator would be a better name? should signals a boolean, and with prepend it seems clearer to me.

mcsf · 2018-10-19T17:38:28Z

packages/rich-text/src/to-dom.js

+	return br;
+}
+
+function pad( node ) {


The function is a little hard to decipher. Perhaps defining some terminology if terminology is needed (e.g. padding), and making the high-level purpose evident.

ellatrix · 2018-10-19T18:32:10Z

Is there anywhere we've said that the multi line support is only intended for lists? Based on the prop name multilineTag, I would expect that no, it's not just for lists, but any tag. However, everything we build around it seems to be specifically only to support lists. Why is that?

It's not, its for both p and li.

aduth · 2018-10-24T14:10:11Z

packages/rich-text/src/to-html-string.js

 */
-export function toHTMLString( value, multilineTag ) {
-	const tree = toTree( value, multilineTag, {
+export function toHTMLString( value, multilineTag, multilineWrapperTags ) {


toDOM( { value, multilineTag, multilineWrapperTags } ); ✅
toTree( { value, multilineTag, multilineWrapperTags } ); ✅
toHTMLString( value, multilineTag, multilineWrapperTags ); 🤔

The problem is that toHTMLString is now a public API. Fine to change it though, and make sure everything keeps working.

ellatrix · 2018-10-24T14:43:31Z

@aduth, thanks, looking at the issues.

ellatrix · 2018-10-24T15:22:40Z

@aduth It's because we're handling ENTER behaviour, but not BACKSPACE. I pushed a fix, which I'm still polishing.

youknowriad

Tested and works well for me 👍

Code-wise, it's a bit complex for me to dive in. cc @aduth since you reviewed earlier.

ellatrix · 2018-10-29T17:23:26Z

Thanks @youknowriad! I'll wait for a second review as well before merging.

aduth · 2018-10-29T17:48:01Z

packages/rich-text/src/to-html-string.js

-export function toHTMLString( value, multilineTag ) {
-	const tree = toTree( value, multilineTag, {
+export function toHTMLString( { value, multilineTag, multilineWrapperTags } ) {
+	// Check other arguments for back compact.


Are we going to deprecate?

Nit: Typo: compact -> compat (or backward compatibility since I'm not sure what we gain in abbreviating)

aduth · 2018-10-29T17:55:12Z

packages/editor/src/components/rich-text/index.js


+		if ( keyCode === DELETE || keyCode === BACKSPACE ) {
+			if ( this.multilineTag ) {
+				const record = this.createRecord();


I have no idea what any of this logic is doing. I would think we're unnecessarily handling backspaces / deletes when removing and not already at the extent of where a line separator would occur. Is that not true?

I'm not sure what you mean? Do you mean that we're calling this when we're the selection is not at the edge of a line? We need to have the value to check if we are in the first place, so I don't see a problem with this.

packages/editor/src/components/rich-text/index.js

aduth · 2018-10-29T17:57:26Z

packages/rich-text/src/remove-previous-line-separator.js

+ * the selection if none are provided.
+ *
+ * @param {Object} value         Value to modify.
+ * @param {number} startIndex    Start index.


Nit: Mis-alignment of spaces for description (two too far).

aduth · 2018-10-29T18:02:38Z

packages/rich-text/src/test/remove-previous-line-separator.js

+import { LINE_SEPARATOR } from '../special-characters';
+import { getSparseArrayLength } from './helpers';
+
+describe( 'removePreviousLineSeparator', () => {


What's the expected behavior of an uncollapsed selection? I'd expect it to be different, at least based on the behavior of backspace in a traditional textarea field.

I'll add tests for it as well. I guess it shouldn't remove the line.

aduth · 2018-10-29T18:04:41Z

packages/rich-text/src/test/remove-previous-line-separator.js

+/**
+ * Internal dependencies
+ */
+


There's no consistency to the newline placement after these blocks. While technically against the standard, most else of the application omits the newline. We should at the very least be consistent within the same file.

Yes. I thought there should be a newline in between, so I try to be consistent with that. It would be good to have a linting rule for it.

packages/rich-text/src/to-dom.js

packages/rich-text/src/to-tree.js

test/e2e/specs/blocks/list.test.js

youknowriad · 2018-10-30T09:22:45Z

Looks like the feedback is addressed and the PR still works properly, let's merge when the tests pass.

ellatrix · 2018-10-30T09:59:22Z

There's just a covecov failure, which we decided to ignore. Probably because I introduces a few files that are something like:

export function getSelectionEnd( { end } ) {
 	return end;
 }

We can add more tests separately if needed.

I think this is a big improvement for multiline values. Let's merge and iterate. I'll keep an eye on any bugs popping up.

talldan · 2018-11-01T06:31:17Z

edit - Ignore this, the issue seems to have been caused by #10723, must have still had the broken build loaded when testing this PR.

This change seems to have resulted in this bug:
#11348

Looking into it now, seems a pretty urgent one to fix.

The bug seems to be caused by unstableOnFocus not triggering in RichText - the table cells are not always being selected when they're clicked on.

ellatrix added the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label Oct 19, 2018

ellatrix added this to the 4.1 - UI freeze milestone Oct 19, 2018

ellatrix added [Type] Bug An existing feature does not function as intended [Priority] High Used to indicate top priority items that need quick attention labels Oct 19, 2018

ellatrix requested a review from a team October 19, 2018 14:37

ellatrix added the [Type] Regression Related to a regression in the latest release label Oct 19, 2018

ellatrix force-pushed the fix/nested-list branch from 0f202f7 to 85fc26d Compare October 19, 2018 15:08

aduth reviewed Oct 19, 2018

View reviewed changes

mcsf reviewed Oct 19, 2018

View reviewed changes

youknowriad modified the milestones: 4.1 - UI freeze, 4.2 Oct 19, 2018

This comment has been minimized.

Sign in to view

ellatrix force-pushed the fix/nested-list branch 3 times, most recently from e3f9229 to c08ce51 Compare October 24, 2018 08:36

ellatrix requested review from a team, aduth and mcsf October 24, 2018 09:23

ellatrix force-pushed the fix/nested-list branch 2 times, most recently from 28df82f to 381d0c7 Compare October 24, 2018 10:38

aduth reviewed Oct 24, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

ellatrix added 7 commits October 29, 2018 11:41

Add unit test for multiline manipulations

6de4b01

Change toHTMLString signature

619e97d

Add comment for e2e mousemove

b57148a

Remove list wrapper references to tag from rich text value

d165347

Adjust list transforms to handle nested items (also broken in master)

595d467

Fix after rebase

64ec102

Fix selection accumulation after ba37dca

a7116e1

ellatrix mentioned this pull request Oct 29, 2018

RichText: absorb internal list controls #10744

Merged

4 tasks

youknowriad approved these changes Oct 29, 2018

View reviewed changes

aduth reviewed Oct 29, 2018

View reviewed changes

Address feedback

d30fc17

ellatrix force-pushed the fix/nested-list branch from 6fdef17 to d30fc17 Compare October 30, 2018 09:08

Add inline comment for padding lines

c78bebe

ellatrix requested a review from a team October 30, 2018 09:13

Update deprecated uses of toHTMLString

25bed6b

youknowriad mentioned this pull request Oct 30, 2018

Fix multiselecting blocks using shift + arrow #11237

Merged

ellatrix merged commit 844f0ea into master Oct 30, 2018

ellatrix deleted the fix/nested-list branch October 30, 2018 10:15

This was referenced Oct 30, 2018

List bullet contents mostly deleted after ENTER + ENTER + BACKSPACE + ENTER #11165

Closed

Can't escape list with ENTER #11163

Closed

mtias added the [Block] List Affects the List Block label Oct 30, 2018

ellatrix mentioned this pull request Nov 8, 2018

Restore native backspace behaviour #11627

Merged

4 tasks

monzerfoda mentioned this pull request Nov 28, 2023

[Snyk] Fix for 10 vulnerabilities monzerfoda/gutenberg#18

Open

Conversation

ellatrix commented Oct 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcsf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ellatrix commented Oct 19, 2018

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ellatrix commented Oct 24, 2018

Uh oh!

ellatrix commented Oct 24, 2018

Uh oh!

youknowriad left a comment

Choose a reason for hiding this comment

Uh oh!

ellatrix commented Oct 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ellatrix commented Oct 19, 2018 •

edited

Loading

talldan commented Nov 1, 2018 •

edited

Loading