Use state for dimensions visualizers

[ajlende]

Description

Fixes #31839 by using component state instead of block attributes for storing the visualizer information.

Props added are undocumented and marked as __experimental because the box visualizer is also marked as __experimental. I expect that when the visualizer API is finalized, we can finalize these props and document them as something that the hook provides for components that wish to use them.

How has this been tested?

1. Add cover block
2. Add padding
3. See that attributes are not updated when hovering the padding controls, but UI still shows.
4. The "Save draft" button shouldn't be affected and an undo state shouldn't be added either since attributes aren't updated.

Screenshots

2021-07-19.15-52-13.mp4

Types of changes

Bug fix

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.
• I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

[aaronrobertshaw]

Nice work @ajlende! 👍

This tests great for me as per the PR instructions and solves #31839

✅ Cover block visualizer still displays correctly
✅ Visualizer properties aren't saved within the block's attributes
✅ "Save draft" and undo state function as advertised
✅ Works across multiple blocks and dimensions controls
✅ Individual sides of the visualizer still display when BoxControl is unlinked/linked

There was some recent discussion about possibly overhauling how all visualizers work.

Before approving this PR, it would be great to see how this fits in with that bigger picture. @youknowriad do you have any thoughts on this?

If there aren't any imminent changes to the visualizer API, I don't see why we couldn't land this now.

Thanks again for taking care of this one!

[ntsekouras]

👋 - thanks for your work here! I understand the problem we have and your PR solves it, but it seems many changes are needed and required changes to block's edit function as well (__experimentalStyleVisualizer).

I was wondering if the alternative I propose in the comment could be considered and I'm not sure if it is too hacky 😄 . This way we could add BoxControlVisualizer to other blocks that use padding support without any other change needed.

What do you think?

[ntsekouras]

Could we just use __unstableMarkNextChangeAsNotPersistent here and __unstableMarkLastChangeAsPersistent before the setAttributes at the onChange function? The second action is because of this: https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/store/reducer.js#L497

All changes are considered persistent except when updating the same block attribute as in the previous action.

[ajlende]

Could we just use __unstableMarkNextChangeAsNotPersistent here and __unstableMarkLastChangeAsPersistent before the setAttributes at the onChange function?

I wasn't aware of these, but after skimming through the code, if I understand it correctly, it looks like they are basically just a complicated way to toggle between triggering the onInput versus onChange props for the BlockEditor. It doesn't really solve the root-cause of this issue—using attributes to store editor-specific data.

I was wondering if the alternative I propose in the comment could be considered and I'm not sure if it is too hacky 😄 .

Using attributes for anything other than properties needed to render the final block is what I think is the hacky part. As long as we're using attributes for editor data, there are always going to be opportunities for bugs like this one to crop up.

This way we could add BoxControlVisualizer to other blocks that use padding support without any other change needed.

The ideal case would be incorporating BoxControlVisualizer directly into the hook so only the block.json needs to be updated for the support. Maybe someone has an idea for how to do that in the overhaul that @aaronrobertshaw mentioned. The changes for blocks using the hook are pretty minimal with this approach, but you're right, they aren't nothing.

The implementation would be simpler for hooks that don't have nested properties. In that simple case, you could just pass setVisualizers from useState directly to __experimentalSetVisualizers, and that could be documented with BoxControlVisualizer.

[ntsekouras]

The ideal case would be incorporating BoxControlVisualizer directly into the hook so only the block.json needs to be updated for the support.

Exactly!

Also I wasn't able to reproduce adding extra props in style besides the padding ones. Do I have to do something specific?

[ajlende]

Also I wasn't able to reproduce adding extra props in style besides the padding ones. Do I have to do something specific?

Do you mean attributes.style? If so, #31839, has a screen recording and the steps to reproduce the issue.

[aaronrobertshaw]

The ideal case would be incorporating BoxControlVisualizer directly into the hook so only the block.json needs to be updated for the support. Maybe someone has an idea for how to do that in the overhaul that @aaronrobertshaw mentioned.

This might not be so straightforward when the differences between blocks (and their markup) are taken into account. More complicated again, when both padding and margin wish to be visualized.

With the current visualizer, displaying margins is particularly problematic and can require adding a wrapping element which creates a disjoint between the markup in the editor and the frontend.

One previous suggestion on how the visualizer could work was making it not rely on a parent element and instead maybe use a ref/popover.

For the moment, I'd think being able to handle the visualization on a block by block basis would be best.

[ramonjd]

Thanks for working on this @ajlende

I've also tested according to the instructions ✅

I fooled around with adding a margin visualizer as well and it worked functionally as expected as well in the post editor.

I can confirm that it also addresses the issue: #36418

I can't comment with any real authority on the bigger picture. @youknowriad mentioned the following:

The reason they're not used more extensively is because the way they're implemented, they forces you to have extra DOM wrappers in a lot blocks if you want to add them. I'd love personally if we solve this and increase their usage either by using a "Popover" to render them or by using :before or :after styles or any technique that don't mess too much with the DOM tree.

By extracting the visualizer state from the block attributes, I think this PR makes a worthy change on which we could iterate with the above goals in mind.

What do others think?

Also I had to rebase on trunk to test. If you don't feel like doing that here's the patch after rebase.

[ndiego]

@ajlende just checking in on this PR. I was just able to test the original issue and confirm it still exists.

[Mamaduka]

The original issue was fixed via #40505.