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 lock feature for Reusable Blocks to protect inner blocks #32710

Conversation

thisissandip
Copy link
Contributor

Description

Fixes #32461

Add lock option for the Reusable Blocks in the parent toolbar.
Protects the inner blocks from accidental edits.

Unlock Icon is not available in @wordpress/icons package. So as of now, I have used plain text to display the lock and unlock state in the toolbar.

How has this been tested?

  1. Select a Reusable Block
  2. Try selecting the inner blocks directly
  3. You should not be able to select them
  4. Unlock the block from the toolbar
  5. Select inner block
  6. Lock the block again to prevent accidental changes

Screenshots

LockAndUnlock.mov

Types of changes

New Feature

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@Mamaduka
Copy link
Member

Hi, @thisissandip

Thanks again for working on Reusable Block issues.

I just wanted to mention a general discussion issue on "Locking" - #29864.

@paaljoachim
Copy link
Contributor

paaljoachim commented Jun 16, 2021

Here is a comment made by @cpsyctc
#32461 (comment) we also need to consider.

"Surely if a reusable block has already been used in any other page/post it should be locked and you should be prompted if you try to edit it and warned that any edit will immediately change all other uses of the block? I would expect a dialogue to come up asking if I'm sure I want to edit the reusable block and offering to convert to an ordinary block."

@hrkhal
Copy link

hrkhal commented Jun 16, 2021

@thisissandip While this does prevent accidental edits by clicking into the inner block it will not prevent tabbing into the inner blocks and/or accessing them via the document block list view.

If you also "locked" the block by wrapping it in the <Disabled /> component that would at least prevent tabbing into it even if the accessing via list view still existed. This is what we've had to do internally to prevent users botching reusable blocks since the changes in 5.7.

The discussion in #29864 is promising but this is a good interim solution to prevent accidental edits which have been a substantial annoyance amongst editorial teams.

@thisissandip
Copy link
Contributor Author

thisissandip commented Jun 16, 2021

Using the disabled component is much better than using the z-index.
It disables the direct selection of inner blocks from the list view and also while tabbing through the blocks!
Thanks, @hrkhal ! 😄

lockFeature.mov

@jasmussen
Copy link
Contributor

A great deal of design thought has been put into using click-through for reusable blocks: #29337 — has that been considered in these explorations?

@paaljoachim
Copy link
Contributor

paaljoachim commented Jun 16, 2021

The toolbar will look similar to this example.

Reusable-block-with-lock-in-toolbar

Example from #29864
Screen Shot 2021-06-16 at 13 03 25

@thisissandip
Copy link
Contributor Author

"Surely if a reusable block has already been used in any other page/post it should be locked and you should be prompted if you try to edit it and warned that any edit will immediately change all other uses of the block? I would expect a dialogue to come up asking if I'm sure I want to edit the reusable block and offering to convert to an ordinary block."

Using a dialogue box to come up every time while editing/unlocking the reusable block might be annoying to the users who know what reusable blocks are. I think the locking of inner blocks will indicate to the user that they are editing a reusable block.

Also, while the reusable blocks are saved, users are notified about the changes/warnings in the sidebar.

image

P.S. after PR #32464 is completed. They can even learn more about the reusable blocks using the welcome guide.

@paaljoachim
Copy link
Contributor

Hi @jasmussen Joen.

A great deal of design thought has been put into using click-through for reusable blocks: #29337 — has that been considered in these explorations?

The answer to that is that additional explorations should be done. I am thinking that Addison @Addison-Stavlo will likely have some feedback on this PR.

I have considered my last test of the click through approach here: #31109 (comment)
Here is what I noticed when trying out the Click through approach:
Step 1: Hovering over a Reusable block shows a hover effect. Showing a color overlay on top of the Reusable block.
Step 2: Clicking the Reusable block is selected. A border surrounding the Reusable block is seen.
Step 3: Clicking the blocks inside the Reusable block. The border is still seen.

Transferring above steps over to this lock/unlock PR:
Step 1: Clicking the lock icon (to unlock the inner content). Hovering over the inner content I assume an overlay is seen.
Step 2: A border around the content is also seen.
Step 3: Clicking the blocks inside the Reusable block. The border is still seen.

@jasmussen
Copy link
Contributor

Agreed about the prompt, we don't want to overshoot the target. That's also why I asked about #29337 from February, which was created to solve the exact same problem but with reduced friction. As much as I appreciate the work going on here — at this juncture I can understand wanting some temporary solutions in place for 5.8 — but it'd be good to consult with some designers. Jay is on vacation, but @critterverse or @javierarce might have some thoughts as well. Clickthrough, I believe, is ultimately where we want this to go.

@thisissandip
Copy link
Contributor Author

thisissandip commented Jun 16, 2021

Hey, @paaljoachim!

The Block Icon and Block Title in the parent toolbar is a single component that is used to toggle the block type. It is not possible to add a button between them. This is the reason I added the lock option before the convert to regular blocks button.

Toolbar

image

@hrkhal
Copy link

hrkhal commented Jun 16, 2021

Clickthrough, I believe, is ultimately where we want this to go.

I'm not sure what real world users research was done upon but I can assure you that in an editorial newsroom setting with 100s of users, that clickthrough to inline edit a reusable block is a terrible idea.

For example, an SEO editor makes a topical reusable block surrounding a current news event and the editorial team are asked to use it in all relevant articles, but then someones fudges it or doesn't understand its purpose and edits it inline whether accidental or not. They then hit save/publish as they have always done and that change is then propagated across 100s and 100's of articles. It's reckless.

The counter argument may be that users are notified about the impending reusable block save in the sidebar when they hit save/publish but in reality the editorial teams haven't clocked it or have just continued onwards regardless with saving without being aware that are about to unintentionally 100s of articles. Authors have deadlines and when we're dealing with breaking news speed is of the essence and UI that blends in with the rest of the page furniture just isn't enough to make users aware.

Therefore the locking of reusable blocks safeguards us against these mistakes. Yes you could highlight the severity of what the user is about to do better at the point of saving but the easier way to address this is to prevent the edit in the first place.

Since 5.7 we've had to address these issues ourselves because it was becoming a mess to manage both editorially and via the support requests we were getting put in. We've had to filter the reusable block edit method to wrap it in a disabled component to prevent the bulk of inline edits, added a title bar to reusable block that clearly states it's a reusable block and have additionally written apiFetch middleware to short circuit any PUT requests to the blocks endpoint.

For the most part we've reworked reusable blocks to prevent the majority of our users from being able to create or edit them inline.

In a nutshell, we are unable to trust the integrity of reusable block in their current 5.7 guise when it's far to easy for any user with an edit_posts cap to unintentionally botch 100s, if not 1000s (in some use cases) of articles

@critterverse
Copy link
Contributor

Hey all! Thanks for exploring this lock/unlock functionality. From scanning a number of open issues/existing explorations, I'm getting the sense that:

  • Locking is something that needs to be solved for beyond the scope of the Reusable block
  • There are several open questions about the best way to implement this (with the toolbar icon being one option)

Because of these factors and where we are currently in the release schedule, I'm not sure it's something that's ready for inclusion in 5.8 😬

I agree with @jasmussen that the clickthrough approach in #31109 is a great candidate for introducing some more friction to editing. This approach has been deeply iterated upon by myself and other contributors for several months now so I believe it's a bit more complete in its thinking. In my opinion, the clickthrough certainly doesn't resolve all of the known issues with the Reusable block but it's a big improvement over where we are currently!

@Addison-Stavlo
Copy link
Contributor

I am thinking that Addison @Addison-Stavlo will likely have some feedback on this PR.

👋 Hiya! Its nice to see more explorations on preventing extra-contextual content from being edited by mistake. I think there are definitely some tradeoffs to each approach (lock vs. overlay). I think the overlay has a little less friction and is a bit more visible in the editor. However, if more friction is needed in list-view and keyboard-nav flows a locking system like this would definitely help with that. I would think its best to start on the side of less friction and move this direction if the former isn't enough.

I'm not sure what real world users research was done upon but I can assure you that in an editorial newsroom setting with 100s of users, that clickthrough to inline edit a reusable block is a terrible idea.

@hrkhal - From your description I think you may be misunderstanding what was meant by 'clickthrough' approach (thats understandable, as the name itself kind of implies you can just click through to edit with no safety steps in between). The idea here would be to prevent users from selecting children (content of the reusable block) without first selecting the parent. When the reusable block is hovered in editor or list-view, a semi-transparent colored overlay would appear over its content. Clicking the overlay would then force selection to the reusable block itself instead of its children, unlocking it so that its children could then be selected. (explored in #31109)

@thisissandip
Copy link
Contributor Author

Can someone define the steps to lock and re-lock the blocks in the clickthrough approach?

I will update this PR and then we can test and come to a conclusion that which approach feels robust against accidental edits. Is that okay?

From #29337 example video, The steps to unlock are:

  1. When you hover over the reusable block the block is highlighted with a light overlay
  2. You can select the parent block with a single click
  3. Double click to unlock and access the inner blocks

What are the steps to re-lock the block?

@jasmussen
Copy link
Contributor

jasmussen commented Jun 17, 2021

What are the steps to re-lock the block?

Just deselect all blocks.

Double click to unlock and access the inner blocks

That's not actually right. A single click selects the top level block, the reusable block itself. Now all child blocks are unlocked and any child block can be selected. Double click as described by Jay in 29337 just means "click twice" — once to select the reusable block, once again to select the block inside.

Thank you for looking.

@thisissandip
Copy link
Contributor Author

thisissandip commented Jun 17, 2021

Hey guys! 👋

I have implemented the clickthrough approach for locking and unlocking!

If you guys can take a look 👀 . That'd be great!
Thank you for reviewing and exploring this PR! 😄

clickthrough.mov

@thisissandip
Copy link
Contributor Author

thisissandip commented Jun 17, 2021

Adding my two cents here:

IMO, having a dedicated lock option is more robust and intuitive.

For example: In Notion (which uses a similar block-based editing approach), they have a dedicated lock option to lock the page so that no changes can be made unless one clicks on the dedicated unlock button.

notion
Source

Another example: In Photoshop and Illustrator, there is an option to lock the layer and it cannot be unlocked unless we click the dedicated lock option again.

Imagine while designing something in PS and we accidentally unlock a layer by clicking on it, adding accidental changes.

@cpsyctc
Copy link

cpsyctc commented Jun 17, 2021 via email

@cpsyctc
Copy link

cpsyctc commented Jun 17, 2021 via email

@critterverse
Copy link
Contributor

Nice exploration @thisissandip 👏

I just wanted to flag that we should try to be conscious about creating duplicate PRs here as we already have one that achieves the "clickthrough" functionality for Reusable blocks quite well in #31109 :)

@cpsyctc I'm not sure whether this completely answers your questions but I thought @paaljoachim explained the usefulness of the clickthrough approach really well the other day on Slack: https://wordpress.slack.com/archives/C02RP4X03/p1621618806126700

Here's his comment in case you can't view the link:

Regarding the overlay or as I call it the lid on the box one opens before having access to the contents. The lid can have different flavors:
One can click anywhere on the lid to have it open — ex. clickthrough approach
One can click a specific area on the lid to open the box — ex. an edit or unlock button
One needs a key to open the box — ex. administrator is the only one who can open the box
The above is added to create an extra measure of protection. So the user hopefully makes less accidental changes... as one complaint in regards to Reusable blocks in WP 5.7 is that it was too easy to click a block and accidentally change the inner contents of the Reusable block. Adding the clickthrough approach will hopefully be one small protective measure to avoid accidental changes.

@paaljoachim
Copy link
Contributor

paaljoachim commented Jun 18, 2021

I am adding in a threaded Slack conversation during the Wednesday Core Editor chat: https://wordpress.slack.com/archives/C02QB2JS7/p1623855296191000

@jasmussen
I worry that a lock icon and toolbar action will not be an obvious affordance for editing the contents, or help indicate why this block is any different from the surrounding blocks which you can click and edit. I think some of Jays designs from the previous ticket have potential.

@paaljoachim
Actually one approach is clicking the overlay of a Reusable block and have the lock icon have an overlay as well, or some kind of signal that unlocking/gaining access has to do with the lock.

Carlos Garcia Prim
👋 On the mobile version, we had a similar problem and one solution we came up with was to highlight somehow the lock icon (in our case is the edit button) with an animation effect, here you can see how it looks like in case it helps for the web version:
https://wordpress.slack.com/files/U01HBV9AT37/F025YMK855E/floating_toolbar_shake_animation_-_ios.mp4
(Edit: I noticed the video I pasted came from Slack, so it might not come up.)

@paaljoachim
Copy link
Contributor

paaljoachim commented Jun 19, 2021

Brainstorming.

Here is the newest version of the Click through approach in this PR: #31109 (comment)

1- Hover over a Reusable block and notice the overlay.
2- Click and the (top parent) Reusable block is selected.
3- Second click (in following example) the Heading is clicked into.

Click-through-Reusable-block.mp4

Various blocks that have inner content such as Buttons, Social Icons, Columns, Groups and other blocks could benefit well from using the Click through approach.

  • A kind of basic level 1 protection. Making it easier to select the top parent container. -

  • A kind of intermediate level 2 protection. Focus is on protecting content from accidental changes. Here the user needs to deliberately select to edit the content. -

Reusable blocks are more vulnerable for editing because of the nature of a Reusable block. It is meant to be a block that can be added to multiple pages or posts. Editing any of these instances of the Reusable block will also affect all other instances where it is used. Which means either one creates a stronger protection such as unlocking a lock (ex. in WP 5.6 clicking the Edit button) or one adds a protective measure to when the block is saved. Making it harder forcing the user to make a deliberate choice.

It is better to add a protective measure into the editing state
Here is a suggestion building upon the Click through approach.

1- Hover the block and see the overlay (similar to the Click through approach) + See a lock.
2- Click the overlay. See the toolbar with the lock.
3- Click the lock to unlock the inner content.

Perhaps something like this:

Reusable-blocks-with-click-through-and-a-lock.mp4

Adding a lock says this is something that should not be edited lightly.

@thisissandip
Copy link
Contributor Author

  • A kind of intermediate level 2 protection. The focus is on protecting content from accidental changes. Here the user needs to deliberately select to edit the content.

hey @paaljoachim! 👋

Should I update this PR with the second option ( light overlay with a lock )?

@jasmussen
Copy link
Contributor

I'd love to give this branch a run tomorrow. I don't think we need locks.

@jasmussen
Copy link
Contributor

Thank you again for working on this, @thisissandip. Your patience and work here is much appreciated, it's been inspiring to see.

Here's what I see in your branch:

this

This is impressive work, and I think validates the idea of the overlay color, and the extra click. It also lets us avoid complex accessibility scenarios where you have to tab backwards to get to the lock, then forwards again to get to the block you meant to edit. In the clickthrough scenario, mouse and keyboard are equals.

The only downside is that puts this branch in walking distance of #31109, which does the same but for template parts as well. To Paal's point, we might want to land that first, and then look at what improvements we need to make after that fact.

Do we need further friction? My sincere instinct tells me no: any changes you make aren't applied until you've had a chance to confirm them in the publish dialog, and the emphasis on that dialog might give it the separate attention it needs. By starting small, we can carefully add improvements to safeguard against accidental changes, such as limiting the editing of reusable blocks to certain user roles, perhaps only to the original author or administrators.

If after testing that in the plugin for a bit and we find the need to add an extra click to unlock, we can revisit how to do that without affecting the tab-order, and we can revisit this one. By trying one before the other, we'll better know what improvements we can get into 5.9.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Thank you for the explorations and discussions going on! I'm not sure of what path will be taken on but as promised to @paaljoachim I'm leaving a code review here. Generally from the code point of view things seem good 👍 I just left some logic questions.


const currentBlockId = getSelectedBlockClientId();
const parents = getBlockParents( currentBlockId );
const _firstParentClientId = parents[ parents.length - 1 ];
Copy link
Member

Choose a reason for hiding this comment

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

I guess if the reusable block is used inside a columns block for example this logic would not work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it does work when reusable blocks are inside columns.


// lock the blocks when deselected
useEffect( () => {
const isInnerBlock = parentBlockName === 'core/block'; // first check if selectedblock is inner block
Copy link
Member

Choose a reason for hiding this comment

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

We check if the block is inner block by block type and not by verifying if the specific reusable block id is the parent of the deselected block, if we have multiple reusable blocks on the page will this logic handle that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for taking a look 😄

Yes, it does work when multiple reusable blocks are on the page because first we have to select the reusable block itself, and only after that, we can select the inner blocks.

So, when multiple reusable blocks are on the page, after working on the first reusable block, we go and select the other one. Now, first, we have to select the outer block (reusable block itself) which won't have a parentBlockName of core/block thus it will lock the reusable blocks. The same logic follows in the case of the reusable blocks inside columns.

The bug I came across while testing this was when a reusable block is selected and its inner blocks are not selected right after that but another reusable block is selected.
In such a scenario, the previously selected reusable block remains unlocked.

I am adding a commit to fix this bug. Once again, thank you for reviewing it! 🙏

@paaljoachim
Copy link
Contributor

@thisissandip Sandip it would be great to use the learning from this PR in creating a click through approach and then look at @Addison-Stavlo 's PR and do a code review etc there. As we should first get that PR merged. Test it out for a while and then see how your PR here can be brought forward.

@thisissandip
Copy link
Contributor Author

@paaljoachim Yeah sure! 😄

@annezazu annezazu added the [Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) label Jul 28, 2021
@ms-hetty
Copy link

Hi everybody.

After using Reusable Blocks for a very long time on many projects. Today, I got finally so annoyed by editing them accidently, that I looked for a solution to lock them from accidental editing while editing another post. I'm using them over 2 years daily, and I want to give you the feedback, the implemented solution in the PR 31109. is not sufficient to stop editing them. Until I've read it here, I even didn't understand, that the clickthrough approach was implemented in 11.0/5.8.2. My thought process from an editor perspective is more like that the first click "didn't work".

I strongly want to suggest looking again into the dedicated locking feature with a button in the toolbar for Reusable Blocks. I would help with it, but I'm not good enough in JavaScript to actually understand the Gutenberg code.

Thank you!

@paaljoachim
Copy link
Contributor

Hi @ms-hetty Stefanie

I look forward to having this PR continued. At the moment I believe there is still some discussion needed in this issue:
On Locking and TemplateLocking
#29864
That can also help move this PR onward.

Happy New Year!

@Mamaduka
Copy link
Member

Closing now that #39950 is merged.

@Mamaduka Mamaduka closed this Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Synced Patterns Related to synced patterns (formerly reusable blocks)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reusable block: Add a lock to the parent toolbar to protect the inner content from accidental editing.