Amp-wp: Duplicating CTA / Page Attachment block crashes browser

[spacedmonkey]

Bug Description

In the stories editor, there when a block is a selected, a context menu icon appears in the toolbar that looks like this.

If a CTA block is selected and the duplicate block is selected, the browser becomes unresponsive and sometimes crashes.

There might be some infinite loop, perhaps caused by some faulty React hook somewhere.

Expected Behaviour

Block should fail to duplicate or at least a warning about duplicate CTA blocks is shown.

Steps to reproduce

1. Create new story in editor
2. Create a second page
3. Insert CTA block.
4. Select CTA block.
5. Click three dots in top toolbar.
6. Select duplicate block.

Screenshots

If the browser doesn't crash, then two CTA blocks are shown.

Additional context

• WordPress version: 5.3
• Plugin version: 1.3
• Gutenberg plugin version (if applicable):
• AMP plugin template mode:
• PHP version:
• OS: Ubuntu 18.04
• Browser: Chrome 77.0.3865.75 (Official Build) (64-bit)
• Device: Dell XPS 13

---

Acceptance criteria

• AC1: Duplicating a CTA block should not cause the browser to lag / crash
• AC2: Duplicating a Page Attachment block should not cause the browser to lag / crash

Implementation brief

Reason for crashing

When either a CTA or a Page Attachment block is present on a Page, enforcing the block to be the last block on the Page is triggered. If there are more than one CTA / Page Attachment blocks then this causes the logic to be triggered infinitely since only one block can be the last block:
https://github.com/ampproject/amp-wp/blob/744b640f069c43bca209ad2bf988b105e345a8d4/assets/src/stories-editor/blocks/amp-story-page/edit.js#L145-L166

Suggested solution

If more than one CTA/Page Attachment blocks are detected then before enforcing the block to be the last, two things should happen:

• The extra block(s) should be removed.
• A snackbar notice is displayed to let know about the block being removed.

We could add an additional condition to the same useEffect hook that enforces the CTA/Attachment block to be the last, to only run if there is only one CTA or one Page Attachment block. Additionally, we can add another useEffect which removes the extra block(s) if detected which can also display a snackbar notice saying that the block was removed since each Page can have only 1 CTA block or 1 Attachment block.

QA testing instructions

1. Create a new story
2. Add a new page
3. Add a CTA block
4. In the block toolbar, click on the three dots and then on "Duplicate"
   -> Verify that block is not duplicated, but a warning message appears in the bottom left.
5. Add a new page
6. Add a Page Attachment block
7. In the block toolbar, click on the three dots and then on "Duplicate"
   -> Verify that block is not duplicated, but a warning message appears in the bottom left.

Demo

https://www.youtube.com/watch?v=iItTJkcF7Qg&feature=youtu.be

Changelog entry

• Prevent duplicating CTA or Page Attachment blocks.

[spacedmonkey]

There has already been a lot of work done on context menu for right click. See

Barring the edit as html option in this context menu, the options found in the right click context menu are much more useful. I would recommend removing the built in context menu and removing this the same menu found when right clicking.

This would require following.

1. New option of edit as html added to the right click context menu.
2. Top toolbar context menu would have to allow for duplication and removal of page blocks.

Related: https://github.com/ampproject/amp-wp/pull/3417 https://github.com/ampproject/amp-wp/issues/3466

[spacedmonkey]

The keyboard shortcut CTRL+Shift+D also has the same effect.

[swissspidy]

There seems to be some infinite loop somewhere causing that crash:

I would recommend removing the built in context menu and removing this the same menu found when right clicking.

Let's create a separate issue to discuss that.

[miina]

Looking into this.

[miina]

I'm adding another AC:

• It shouldn't be allowed to duplicate a CTA block.

Haven't looked into the code yet but we have a logic in place that is trying to set the CTA block to be the last on a Page -- if there would be two CTA blocks then that most likely would cause infinite updating. Having 2 CTA blocks shouldn't be allowed anyway, however, warning instead of crashing and/or removing the extra CTA block if this should happen for some reason would be good, too.

[swissspidy]

That sounds like a plausible cause 👍

Preventing wherever possible makes sense, though we cannot remove the keyboard shortcut or the option from the menu at the moment.

[miina]

Ah, okay, thought that it's the context menu but it's happening in case of the default More Options Menu, and that can't be removed.

I guess the same should go for the Page Attachment block as well then -- that also triggers the enforcing being the last block logic.

Any objections to adding Page Attachment to this issue as well since it's basically the same issue?

[swissspidy]

Any objections to adding Page Attachment to this issue as well since it's basically the same issue?

works for me 👍

[miina]

To confirm with UX:
Should we display a snack-bar notice when the forbidden block gets removed?

Otherwise IB ready.

[miina]

Updated the IB to add the snack-bar notification. Ready for review now.

[barklund]

I think this speaks into the larger issue of customising the "More Options" menu (the Gutenberg term is BlockSettingsMenu).

• We want to disable "duplicate" for some elements.
• We want to rename "block" to either "element" or "page" depending on context.
• "Insert Before" / "Insert After" doesn't really make sense except for pages.

There are some checks for some of these things we want to do (the component has a canDuplicate flag), but we can't easily manipulate it and there are no filters readily available.

Should we (rather than fix this single issue only) look into either:

• replacing the BlockSettingsMenu with our own menu as we have done for other elements (e.g. the media inserter)?
• editing BlockSettingsMenu upstream to allow the level of customization that we need?

Or maybe even both, as the latter has a longer turn-around.

Note: We also want to add custom menu items, but that's already possible through the BlockSettingsMenuPluginsExtension, however of course with limited ability for customization here as well in terms of ordering, UI, etc.

[miina]

Should we (rather than fix this single issue only) look into either:

It might make sense fixing this issue first as a bugfix and additionally create a follow-up issue for looking into generally replacing the BlockSettingsMenu or updating it upstream. It'll probably take enough time not to make it to 1.4 otherwise.

Thoughts?

[swissspidy]

I looked into customizing those components in https://github.com/ampproject/amp-wp/issues/3504#issuecomment-544566566, where it seems like replacing those is not easily possible. Upstream contributions might be possible, but it's unsure whether these would be (a) accepted and (b) implemented in time.

It's one of the many symptoms of the reliance on Gutenberg. As discussed in today's meeting, I am planning on writing a more detailed design doc based on #2554 to plan a bigger technical overhaul of the stories editor. This would reduce the strong dependency on Gutenberg to gain more control over the whole editor UI.

[spacedmonkey]

I spent a fair bit of time looking at change or removing the BlockSettingsMenu and couldn't find a way. There is no other hacky way to do it either, as they are no solid css classes to hide it. The menu is also conditional, so way to easily remove it.

I have said before, I believe the context menu that is shown when right clicking should be shown here, when a block is selected. It would keep the two menus in line and make it easier to find the right click options via screen readers etc.

I agree with above statements that we should fix the root issue, as they maybe other ways we haven't thought of yet to copy CTA / attachment blocks.

[miina]

Note that considering the RC this Friday and release next week then that's not a valid option right now. We can "fix" the bug for the release and look into the root issue separately since that will need some planning and will take quite some time to figure out a general best solution + implement it.

Probably would be good to continue the discussion in a separate dedicated issue outside of this bugfix issue.

[swissspidy]

Agree with both of you. Hence the planned design doc in #2554 to evaluate how we can fix the root issue.

[csossi]

Was unable to duplicate, but the message that appears is:
"1 block removed. Only one block of this type is allowed per page"
There wasn't a block removed

[spacedmonkey]

I think that this issue is related to https://github.com/WordPress/gutenberg/issues/14515 . If the allow blocks could be change dynamically then the duplicate block would never appear.

If we fix this upstream, it would be useful considering these lines of code

[kopepasah]

Issue was erroneously closed automatically before completing. Reopening.

[csossi]

Verified in QA