Stop trying to autosave when title and classic block content both are empty. #10404

rahulsprajapati · 2018-10-08T14:29:41Z

Description

Add check in getBlocksForSerialization when single classic block ( core/freeform ) is available with empty content.

gutenberg/packages/editor/src/store/selectors.js

Lines 372 to 381 in 69ced17

    
           export function isEditedPostEmpty( state ) { 
        
           	// While the condition of truthy content string would be sufficient for 
        
           	// determining emptiness, testing saveable blocks length is a trivial 
        
           	// operation by comparison. Since this function can be called frequently, 
        
           	// optimize for the fast case where saveable blocks are non-empty. 
        
           	return ( 
        
           		! getBlocksForSerialization( state ).length && 
        
           		! getEditedPostAttribute( state, 'content' ) 
        
           	); 
        
           }

How has this been tested?

Post autosave is not triggerd when one classic block content is availble and it's empty.
Screencast: https://drive.google.com/file/d/1p6HfNi7I6_57y8mbLhe9J8HTU4lAq--6/view

Screenshots

Types of changes

Bug fix (non-breaking change which fixes an issue)

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

👍 I've tested this locally and confirm autosave doesn't run until some text is entered in the post content.

I'll defer to @aduth or @youknowriad on the underlying JavaScript

aduth

My suggestion noted at #6556 (comment) would avoid the need to inspect specific attributes of the block, rather relying on the existing serializer behavior to be performed, testing the result of which as being non-empty to determine whether a save can occur.

Pseudo-code:

const isEditedPostEmpty = ( state ) => ! hasKnownBlock( state ) && ! getEditedPostAttribute( state, 'content' ) 

const hasKnownBlock = ( state ) => getBlockOrder( state ).some( ( clientId ) => (
	state.editor.current.blocksByClientId[ clientId ].name !== getUnknownTypeHandlerName()
) );

(In second function, I avoid getBlocks because it has a non-trivial cost to execute)

aduth · 2018-10-10T15:50:09Z

packages/editor/src/store/selectors.js

+	const isSingleEmptyFreeformBlock = (
+		blocks.length === 1 &&
+		blocks[ 0 ].name === 'core/freeform' &&
+		( isEmpty( blocks[ 0 ].attributes ) || blocks[ 0 ].attributes.content === '' )


This pierces the abstraction of blocks by duplicating the save implementation of the freeform block between block-library and editor. Something like the above isUnmodifiedDefaultBlock (in blocks) is a small improvement.

Hey, I think I'm confused about this. Can you give me more( simple ) detail?
Sorry, Gutenberg and react is still new for me so I might not be doing this right.

Sorry for being unclear. In short: We shouldn't pretend to know anything about what attributes do or don't exist here, because it will make our lives more difficult later if we ever need to change the attributes.

It's not entirely relevant if we consider the recommendation at #10404 (review)

ok, yeah sure. Got it we should not use specific attribute name from a direct object and dependent on that. Also, this one is just checking for one empty block there might be more so it would be better if we check in all blocks in the current state. #10404 (review)

I'll update this occordingly.

aduth · 2018-10-10T15:51:47Z

packages/editor/src/store/selectors.js

+	// A single empty core/freeform block equivalent to an empty post.
+	const isSingleEmptyFreeformBlock = (
+		blocks.length === 1 &&
+		blocks[ 0 ].name === 'core/freeform' &&


The fallback block for which comment demarcation is omitted is not guaranteed to be explicitly 'core/freeform'.

See:

gutenberg/packages/blocks/src/api/serializer.js

Lines 263 to 269 in 286317c

switch ( blockName ) {

case getUnknownTypeHandlerName():

return saveContent;

default:

return getCommentDelimitedContent( blockName, saveAttributes, saveContent );

}

As in this snippet, it should be tested against the value returned by getUnknownTypeHandlerName.

Updated this with getUnknownTypeHandlerName,

rahulsprajapati · 2018-10-15T11:03:48Z

Sure @aduth, I'll look into this feedbacks and update this PR soon.

danielbachhuber · 2018-10-18T00:32:00Z

@rahulsprajapati Any chance you can finish this up Thursday (10/17) IST so we can get this landed?

aduth

As of #8274, getUnknownTypeHandlerName has been replaced by two functions getFreeformFallbackBlockName and getUnregisteredFallbackBlockName.

You'll need to update your branch to bring in the latest master changes.

rahulsprajapati · 2018-10-18T18:55:06Z

As of #8274, getUnknownTypeHandlerName has been replaced by two functions getFreeformFallbackBlockName and getUnregisteredFallbackBlockName.

sure, will update this.

aduth · 2018-10-19T13:25:50Z

packages/editor/src/store/selectors.js

@@ -360,6 +361,20 @@ export function isEditedPostSaveable( state ) {
 	);
 }

+/**


One of the test failures is due to the fact that documentation needs to be regenerated by npm run docs:build.

aduth · 2018-10-19T13:29:44Z

packages/editor/src/store/selectors.js

@@ -375,7 +390,7 @@ export function isEditedPostEmpty( state ) {
 	// operation by comparison. Since this function can be called frequently,
 	// optimize for the fast case where saveable blocks are non-empty.
 	return (
-		! getBlocksForSerialization( state ).length &&


Hmm, in retrospect my original suggestion may have been made at a time before we'd considered the extra condition added by getBlocksForSerialization to consider the empty paragraph (see #9808). I've got to imagine this change here regresses on that intended behavior.

Maybe we should update hasKnownBlocks to accept blocks as an argument, so we can pass the result of getBlocksForSerialization to use in place of getBlockOrder.

aduth · 2018-10-19T13:37:07Z

packages/editor/src/store/selectors.js

+export function hasKnownBlocks( state ) {
+	return getBlockOrder( state ).some( ( clientId ) => (
+		state.editor.present.blocksByClientId[ clientId ].name !== getFreeformContentHandlerName() &&
+		state.editor.present.blocksByClientId[ clientId ].name !== getUnregisteredTypeHandlerName()


The split of these two functions has me wondering if the semantics are meaningfully different enough to consider. "Unknown" would be truly a block, with demarcations, and thus non-empty for save. I think what we're really targeting is specifically freeform content.

Thus, I think we don't need this line and it could be dropped.

In doing so, and in reconsideration of targeting specifically freeform content, I wonder if the function name we've chosen hasKnownBlocks is very accurate. I'll need to sit and think on this one...

I wonder if the function name we've chosen hasKnownBlocks is very accurate. I'll need to sit and think on this one...

If we are just going to check that the post have any freeform block or not then hasFreeFromBlock name will be more suitable for this.

That name seems to suit the purpose, but it highlights to me that it's so specific to the isEditedPostEmpty use-case that we might be better off to include the logic there, rather than to expose a new separate selector.

Thinking something like (haven't verified the logic):

export function isEditedPostEmpty( state ) { const blocks = getBlocksForSerialization( state ); if ( ! blocks.length ) { return true; } const freeformBlockName = getFreeformContentHandlerName(); if ( ! some( blocks, { name: freeformBlockName } ) ) { return false; } // Content serialization is considered an expensive operation, and should // only be considered after more trivial operations of assuming presence of // non-freeform blocks as implying that serializable content exists. return ! getEditedPostAttribute( state, 'content' ); }

@aduth This works correctly.

@rahulsprajapati @aduth : Yup, the code works correctly, but while writing test cases for this, existing test case should return false if edits include a non-empty content property fails. So I think need to check for a more suitable solution. What do you guys say?

aduth · 2018-10-30T21:36:53Z

There appear to be some test failures for the impacted selectors. This is otherwise looking quite nice.

rahulsprajapati · 2018-10-31T04:13:44Z

Great. I'll look into tests failure.

aduth · 2018-10-31T14:40:24Z

I looked over the tests and found that we had a few issues in their authoring:

We assumed core/freeform was the name of the freeform handler, but this is not assigned in the test environment (intentionally so).
- The tests now register and set a freeform content handler test block
The block entries lacked a necessary isValid flag which is important for consideration by the block serializer

I also discovered a completely separate bug in our optimization of blocks existing, in that a content edit should always take precedence. This can occur if, for example, the post has blocks but the user switches to the Code Editor mode and removes all content. It's my understanding based on the current implementation in master that this would be an existing issue, solved in the most recent commit by considering the content edit.

I've further revised the implementation to avoid Array#some, since we can be certain that if there is more than one block, even if both blocks are the freeform block, and both freeform blocks produce an empty save result, that the overall serialized result would still be non-empty thanks to the Array#join which occurs in the serializer. This allows us to consider a trivial operation (and common occurrence) of blocks.length > 1 as non-empty.

rahulsprajapati · 2018-10-31T17:36:03Z

Thank you @aduth for the explanation. 🙂

I was looking over the test only and was wondering why it's failing when the test case was correct and then just found that free-form was not detected with getFreeformContentHandlerName() but I guess got too slow 😅

… unregistered or empty content block is available in post.

danielbachhuber · 2018-10-31T21:14:50Z

I've manually verified that autosave doesn't fire when Title is empty and the post only has a Classic Block.

Thanks for your work on this @rahulsprajapati, @aduth, @sanketio

…rnmobile/port-quote-block-step-1 * 'master' of https://github.com/WordPress/gutenberg: (22 commits) Add removed periods to block descriptions. (#11367) Remove findDOMNode usage from the inserter (#11363) Remove deprecated componentWillReceiveProps from TinyMCE component (#11368) Create file blocks when dropping multiple files at once (#11297) Try avoiding the deprecated findDOMNode API from DropZone Provider (#11168) Fix: make meta+A behaviour of selecting all blocks work on safari and firefox. (#8180) Remove _wpGutenbergCodeEditorSettings and wp.codeEditor assets (#11342) Remove the Cloudflare warning (#11350) Image Block: Use source_url for media file link (#11254) Enhance styling of nextpage block using the Hr element (#11354) Embed block refactor and tidy (#10958) Nonce Middleware: Wrap the nonce middleware function into it's own function that isn't regenerated on every API request. (#11347) Fix RTL block alignments (#11293) RichText: fix buggy enter/delete behaviour (#11287) Remove code coverage setup (#11198) Parser: Runs all parser implementations against the same tests (#11320) Stop trying to autosave when title and classic block content both are empty. (#10404) Fix "Mac OS" typo + use fancy quotes consistently (#11310) Update documentation link paths (#11324) Editor: Reshape blocks state under own key (#11315) ... # Conflicts: # gutenberg-mobile

danielbachhuber added this to the 4.1 milestone Oct 9, 2018

danielbachhuber requested review from aduth and danielbachhuber October 9, 2018 19:17

danielbachhuber approved these changes Oct 9, 2018

View reviewed changes

danielbachhuber added [Type] Bug An existing feature does not function as intended Backwards Compatibility Issues or PRs that impact backwards compatability [Feature] Saving Related to saving functionality labels Oct 9, 2018

danielbachhuber requested a review from youknowriad October 9, 2018 19:29

aduth requested changes Oct 10, 2018

View reviewed changes

aduth modified the milestones: 4.1 - UI freeze, 4.2 - API freeze Oct 18, 2018

aduth requested changes Oct 18, 2018

View reviewed changes

aduth reviewed Oct 19, 2018

View reviewed changes

youknowriad modified the milestones: 4.2, WordPress 5.0 Oct 30, 2018

aduth approved these changes Oct 31, 2018

View reviewed changes

aduth force-pushed the fix/empty-classic-block-update-fail branch 2 times, most recently from 280f8ae to a25bc1a Compare October 31, 2018 19:24

rahulsprajapati and others added 4 commits October 31, 2018 16:16

Do not auto-update if title and classic block content is empty.

16deea4

Update isEditedPostEmpty: use hasKnownBlocks and check if freeform or…

0da3b58

… unregistered or empty content block is available in post.

Update isEditablePost logic to check length of availalbe block first.

75517a2

Editor: Improve selector consideration of empty content edit

4c07ae6

aduth force-pushed the fix/empty-classic-block-update-fail branch from a25bc1a to 4c07ae6 Compare October 31, 2018 20:21

danielbachhuber merged commit fae7777 into WordPress:master Oct 31, 2018

mtias modified the milestones: WordPress 5.0 RC, 4.3 Nov 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop trying to autosave when title and classic block content both are empty. #10404

Stop trying to autosave when title and classic block content both are empty. #10404

rahulsprajapati commented Oct 8, 2018

danielbachhuber left a comment

aduth left a comment

aduth Oct 10, 2018

rahulsprajapati Oct 18, 2018

aduth Oct 18, 2018

rahulsprajapati Oct 18, 2018

aduth Oct 10, 2018

rahulsprajapati Oct 18, 2018

rahulsprajapati commented Oct 15, 2018

danielbachhuber commented Oct 18, 2018

aduth left a comment

rahulsprajapati commented Oct 18, 2018

aduth Oct 19, 2018

aduth Oct 19, 2018

aduth Oct 19, 2018

rahulsprajapati Oct 19, 2018

aduth Oct 22, 2018

rahulsprajapati Oct 30, 2018

sanketio Oct 31, 2018 •

edited

Loading

aduth commented Oct 30, 2018 •

edited

Loading

rahulsprajapati commented Oct 31, 2018

aduth commented Oct 31, 2018

rahulsprajapati commented Oct 31, 2018

danielbachhuber commented Oct 31, 2018

	export function isEditedPostEmpty( state ) {
	// While the condition of truthy content string would be sufficient for
	// determining emptiness, testing saveable blocks length is a trivial
	// operation by comparison. Since this function can be called frequently,
	// optimize for the fast case where saveable blocks are non-empty.
	return (
	! getBlocksForSerialization( state ).length &&
	! getEditedPostAttribute( state, 'content' )
	);
	}

	switch ( blockName ) {
	case getUnknownTypeHandlerName():
	return saveContent;

	default:
	return getCommentDelimitedContent( blockName, saveAttributes, saveContent );
	}

@@ @@ -360,6 +361,20 @@ export function isEditedPostSaveable( state ) { @@
               	);
               }
+              /**

Stop trying to autosave when title and classic block content both are empty. #10404

Stop trying to autosave when title and classic block content both are empty. #10404

Conversation

rahulsprajapati commented Oct 8, 2018

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

danielbachhuber left a comment

Choose a reason for hiding this comment

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahulsprajapati commented Oct 15, 2018

danielbachhuber commented Oct 18, 2018

aduth left a comment

Choose a reason for hiding this comment

rahulsprajapati commented Oct 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanketio Oct 31, 2018 • edited Loading

Choose a reason for hiding this comment

aduth commented Oct 30, 2018 • edited Loading

rahulsprajapati commented Oct 31, 2018

aduth commented Oct 31, 2018

rahulsprajapati commented Oct 31, 2018

danielbachhuber commented Oct 31, 2018

sanketio Oct 31, 2018 •

edited

Loading

aduth commented Oct 30, 2018 •

edited

Loading