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.

Sign up for GitHub

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

Jump to bottom

Only disable wpautop on the main TinyMCE instance for the page #8229

Merged
merged 6 commits into from
Jul 30, 2018
Merged

Only disable wpautop on the main TinyMCE instance for the page #8229

merged 6 commits into from
Jul 30, 2018

Conversation

danielbachhuber
Copy link
Member

@danielbachhuber danielbachhuber commented Jul 26, 2018 •
edited
Loading

Description

content is a TinyMCE editor id we can use to identify this main TinyMCE instance. We don't want to disable wpautop on all editor instances on the page, because plugins could be
dependent on it.

From #2708
Related #4672 (comment)

How has this been tested?

  1. Create a new post in the Classic Editor and add the following post content in Text Mode:
<!-- wp:paragraph -->
<p>Foo bar four five six</p>
<!-- /wp:paragraph -->

This is another set of text
  1. Save the post while in Text mode.
  2. Switch to Visual Mode, and then back to Text Mode.

tinymcewpautop

  1. Observe that the content now appears like this:
<!-- wp:paragraph -->
<p>Foo bar four five six</p>
<!-- /wp:paragraph -->
<p>This is another set of text</p>

This expected output reflects the expected behavior when wpautop is disabled. When TinyMCE wpautop is disabled, TinyMCE forces a root node (<p>) on the individual line of text. If wpautop were to be enabled, then removep would be run on the switch back to Text Mode and both instances of <p> tags would be removed.

See #4672 (comment) for more background.

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.

@danielbachhuber danielbachhuber added [Type] Bug An existing feature does not function as intended Backwards Compatibility Issues or PRs that impact backwards compatability labels Jul 26, 2018
@danielbachhuber danielbachhuber added this to the 3.4 milestone Jul 26, 2018
@danielbachhuber
Copy link
Member Author

Adding e2e tests

danielbachhuber
danielbachhuber commented Jul 26, 2018
View reviewed changes
test/e2e/specs/classic-editor.test.js Outdated
await page.keyboard.type( original );

// Save the post so that TinyMCE loads with wpautop disabled.
await page.click( '#save-post' );
Copy link
Member Author

Choose a reason for hiding this comment

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

@tofumatt Could I get your help debugging this e2e test failure? My intent is to save the Classic Editor post, and then inspect TinyMCE behavior after the post is reloaded. However, it seems the test is progressing before the post is reloaded.

Copy link
Member

Choose a reason for hiding this comment

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

Sure thing, I should be able to have a look in maybe... thirty-ish minutes 😄

danielbachhuber
danielbachhuber commented Jul 26, 2018
View reviewed changes
test/e2e/specs/classic-editor.test.js Outdated
await page.type( '#content', original );

const initialTextEditorContent = await page.$eval( '.wp-editor-area', ( element ) => element.value );
expect( initialTextEditorContent ).toEqual( original );
Copy link
Member Author

Choose a reason for hiding this comment

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

@tofumatt I made a little bit of progress with f93f875d4

At this point, the assertion is failing with:

image

Copy link
Member Author

Choose a reason for hiding this comment

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

I also tried:

await page.type( '#content', '<!-- wp:paragraph -->' );
await page.keyboard.press( 'Enter' );
await page.type( '#content', '<p>Foo bar four five six</p>' );
await page.keyboard.press( 'Enter' );
await page.type( '#content', '<!-- /wp:paragraph -->' );
await page.keyboard.press( 'Enter' );
await page.keyboard.press( 'Enter' );
await page.type( '#content', 'This is another set of text' );

But it fails with the same error.

@danielbachhuber danielbachhuber force-pushed the 2708-only-main-editor branch from 578b479 to 5ae19e1 Compare July 26, 2018 16:20
@danielbachhuber
Copy link
Member Author

I've removed the e2e tests for now with bc03138

#8213 will break / complicate the e2e test anyway, so I think there's more valuable things to spend our time on right now.

@danielbachhuber danielbachhuber requested a review from a team July 26, 2018 17:04
@tofumatt
Copy link
Member

I figured this out!

Ran PUPPETEER_HEADLESS=false PUPPETEER_SLOWMO=50 npm run test-e2e:watch classic and saw that the page wasn't changing because of how vistAdmin works–it is using Puppeteer's page.goto under-the-hood and not reloading if the URL is the same as before!

So the content in the editor stays the same between tests. We should probably change vistAdmin to refresh the page.

Should have a fix in a second. I think it's useful to have this test and to have to change it when the modal merges.

@danielbachhuber
Copy link
Member Author

Should have a fix in a second. I think it's useful to have this test and to have to change it when the modal merges.

Cool, sounds good.

@tofumatt
Copy link
Member

tofumatt commented Jul 26, 2018 •
edited
Loading

As mentioned in Slack I was way too optimistic. I thought it was just the lack of navigation causing issues but something is super weird and I don't think it's your tests. I dunno, they are a good spec for what should happen so I wonder about keeping them but skipping them and noting that it seems to be the test suite that's causing issues. Anyway, I don't think I can get them fixed for now, sorry. 😢

@danielbachhuber
Copy link
Member Author

Anyway, I don't think I can get them fixed for now, sorry.

Ok, no worries. Build seems to be failing intermittently now. I've restarted it a few times and am getting different test failures.

@danielbachhuber
Copy link
Member Author

Build seems to be failing intermittently now. I've restarted it a few times and am getting different test failures.

Deleting the build cache seems to have done the trick. Good to go now.

@danielbachhuber danielbachhuber force-pushed the 2708-only-main-editor branch from bc03138 to 59cd86c Compare July 27, 2018 18:41
@danielbachhuber
Copy link
Member Author

@WordPress/gutenberg-core This is ready to land now.

pento
pento approved these changes Jul 30, 2018
View reviewed changes
Copy link
Member

@pento pento left a comment

Choose a reason for hiding this comment

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

🎉

@pento pento merged commit 2d54b09 into master Jul 30, 2018
@pento pento deleted the 2708-only-main-editor branch July 30, 2018 03:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tofumatt tofumatt tofumatt left review comments

@pento pento pento approved these changes

Assignees
No one assigned
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
3 participants
@danielbachhuber @tofumatt @pento