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: convert HTML formatting whitespace to spaces #12166

Merged
merged 4 commits into from Nov 30, 2018

Conversation

Projects
None yet
9 participants
@iseulde
Copy link
Member

iseulde commented Nov 21, 2018

Description

Fixes #11588, #5872 and #7474.

When creating a rich text value from text nodes, this branch reduces any sequence of new lines and tabs to one space. This is needed because any sequence of new lines and tabs is used to format HTML, not as content. The browser will only display it as one space, if it is used in between words.

Previously we were just removing any line breaks, which would normally still be displayed by the browser as a space.

It also visualises and leading and trailing spaces, and and consecutive spaces elsewhere. This prevents buggy behaviour in different browsers when there is only one spaces, and prevent e.g. Chrome from inserting non breaking spaces and Firefox a trailing line break to visualise a trailing space when typing.

How has this been tested?

Edit a paragraph as HTML. Remove a space and insert a line break (use ENTER). Edit visually. You should see a space where you inserted the line break. Edit the content. Edit as HTML. The line break should be replaced by a space.

Previously the line break would have been removed.

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

This comment has been minimized.

Copy link
Contributor

designsimply commented Nov 25, 2018

I re-tested the steps at #11588 (comment) using the try/rich-text-newlines-to-space branch and found that no spaces went missing after copying content from a locally installed word processing app into Gutenberg.

I also tested the following steps:

  1. Edit a paragraph as HTML.
  2. Remove a space and insert a line break (use ENTER).
  3. Edit visually.
  4. You should see a space where you inserted the line break.
  5. Edit as HTML.
  6. The line break should be replaced by a space.

Result: after using "Edit as HTML" and replacing a space with a line break and then switching back to "Edit visually," I could see a space where I had inserted a line break. After switching back to "Edit as HTML" the line break was still there and was not replaced by a space (as described in step 7).

screen shot 2018-11-25 at 4 16 35 pm
Seen at http://localhost:8888/wp-admin/post.php?post=24&action=edit using WordPress 4.9.8 and Gutenberg 0431abae0 (try/rich-text-newlines-to-space branch) with Firefox 63.0.3 on macOS 10.13.6.

@iseulde

This comment has been minimized.

Copy link
Member

iseulde commented Nov 26, 2018

@designsimply If you switch between visual and HTML mode without editing the content, no content will be saved, so it's normal that the line break stays there if the content in unchanged.

@iseulde iseulde added this to the 4.6 milestone Nov 26, 2018

@iseulde iseulde requested a review from WordPress/gutenberg-core Nov 26, 2018

@Haldaug

This comment has been minimized.

Copy link

Haldaug commented Nov 26, 2018

This resolved the bug #11588 for me. Nice work, @iseulde.

@nosolosw

This comment has been minimized.

Copy link
Member

nosolosw commented Nov 26, 2018

This is what I've tested (both in chrome 70 and firefox 63, Linux OS). This seems ready.

#7474 is fixed

Test 1:

  • Type "Hey Jude, ". A nbsp characters no longer gets added in chrome. Chrome still shows a space, but it is non-breaking.

Test 2:

  • Type "Hey Jude, "
  • Paste "Don't make it bad."
  • A nbsp character is no longer inserted in the saved content.

Test 3:

  • Type a space " ".
  • Paste an embeddable link, such as https://www.youtube.com/watch?v=A_MjCqQoLLA
  • In the editor, the embed isn't expanded, but after saving the post, the front-end does show that the embed was created and the iframe correctly added. Previously, it rendered a nbsp and the link.

#5872 is fixed

But I could only repro this one partially. This is what I've tried:

  • Add a paragraph or heading with "hey Jude"
  • Go to Code editor mode and add a new line (using `ENTER) and then "Don't let me down"
  • Go to visual mode: new line is converted to space.
  • I save and publish the post and the front-end also shows the space.

I've also noticed that after saving the post, if I edit that paragraph in the code editor mode, I still see the line break.

#11588 is fixed

What I've tested:

  • Copy Hey Jude lyric from Libreoffice.
  • The first space between two words is no longer deleted.
@nosolosw

This comment has been minimized.

Copy link
Member

nosolosw commented Nov 26, 2018

I'm not very familiar with this part of the code, but I'd like us to spend some time adding e2e or unit tests with the steps I've reported above. They do seem like easily reproducible with a test and also easily missed in the future if we don't have them.

I can take a look at adding them tomorrow.

@iseulde

This comment has been minimized.

Copy link
Member

iseulde commented Nov 26, 2018

Last time I checked pasting didn't work in puppeteer, but maybe this changed.

@tofumatt

This comment has been minimized.

Copy link
Member

tofumatt commented Nov 26, 2018

Copy/pasting in Puppeteer still, from what I can tell, doesn't work. I think it's because it's more of an OS-level interaction and not a browser one.

Last time I tried to write an E2E test with copy/paste I went down an hour-long rabbit-hole and then ended up stopping 🤷‍♂️

// breaking spaces in between words. If also prevent Firefox from inserting
// a trailing `br` node to visualise any trailing space, causing the element
// to be saved.
white-space: pre-wrap;

This comment has been minimized.

@nosolosw

nosolosw Nov 27, 2018

Member

pre-wrap will preserve newlines, spaces, and tabs. But in filterStringComplete we're only replacing newlines and tabs with spaces. As a consequence of this change, the editor will now show those consecutive spaces.

Before this change:

peek 2018-11-27 19-21

After this change:

peek 2018-11-27 19-29

Is that a concern?

This comment has been minimized.

@nosolosw

nosolosw Nov 27, 2018

Member

For completeness, here's the same action in visual mode. No visual change, although under the hood the HTML doesn't add nbsp.

Before this change:

peek 2018-11-27 19-23

After this change:

peek 2018-11-27 19-28

I've noticed that with this change applied the leading spaces in the first line collapse before the post is reloaded.

This comment has been minimized.

@iseulde

iseulde Nov 27, 2018

Member

I think that’s fine?

This comment has been minimized.

@iseulde

iseulde Nov 27, 2018

Member

In the rare case that the user want to have those spaces visible on the front end, I think they should insert those non breaking spaces themselves explicitly.

string = string.replace( /[\r\n]/g, '' );
// Reduce any whitespace used for HTML formatting to one space
// character, because it will also be displayed as such by the browser.
string = string.replace( /[\n\r\t]+/g, ' ' );

This comment has been minimized.

@nosolosw

nosolosw Nov 27, 2018

Member

Would it make sense to replace \r and \n with no spaces as it was before, and any number of tabs with one space?

This comment has been minimized.

@iseulde

iseulde Nov 27, 2018

Member

No, one of the issues this or fixes is exactly replacing line breaks with a space.

@nosolosw
Copy link
Member

nosolosw left a comment

Now that I've familiarized myself more with the rich-text library, this looks fine code-wise. I couldn't find a way to break this with the changes proposed :)

@mtias mtias modified the milestones: 4.6, 4.7 Nov 29, 2018

@youknowriad youknowriad merged commit f6bc5cb into master Nov 30, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the try/rich-text-newlines-to-space branch Nov 30, 2018

daniloercoli added a commit that referenced this pull request Nov 30, 2018

Merge branch 'master' of https://github.com/WordPress/gutenberg into …
…rnmobile/danilo-try-to-fix-undo-redo

* 'master' of https://github.com/WordPress/gutenberg:
  Autocompleters: Consider block category (#12287)
  Only init TinyMCE once per instance (#12386)
  RichText: convert HTML formatting whitespace to spaces (#12166)
  Notices: Remove "files" block in package.json (#12438)
  Edit Post: Avoid rendering AdminNotices compatibility component (#12444)
  Correct the docs manifest (#12411)

daniloercoli added a commit that referenced this pull request Nov 30, 2018

Merge branch 'master' of https://github.com/WordPress/gutenberg into …
…HEAD

* 'master' of https://github.com/WordPress/gutenberg:
  [RNmobile] Fix problems with undo/redo on Android (#12417)
  Add registry param to withDispatch component (#11851)
  Autocompleters: Consider block category (#12287)
  Only init TinyMCE once per instance (#12386)
  RichText: convert HTML formatting whitespace to spaces (#12166)
  Notices: Remove "files" block in package.json (#12438)
  Edit Post: Avoid rendering AdminNotices compatibility component (#12444)
  Correct the docs manifest (#12411)
@mcsf

This comment has been minimized.

Copy link
Contributor

mcsf commented Dec 3, 2018

Since this touches RichText's white-space CSS property, I'll cross-reference WordPress/twentynineteen#670 in case we spot any discrepancy in Twenty Nineteen.

@iseulde

This comment has been minimized.

Copy link
Member

iseulde commented Dec 3, 2018

@mcsf It shouldn't affect the fix. In Twenty Nineteen we're replacing a font tile with another, because Hoefler Text has a weird design for a non breaking space. It is true that Chrome will now no longer insert trailing non breaking spaces, so if the fix were to be removed, typing would be fine. But other non breaking spaces would still be displayed too wide.

@mcsf

This comment has been minimized.

Copy link
Contributor

mcsf commented Dec 3, 2018

Thanks for having a look.

@peteschiebel

This comment has been minimized.

Copy link

peteschiebel commented Dec 21, 2018

This looks to have been merged into GB 4.7, which was originally intended to be part of WordPress 5.0.1, but then maybe shifted to GB 4.8/ WP 5.0.2 because of the quick 5.0.1 release...

  1. Has this made it into the core Block Editor?
  2. Is there a handy way to check if a specific GB PR has made it into the latest version of WordPress?

Thanks for the hard work!

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Jan 4, 2019

RichText: convert HTML formatting whitespace to spaces (WordPress#12166)
* convert HTML formatting whitespace to spaces

* Adjust and add e2e tests

* Visualise consecutive spaces

* Update e2e tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment