Amp-wp: Wrapper elements causes problem for conversion in Block Resolving for Stories

[westonruter]

I made a story page as depicted in #2368 and then when I tried switching to the branch, I got the expected:

When clicking “Resolve” the suggested resolution is not correct:

The amp-story-grid-layer and the amp-story-block-wrapper are incorrectly getting added a second time.

Fixing this will be very important once stories are in a stable release, in addition to making sure that we start maintaining deprecated conversions.

[swissspidy]

Note: Also reported in #2376.

[swissspidy]

I cannot reproduce this with the steps outlined in #2376, which would have been a more likely scenario than switching branches.

However, given that theses divs are added by wrapBlocksInGridLayer, I wonder if that function could simply check element to see whether these divs already exist.

Also, since this function is hooked to blocks.getSaveElement, maybe this is a bug in Gutenberg. https://github.com/WordPress/gutenberg/issues/9811 looks relevant here.

[westonruter]

@swissspidy Here's a quick way to easily reproduce this:

1. Start npm run dev
2. Start a story with a single text block.
3. Save draft.
4. Make a change to the output of the block's save function, for example:

diff --git a/assets/src/stories-editor/blocks/amp-story-text/save.js b/assets/src/stories-editor/blocks/amp-story-text/save.js
index b74a1ccd..afc57b99 100644
--- a/assets/src/stories-editor/blocks/amp-story-text/save.js
+++ b/assets/src/stories-editor/blocks/amp-story-text/save.js
@@ -45,7 +45,7 @@ const TextBlockSave = ( { attributes } ) => {
        <ContentTag
            style={ styles }
            className={ className }>
-           <amp-fit-text layout="flex-item" className="amp-text-content"><RawHTML>{ content }</RawHTML></amp-fit-text>
+           <amp-fit-text layout="flex-item" className="amp-fitted-text-content"><RawHTML>{ content }</RawHTML></amp-fit-text>
        </ContentTag>
    );
 };

1. Reload the editor and voilà:

[swissspidy]

@westonruter That‘s not really common, is it? When we change the markup in the future we‘d properly deprecate the old one.

[westonruter]

Well, it _has_ happened very frequently. I _hope_ it wouldn't happen after we release, but there should still be graceful handling if it does happen (e.g. if someone tries editing the HTML directly).

[swissspidy]

Created a PR at #2484 that should theoretically fix this.

[kienstra]

Technical QA?

Hi @swissspidy,
Do you think the 'technical QA' as part of your #2484 is enough to move this to 'Ready for merging?' Or do you think more functional testing would help?

[swissspidy]

I think we're good here!