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

Editor: Switching between Visual/HTML editors marks page as dirty, gives double prompt #6869

Closed
hoverduck opened this issue Jul 18, 2016 · 8 comments
Labels
[Feature] Post/Page Editor The editor for editing posts and pages. [Type] Bug When a feature is broken and / or not performing as intended

Comments

@hoverduck
Copy link
Contributor

Steps to reproduce

  1. Start a new post or page
  2. Do not add any content, but switch between the Visual and HTML editor
  3. Navigate back to My Sites
  4. Navigate to another section of Calypso

What I expected

To be able to move around without issue

What happened instead

I was prompted with an alert saying I had unsaved changes when leaving the editor. That's not ideal, but maybe not a problem. What concerns me more is that I was prompted again when navigating elsewhere in Calypso (on Chrome only, Firefox didn't show that problem). Possibly related to #6859 from earlier today? cc: @nylen

Browser / OS version

Chrome v51 on OSX and Windows 10
Firefox v46 OSX

Screenshot / Video

wp-calypso-dirty-post-alert

Context / Source

manual-testing

@hoverduck hoverduck added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Post/Page Editor The editor for editing posts and pages. labels Jul 18, 2016
@aduth
Copy link
Contributor

aduth commented Jul 18, 2016

The second prompt (step 4) should now be resolved as of #6872, but the issue remains that the editor incorrectly flags a post as being dirty after merely switching editing modes.

@aduth
Copy link
Contributor

aduth commented Sep 5, 2017

After some debugging, the main culprits here are:

  • Escaping: The initial "saved" post will have encoded characters (>, &), and if the post is updated during an edit session with equivalent unencoded characters, they will falsely flag the post as dirty. 'Ribs & Chicken' !== 'Ribs & Chicken, even if they're effectively the same.
  • wpautop: The HTML tab shows its content without paragraphs (removep), but the content as generated in Visual includes paragraphs. '<p>Hello world</p>' !== 'Hello world', even if they're effectively the same.

The solution here is to devise a consistent format for each. If we want unencoded characters to be reflected in an updated post, we ought to decode characters from the saved post when initially loading the editor. Similarly, if we want to use removep for an updated post, we ought to remove paragraph tags from the saved post, and strip them from TinyMCE content before comparing.

@aduth
Copy link
Contributor

aduth commented Sep 5, 2017

This is also related to how we manage content in the editor. Because we decided early on that TinyMCE's getContent was not performant enough to call frequently (in response to user keypresses), we maintain two variations of content: a "raw" (performant but inaccurate) value and the formatted (non-performant) value. The raw content is not run through removep and will include paragraph tags. We can then compare the raw content to what the post had been initialized with when determining if the post has unsaved changes.

Because in the HTML view we do not show or want to show paragraph tags, we need to make sure that raw content is reset in such a way that it is compatible with the textarea value.

In early development, we did so by resetting the raw content when switching to the HTML mode (10420-gh-calypso-pre-oss). Upon closer inspection, I found that I had mistakenly removed this in #5438, likely because the placeholder actions being removed included one of the same name (resetRawContent). In restoring this in my local development environment, I find that it helps in some cases to avoid the post from being flagged as dirty. Where it doesn't work so well is with encoded entities. Here's the log entries where I switch from Visual to HTML modes in a content with a > character:

calypso:posts:post-edit-store Set initial raw content to: null +3s
calypso:posts:post-edit-store Set raw content to: null +1ms
calypso:posts:post-edit-store Set initial raw content to: Ribs &gt; Chicken! +8ms
calypso:posts:post-edit-store Set raw content to: Ribs &gt; Chicken! +1ms
calypso:posts:post-edit-store Set raw content to: Ribs > Chicken! +165ms

Note that while initial raw content and raw content are initially set as the same with an encoded > character, it is immediately set again to the decoded value. This flags the post as dirty, despite no real changes being made. I believe this decoding is occurring here:

setTextAreaContent: function( content ) {
this.setState( {
content: decodeEntities( content )
}, this.doAutosizeUpdate );
},

It seems we'll want one or both of the following:

  • More reliable mechanisms for resetting initial raw content to the desired initial value (decoded or encoded). Currently it is set only if not previously assigned.
  • Not setting HTML content as the "raw" content

The second of these is more interesting. Since the whole purpose of raw content relates to TinyMCE content getters, it seems strange that we try to maintain the value using the textarea value, when we could instead set content directly into the post. Of course, this would mean we'd have to reset raw content back again from post content if the user then switches back to the Visual editor.

@aduth
Copy link
Contributor

aduth commented Sep 5, 2017

The sourcecode shortcode in particular is difficult to manage, and while #8325 resolved some issues around the shortcode in particular, it introduced other new issues. Notably, we shouldn't be blindly decoding entities when switching from Visual to HTML editor, or at least doing so is causing dirty detection to consider the post as changed, since the original saved post's content will be content-encoded, and switching from HTML to Visual mode's will set the unencoded value into the post store.

In wp-admin, the HTML tab's content will always show with encoded value. Since #8325 in Calypso this is not the case. It's arguable which would be preferable, but it's worth pointing out that the value for a sourcecode shortcode must be decoded, else the post will save as double-encoded content (the original issue #8325 intended to resolve).

So we can have encoded values with sourcecode shortcode exempt:

Ribs &gt; Chicken!

[code]
<div>hello</div>
[/code]

Or we can have decoded values:

Ribs > Chicken!

[code]
<div>hello</div>
[/code]

@alisterscott
Copy link
Contributor

Confirming this is still an issue

#bug-scrub #pre-gutenburg-bug-purge

@nylen
Copy link
Contributor

nylen commented Aug 9, 2018

@hoverduck @aduth @alisterscott

Can one of you un-assign me from this issue please?

@nylen nylen unassigned nylen Aug 9, 2018
@nylen
Copy link
Contributor

nylen commented Aug 9, 2018

Huh, never mind, it looks like GitHub decided to do it for me when I commented. Thanks anyway!

@davipontesblog
Copy link
Contributor

I am closing this as the classic editor in calypso is being deprecated in favor of the block editor. If needed, please feel free to reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Post/Page Editor The editor for editing posts and pages. [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants