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 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
Copy link
Member

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 Compare September 18, 2019 10:46
@ellatrix
Copy link
Member Author

@ZebulanStanphill
Copy link
Member

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

@ellatrix
Copy link
Member Author

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

@tfrommen
Copy link
Member

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
Copy link
Member Author

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

@mtias mtias added the [Priority] High Used to indicate top priority items that need quick attention label Sep 23, 2019
@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
Copy link

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 Compare November 25, 2019 15:17
@aduth
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
Copy link
Member Author

@aduth Fixed in the last commit.

@aduth
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 Compare November 25, 2019 16:20
@ellatrix
Copy link
Member Author

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.

character === ' ' ||
character === '\r' ||
character === '\n' ||
character === '\t'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\f (form feed) is also allowed here, though rare in practice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Paste [Priority] High Used to indicate top priority items that need quick attention
Projects
None yet
8 participants