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

Add a warning to the Classic Editor when the post contains blocks #8247

Merged
merged 8 commits into from Jul 31, 2018

Conversation

pento
Copy link
Member

@pento pento commented Jul 27, 2018

Description

The Classic Editor can do funky things to block markup, so it's nice to warn people before they try to edit block-based posts in the Classic Editor.

Fixes #8213.

Screenshots

screen shot of the warning on desktop

screen shot of the warning on mobile

@pento pento added [Type] Enhancement A suggestion for improvement. Needs Design Needs design efforts. Needs Copy Review Needs review of user-facing copy (language, phrasing) labels Jul 27, 2018
@pento pento added this to the Try Callout milestone Jul 27, 2018
@pento pento self-assigned this Jul 27, 2018
@pento pento requested a review from a team July 27, 2018 08:19
@pento
Copy link
Member Author

pento commented Jul 27, 2018

The design and text obviously need work. 🙂

The functionality is all there.

@afercia afercia added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Jul 27, 2018
@kristastevens
Copy link

I propose a minimalist approach in both scenarios, with the following text and the accompanying buttons:

This post was previously edited in Gutenberg. You can continue in the Classic Editor, but you may lose data and formatting.

In the first example, we have a second paragraph of text that seems redundant, given that it just calls out the options on the buttons.

In the second example, we've also got text in bold to explain an option the user doesn't have available to them which seems odd to me, like we're over-explaining.

@chrisvanpatten
Copy link
Member

chrisvanpatten commented Jul 27, 2018

@kristastevens I think in this case a little over-explanation is warranted, because the user is at risk of data loss. I think with that second example in particular, letting the user know they don't have revisions available is important because they don't have an escape hatch if things go poorly.

That said, over-explanation doesn't have to mean my wordy language :)

Something simpler, like this, could work:

This post was previously edited in Gutenberg. You can continue in the Classic Editor, but you may lose data and formatting.

You can also browse your previous revisions and restore a version of the page before it was edited in Gutenberg.

(This one is slightly tricky because we don't necessarily know that a post was created in Classic and then converted to Gutenberg, but as this is the main context you'd want revisions, I think the message is still appropriate.)

If Revisions are disabled:

This post was previously edited in Gutenberg. You can continue in the Classic Editor, but you may lose data and formatting.

Because this post does not have revisions, you will not be able to revert any changes you make in the Classic Editor.

@kristastevens
Copy link

@chrisvanpatten We are telling them they're at risk of data loss in the single sentence, though I'll defer to your suggestions for additional information.

@chrisvanpatten
Copy link
Member

@kristastevens Totally, it's very clear they're at risk, but I think there's a difference between "You might screw up your content, but you can always restore an old revision if you need to" and "You might screw up your content, and you'll be out of luck if you do because you don't have Revisions turned on".

@afercia
Copy link
Contributor

afercia commented Jul 27, 2018

These modal needs an accessibility treatment. It's very similar to the "Heads up!" warnings introduced for the file editors see https://core.trac.wordpress.org/ticket/31779 and https://core.trac.wordpress.org/changeset/41774

The challenge is that it opens on page load so the "classic" accessibility solutions used for modal dialogs didn't work very well. For example, we've opted to not use role="dialog". Basically, it needs the same improvements done in https://core.trac.wordpress.org/ticket/42110 / https://core.trac.wordpress.org/changeset/41876 so at the very least:

  • set initial focus within the modal
  • constrain tabbing within the modal
  • use wp.a11y.speak() to make screen readers announce the modal message
  • hide all the other page content from assistive technologies using aria-hidden="true"

Code for inspiration is in https://core.trac.wordpress.org/changeset/41876
In order to see again the theme and plugin editor warnings, there's the need to delete from the database theme_editor_notice and plugin_editor_notice from the dismissed_wp_pointers entry in the usermeta.

@afercia
Copy link
Contributor

afercia commented Jul 28, 2018

Just noticed the modal is not responsive. I'd suggest to use the same, simplest possible solution already used for the plugin and theme editors modal, see: https://core.trac.wordpress.org/changeset/41854

@afercia
Copy link
Contributor

afercia commented Jul 28, 2018

On a more general note, I'd like to point to a conversation about modals occurred during the core-editor chat on April 18th: https://wordpress.slack.com/archives/C02QB2JS7/p1524059503000582 /Cc @karmatosed

To summarise: modals that open automatically on page load are rarely a good user experience:

  • users tend to dismiss them because of the negative pop-up learnt behaviour
  • they interrupt user experience
  • generally, disruptions and distractions like modals cause a break in cognitive experience
  • modals only work when they're a desired transition

So there's a big difference between modals that open automatically and modals that are invoked by users. I'm not fully convinced the one proposed here will be a good user experience.

While the "Heads up!" modal for the plugin and theme editors appears just once (thus is maybe an acceptable trade-off), this one opens every single time a post with blocks is opened in the Classic Editor. There's no option to dismiss it permanently.

@afercia
Copy link
Contributor

afercia commented Jul 28, 2018

Overall, I understand this warning is a first step in trying to solve the general issue but I'm not sure it solves all the concerns expressed on #8213 and in the related discussions on Slack, #core-editor. I don't want to repeat all the points raised there so, for reference:

As I see it, the relevant points to make a decision about are:

a post started in Gutenberg should not be edited in the classic editor

and

switching back and forth between using Gutenberg and the classic editor is not something that should be done

I'd just like to remind everyone what happened recently with the Text widget, that was a good lesson. I'd tend to think the responsibility of avoiding loosing data or formatting shouldn't be shifted to the users. That should be a responsibility of the software. If there's no good technical solution, then I'd tend to think switching back and forth between using Gutenberg and the classic editor shouldn't be allowed. /Cc @azaozz

Worth also considering the impact any decision made here will have on first impressions of Gutenberg.

@chrisvanpatten
Copy link
Member

chrisvanpatten commented Jul 28, 2018

If there's no good technical solution, then I'd tend to think switching back and forth between using Gutenberg and the classic editor shouldn't be allowed.

I think this makes a lot of sense, but don't know that it will ever be possible to truly prevent this. If someone disables Gutenberg, they are back to editing in Classic with no warning and nothing to prevent them from editing (unless this was patched into core, which seems unlikely at this stage).

It seems like the only way to truly solve this would be to create backups of content before it's converted to blocks, but that introduces a bunch of other tradeoffs (expectations that it would remain in sync with Gutenberg changes, UI to access those backups, etc).

In some ways, it feels like there are competing interests. Obviously no one wants user content to be lost, but I think to a degree those are the situations that are most essential to finishing up Gutenberg dev, and a key reason behind this callout in the first place: finding those edge cases where things fall completely apart due to whatever strange combination of obscure plugins and themes.

(EDIT: To be clear the above are just personal musings, not necessarily trying to endorse one path or another. This is a complex problem; just trying to think it through.)

@afercia
Copy link
Contributor

afercia commented Jul 28, 2018

Yeah I'm aware of the issues deriving from just disabling Gutenberg and that's what I meant above with "I'm not sure it solves all the concerns expressed on #8213". Still, I think the current approach greatly underestimates the issue. It's a matter of trust. With the call to try Gutenberg, WordPress is communicating to users that Gutenberg is "ready". Users will try it also on production sites. And if something goes wrong with content and formatting (which is very likely) trust will be lost. If the decision will be to use just the warning, at the very least it should be patched in core. This implies the release should be postponed a few days or a week. I don't see anything bad in that, as it would be for a greater good.

However, I'd still recommend to consider a different approach. A bit ironically, the "try it" phase (when Gutenberg will be installed as a plugin) will be the most critical one. Instead, if Gutenberg was merged in core, there would be other options, for example introducing a strict distinction between classic-posts and Gutenberg-posts.

@karmatosed
Copy link
Member

I agree with @kristastevens and going for a more minimal version here makes sense. Design wise as in the classic editor can we use the post status lock modal? I assume that has had work done to make it accessible @afercia? I agree modals are a bad experience but this is a one off. We could consider a message but I think for a first step a modal warning is also ok and post status lock has a comparative behaviour we can draw on, being first load.

I am removing 'needs design' as in this case we should use the existing design.

@karmatosed karmatosed removed the Needs Design Needs design efforts. label Jul 29, 2018
@pento
Copy link
Member Author

pento commented Jul 30, 2018

Thank you for the feedback, everyone!

I've updated the screenshots in the PR description, notable changes include:

  • Adding a heading.
  • Rewording the text.
  • Shortening the button text, adding title attributes to them.
  • Making it responsive.
  • Making it accessible.

Thank you in particular to @afercia, the examples you gave were super helpful in understanding both the accessibility requirements, and the practical steps needed to get there. 🙂

I'll also address some of the discussion here:

I'm not sure it solves all the concerns expressed on #8213 and in the related discussions on Slack, #core-editor.

That's a fair point, but I don't think #8213 expresses the entire view of the problem. Right now, classic mangles the HTML and delimiter comments. It's not unrecoverable, but it is a frustrating experience. The intention is to improve classic's handling of blocks, but there are going to be a lot of weird edge cases that we run into. Part of the point if the Try Gutenberg callout is to help discover those cases, and prioritise them. While we collect and fix this cases, however, it's unfair on the end user to not give them a warning that this could happen, along with specific actions they can take to handle the issue.

So, this PR addresses the problem expressed in #8213, that users can mangle their posts without an easy way out. This PR gives them that easy way out. The further work of fixing classic's behaviour will be handled in future Gutenberg releases.

I'd just like to remind everyone what happened recently with the Text widget, that was a good lesson. I'd tend to think the responsibility of avoiding loosing data or formatting shouldn't be shifted to the users.

I agree, WordPress does need to make all efforts to avoid losing data, without shifting that responsibility to the user. That's why WordPress 5.0 will need a better solution than this warning dialog. Trying to handle all of those cases before we know what they are, however, would be premature.

If the decision will be to use just the warning, at the very least it should be patched in core.

We should certainly get it into Core, but I don't believe that it's a good idea to put it into 4.9.8. Putting it Gutenberg for now allows us to iterate on it much more quickly, and to start disabling it in cases where we can detect that the classic editor can handle the block markup without mangling it all.

Design wise as in the classic editor can we use the post status lock modal?

This is based on the post lock modal: I literally copy/pasted the HTML and relevant CSS. 🙂

I added the 'Needs Design' tag to check that you were fine with it, it looked kind of bad to begin with. Reducing the amount of text, and adding the heading made it better.

@afercia
Copy link
Contributor

afercia commented Jul 30, 2018

I assume that has had work done to make it accessible @afercia?

@pento already made it, thanks 🎉 (just to clarify again: this is not the recommended way to implement a modal and was done this way just because these modals appear on page load)

@pento pento modified the milestones: Try Callout, 3.4 Jul 30, 2018
@karmatosed
Copy link
Member

My suggestion would be to iterate the copy a bit to this:

artboard

@pento
Copy link
Member Author

pento commented Jul 30, 2018

Thanks for the suggestion, @karmatosed!

I updated the PR, and the screenshots in the description.

@aduth aduth self-requested a review July 30, 2018 12:51
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

It's a bit odd that if a user chooses "Edit in Classic Editor", then proceeds to save the post, they're once again prompted with the original notice.

lib/compat.php Outdated
return;
}

wp_enqueue_script( 'wp-sanitize' );
Copy link
Member

Choose a reason for hiding this comment

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

At first glance, I wasn't clear what this was here for (could do for code comment). I eventually found the use in inline script below. Appears then we'd also need to enqueue the dependency making wp.a11y available?

@pento
Copy link
Member Author

pento commented Jul 31, 2018

Thanks for the feedback, @aduth. Clicking the classic editor button now closes the dialog (instead of relying on the URL parameter, which can cause a bunch of weird issues). I've also explicitly enqueued wp-a11y (it's already enqueued by a bunch of other things, but it's good to explicitly include it).

@pento pento merged commit d54a33b into master Jul 31, 2018
@pento pento deleted the add/8213-classic-editor-block-warning branch July 31, 2018 02:28
@afercia
Copy link
Contributor

afercia commented Jul 31, 2018

We shouldn't introduce new title attributes in core, especially after all the work done to remove most of them, see https://core.trac.wordpress.org/query?keywords=~title-attribute
Title attributes are not available to keyboard users, and may not be announced by screen readers depending on user settings. UI controls should be labelled in a way that's clear without requiring additional (semi-hidden) information that's not available to all users.
Will open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Copy Review Needs review of user-facing copy (language, phrasing) [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants