Block warning: allow re-editing of HTML #9088

johngodley · 2018-08-17T12:48:21Z

If a block is flagged as invalid then allow it to be re-edited in HTML mode so the content can be fixed. This allows a quicker route to fixing an invalid block if you happen to make a mistake in the HTML.

Leads to:

Note that selecting 'edit visually' while in HTML will re-trigger the invalid warning if the problem has not been fixed.

This fixes the issue of the 'edit as HTML' / 'edit visually' block options being available but having no effect in #7743

How has this been tested?

Additional unit test has been added to verify that a block can be re-edited in HTML mode.

To manually test:

Add a paragraph block, edit as HTML, and remove the trailing </p> - tab out of the editor to trigger an invalid block warning
From the block menu select 'Edit as HTML'
Verify that the invalid block warning is removed and the HTML editor is shown with the invalid HTML content
Verify that selecting 'Edit visually' from the block menu triggers the invalid block warning again

Types of changes

Potentially breaking feature.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

ZebulanStanphill · 2018-08-17T18:08:56Z

Nice change! I think this is a good idea. Previously you would have had to use the Undo button to fix a block that you made invalid without potentially losing data (due to issues like #6878) or converting to a Custom HTML block.

On a side note, I feel like the Keep as HTML button should be renamed to Convert to Custom HTML block, or Convert to HTML block, or Keep as HTML block, or something like that. As it is, Keep as HTML is kind of misleading.

jorgefilipecosta

I like this feature, and it may be a good option to fix blocks if the user changed them by mistake in the code editor for example.
In the future, we may consider adding an "Edit As HTML" button in the invalid block waring so the user knows this option is available in case the block is valid. Right now this feature is a bit hidden and the user is only aware of this option if the menu is open.

jorgefilipecosta · 2018-08-22T19:01:49Z

packages/editor/src/components/block-list/block-html.js

+
+		if ( ! isValid ) {
+			// An error was detected so flip the block back to visual mode so we see the invalid block warning
+			this.props.onToggleMode( this.props.clientId );


Switching back to visual mode if the block becomes invalid may cause some troubles. Blur events are very easy to trigger, and the user may be in the invalid state temporarily while in process of writing a valid change.
I would prefer if the switch was not automatic, but I would love other options regarding the UX cc: @jasmussen, @karmatosed.
Another possibility for automatic switching to the visual mode when the block is invalid would be switching when another block is selected/block is unselected. It would also make the user notice the warning, but the switch is not as unexpected as with the blur event.

I probably didn't explain this well in the description but nothing really changes for the user, and the behaviour is the same as currently.

When the onBlur fires the HTML validation is performed and the block is set as invalid, showing the invalid block message. At this point the block is still in HTML mode, although you don't see this, and the dropdown menu option will be 'Edit visually' (which we don't want).

The extra onToggleMode switches the block to visual mode so the menu will show `Edit as HTML'. The only change is for the menu toggle - nothing will visually change for the user as the block is invalid and will continue to show the invalid block message (i.e. the visual mode is not shown)

jorgefilipecosta · 2018-08-22T19:10:03Z

packages/editor/src/components/block-list/block.js

@@ -405,9 +405,12 @@ export class BlockListBlock extends Component {
 		const shouldShowInsertionPoint = ( isPartOfMultiSelection && isFirst ) || ! isPartOfMultiSelection;
 		const canShowInBetweenInserter = ! isEmptyDefaultBlock && ! isPreviousBlockADefaultEmptyBlock;

+		// We allow an invalid block to be re-edited in HTML mode
+		const isValidMode = isValid || mode === 'html';


Minor: the variable name suggests that it checks if the mode is valid or not, while the variable contains if the block is valid or we are in HTML mode. Maybe we can call it isValidOrModeIsHtml.

Would it be isValidOrModeIsHTML or isValidOrModeIsHtml? I forgot what the consensus was for acronyms in variable names. (I know id should be id if at the start or Id otherwise, since it is not an acronym but just an abbreviation like var is an abbreviation of variable.)

johngodley · 2018-08-23T12:18:59Z

In the future, we may consider adding an "Edit As HTML" button in the invalid block waring so the user knows this option is available in case the block is valid. Right now this feature is a bit hidden and the user is only aware of this option if the menu is open.

Yep. It came up in discussion in #7743 (comment) where it was decided not to do that right now, but it's worth considering in the future if things change.

jasmussen · 2018-08-24T07:32:29Z

So here's a GIF:

Steps taken:

Click Edit as HTML
Insert a single unclosed <div>
Click outside

In master, and in this branch, clicking outside switches back to Visual editing for the block, and it shows the block warning.

I'm not against merging improvements even with that behavior in place, however it seems like the better experience would be that after you start editing in HTML mode and click outside, the block remains in HTML mode, and the warning is only triggered once you explicitly exit HTML mode for that block.

However I see the problem with that too.

Another solution would be to always switch out of HTML mode when you click outside the block, at least then it would be consistent behavior.

A third option would probably be more ideal, though not sure if it's in scope of PR. That is to show the block warning also in HTML mode — it would have to be a warning that doesn't change the size of the block itself, still allows you to continue editing and so on. Mockup:

This follows a separate idea that we've had, of adding such an "exit" bar to the code editor too:

What do you think? Easy? Hard? If hard, worth ticketing separately.

johngodley · 2018-08-24T08:44:39Z

I get what you mean in the GIF, although it is still an improvement over master where it's broken!

In master, and in this branch, clicking outside switches back to Visual editing for the block, and it shows the block warning.

In master the block is technically still in HTML mode, but this is hidden. What you see is the invalid block, which displays a visual (and non-editable) render of the block. You can verify this by noting that the dropdown menu on an invalid block shows 'Edit visually', indicating it's in HTML mode.

This PR contains a change that will switch it to visual mode. Again this is still hidden, but enables the dropdown menu to then show the 'edit as HTML' option.

adding such an "exit" bar to the code editor too:

That seems like a good idea. I'm not sure if the dropdown menu disappears in this case, but either way I think it's a separate improvement. The inability to re-edit HTML will still exist though, as once you've exited the HTML editor the block will be invalid and the 'edit as HTML' option will have no effect.

It seems like there are two issues:

What triggers the invalid block (currently onBlur)
Allow an invalid block to be re-edited (currently broken)

This PR attempts to fix 2 using the current onBlur trigger. It makes no changes to 1.

The question is whether this PR is a good step towards fixing 2? Improving 1 can be done in another PR, and the fix to 2 here should still apply (at this point the change in onBlur here probably won't be needed)

jasmussen · 2018-08-24T08:52:13Z

Yep, solid arguments. I think it would be good to ship this, and then look at any additional improvements separately.

johngodley · 2018-08-24T09:01:11Z

I've tracked the trigger issue in #9304

If a block is flagged as containing invalid content then allow it to be re-edited in HTML mode so the content can be fixed.

gziolo · 2019-02-01T09:02:58Z

@mapk and @youknowriad - is this something we should prioritize to include in Phase 2?

youknowriad · 2019-02-01T09:04:47Z

I don't think it needs to be prioritized, this could be considered part of the "tightning up", it's not crucial but good to have.

mapk · 2019-02-05T00:26:59Z

It appears there's not much more to do here, so if it's solid where it's at, LGTM.

aduth

Code-wise, this looks and works well.

Two things:

Could you rebase to resolve the conflicts?
The design of this warning has changed since the pull request was first opened. There's now a separate dropdown menu which appears for an invalid block, in addition to the default block settings menu. I think it's generally okay in that "Edit as HTML" is always in the block settings menu, but I'd be lying if I said it wasn't my first intuition to check under the dropdown menu of the invalid block.

kjellr · 2019-04-24T18:48:31Z

The design of this warning has changed since the pull request was first opened. There's now a separate dropdown menu which appears for an invalid block, in addition to the default block settings menu. I think it's generally okay in that "Edit as HTML" is always in the block settings menu, but I'd be lying if I said it wasn't my first intuition to check under the dropdown menu of the invalid block.

That's a good point, @aduth. I wouldn't mind it being included in two places in this case.

paaljoachim · 2019-08-30T23:16:30Z

Hey @johngodley

What is needed to move this PR onward? Could you give a status update to where the PR is at the moment?

@gziolo

johngodley · 2019-09-02T13:45:11Z

@paaljoachim I doubt very much whether this code still works, and it probably requires more than just a rebase. I'm not looking at it currently, but may return in the future. If anyone wants to take it over then that's great.

youknowriad · 2020-03-18T10:40:18Z

Trying to triage PRs today. Given that we're not investing in this right now and that the PR needs to be rewritten entirely with all the changes in master. I'm going to close this PR for now. Thanks all for your efforts.

johngodley added [Feature] Blocks Overall functionality of blocks [Status] In Progress Tracking issues with work in progress labels Aug 17, 2018

This was referenced Aug 17, 2018

Block options: disable mode toggle when block is in warning state #7886

Closed

Block option 'edit visually' and 'edit HTML' still available when block is in warning state #7743

Open

johngodley changed the title ~~WIP: Block warning: allow re-editing of HTML~~ Block warning: allow re-editing of HTML Aug 21, 2018

johngodley removed the [Status] In Progress Tracking issues with work in progress label Aug 21, 2018

jorgefilipecosta reviewed Aug 22, 2018

View reviewed changes

johngodley mentioned this pull request Aug 24, 2018

Improve invalid block trigger #9304

Closed

johngodley mentioned this pull request Aug 28, 2018

Block Validation, Deprecation and Migration Experience #7604

Open

johngodley requested a review from a team September 6, 2018 12:47

johngodley force-pushed the add/block-reedit-invalid branch from b32e09f to 3cc9f91 Compare September 13, 2018 11:47

johngodley force-pushed the add/block-reedit-invalid branch from ac6a5ff to 2a65971 Compare October 17, 2018 09:23

johngodley force-pushed the add/block-reedit-invalid branch 2 times, most recently from ef58acb to 2affe3d Compare November 1, 2018 12:39

johngodley force-pushed the add/block-reedit-invalid branch from 2affe3d to 5e17532 Compare November 12, 2018 13:47

johngodley added 2 commits December 4, 2018 13:50

Block warning: allow re-editing of HTML

18c50ba

If a block is flagged as containing invalid content then allow it to be re-edited in HTML mode so the content can be fixed.

Fix onBlur unit test

716b15a

johngodley force-pushed the add/block-reedit-invalid branch from 5e17532 to 716b15a Compare December 4, 2018 13:52

gziolo added this to the 5.1 (Gutenberg) milestone Feb 1, 2019

noisysocks modified the milestones: 5.1 (Gutenberg), 5.2 (Gutenberg) Feb 18, 2019

gziolo modified the milestones: 5.2 (Gutenberg), 5.3 (Gutenberg) Mar 4, 2019

youknowriad removed this from the 5.3 (Gutenberg) milestone Mar 18, 2019

aduth reviewed Apr 24, 2019

View reviewed changes

youknowriad closed this Mar 18, 2020

aduth deleted the add/block-reedit-invalid branch March 18, 2020 20:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block warning: allow re-editing of HTML #9088

Block warning: allow re-editing of HTML #9088

johngodley commented Aug 17, 2018

ZebulanStanphill commented Aug 17, 2018

jorgefilipecosta left a comment •

edited

Loading

jorgefilipecosta Aug 22, 2018

johngodley Aug 23, 2018

jorgefilipecosta Aug 22, 2018

ZebulanStanphill Aug 22, 2018

johngodley commented Aug 23, 2018

jasmussen commented Aug 24, 2018

johngodley commented Aug 24, 2018

jasmussen commented Aug 24, 2018

johngodley commented Aug 24, 2018

gziolo commented Feb 1, 2019

youknowriad commented Feb 1, 2019

mapk commented Feb 5, 2019

aduth left a comment

kjellr commented Apr 24, 2019

paaljoachim commented Aug 30, 2019

johngodley commented Sep 2, 2019

youknowriad commented Mar 18, 2020

Block warning: allow re-editing of HTML #9088

Block warning: allow re-editing of HTML #9088

Conversation

johngodley commented Aug 17, 2018

How has this been tested?

Types of changes

Checklist:

ZebulanStanphill commented Aug 17, 2018

jorgefilipecosta left a comment • edited Loading

Choose a reason for hiding this comment

jorgefilipecosta Aug 22, 2018

Choose a reason for hiding this comment

johngodley Aug 23, 2018

Choose a reason for hiding this comment

jorgefilipecosta Aug 22, 2018

Choose a reason for hiding this comment

ZebulanStanphill Aug 22, 2018

Choose a reason for hiding this comment

johngodley commented Aug 23, 2018

jasmussen commented Aug 24, 2018

johngodley commented Aug 24, 2018

jasmussen commented Aug 24, 2018

johngodley commented Aug 24, 2018

gziolo commented Feb 1, 2019

youknowriad commented Feb 1, 2019

mapk commented Feb 5, 2019

aduth left a comment

Choose a reason for hiding this comment

kjellr commented Apr 24, 2019

paaljoachim commented Aug 30, 2019

johngodley commented Sep 2, 2019

youknowriad commented Mar 18, 2020

jorgefilipecosta left a comment •

edited

Loading