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

Paste: Remove HTML Formatting Space #17470

Merged
merged 7 commits into from Nov 25, 2019
Merged

Conversation

@ellatrix
Copy link
Member

ellatrix commented Sep 18, 2019

Description

Fixes #13497. (Alternative to #17459. This is a more general solution to work with any source and any block type, so it also fixes the following issues:)
Fixes #17302.
Fixes #14298.
Fixes #13647.

In HTML, there are a few characters you can use to format the code (indent, make it pretty etc.): \n, \t and normal spaces. We don't want our paste output to have these characters. Sequences of these characters should be removed, or replaced with a single normal space if it is a meaningful space (in other words, part of the content).

To do: write unit tests.

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

This comment has been minimized.

Copy link
Member

tfrommen commented Sep 18, 2019

Hm, I think this should work for the MS Word issue (see #17459), too.

Could we maybe add something like these two sample paragraphs to the MS Word fixture, though?

@ellatrix ellatrix force-pushed the try/paste-remove-formatting-space branch from 2b1d9df to 05bfe9b Sep 18, 2019
@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Sep 18, 2019

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Sep 18, 2019

Does this account for <pre> elements, where whitespace is expected to be considered as part of the content?

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Sep 19, 2019

@ZebulanStanphill that’s a good catch! I’ll disable it in pre elements.

@tfrommen

This comment has been minimized.

Copy link
Member

tfrommen commented Sep 19, 2019

Hm, somehow I thought this would be specific to certain destinations, like MS Word, Google Doc, whatever. But now I see that this is running always.

In that case, there are more use cases where line breaks are to be left alone, for example, the content of a <textarea />.

I understand the reasoning behind this PR, but, for now, I would really love for the other, much more focused PR, #17459, to get merged. 😶
If some other change later on accounts for what my PR is doing for MS Word only, it can easily be removed.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Sep 19, 2019

Form elements like textarea won't get pasted. I think this is the right direction to go.

@ellatrix ellatrix added this to the Gutenberg 6.6 milestone Sep 25, 2019
@youknowriad youknowriad removed this from the Gutenberg 6.6 milestone Oct 3, 2019
@ellatrix ellatrix added this to the Gutenberg 6.7 milestone Oct 4, 2019
@aduth aduth modified the milestones: Gutenberg 6.9, Gutenberg 7.0 Nov 11, 2019
@yankiara

This comment has been minimized.

Copy link

yankiara commented Nov 25, 2019

Hi,
How can we help speeding up this merge?
I'm willing to test/review/do anything to commit.
Yan

@ellatrix ellatrix force-pushed the try/paste-remove-formatting-space branch from 4a454f6 to 29d2f30 Nov 25, 2019
@aduth

This comment has been minimized.

Copy link
Member

aduth commented Nov 25, 2019

There's a test failure which appears it could be legitimate (failing test/integration/blocks-raw-handling.test.js).

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Nov 25, 2019

@aduth Fixed in the last commit.

@aduth
aduth approved these changes Nov 25, 2019
@aduth

This comment has been minimized.

Copy link
Member

aduth commented Nov 25, 2019

I'm not 100% confident that this is following the previously-mentioned specification, especially with regards to:

Any sequence of collapsible spaces and tabs immediately preceding or following a segment break is removed.

https://www.w3.org/TR/css-text-3/#white-space-phase-1

...which isn't really explicitly handled here. But it does seem to work as expected with everything I've thrown at it.

@ellatrix ellatrix force-pushed the try/paste-remove-formatting-space branch from 4c95a1d to 760a492 Nov 25, 2019
@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Nov 25, 2019

I'm not 100% confident that this is following the previously-mentioned specification, especially with regards to:

Any sequence of collapsible spaces and tabs immediately preceding or following a segment break is removed.

https://www.w3.org/TR/css-text-3/#white-space-phase-1

...which isn't really explicitly handled here. But it does seem to work as expected with everything I've thrown at it.

Yeah, this is a simplified version. We can make adjustments if it's not enough. Tabs are replaced with spaces, which are preserved or removed depending on the context.

@ellatrix ellatrix merged commit 0413162 into master Nov 25, 2019
1 of 2 checks passed
1 of 2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Canceled
Details
@ellatrix ellatrix deleted the try/paste-remove-formatting-space branch Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.