Full Site Editing: Hovering block padding and margin inputs are registered as changes

[zaguiini]

Description

Putting the mouse cursor on top of some block attributes inputs (e.g. padding, margin) enables the "Save" button on the top right, even though nothing changed.

This behavior exists because of the cover block padding and margin visualizer, introduced on #23041.

Note that we do want to keep the visualizer showing up when hovering over the input. What we don't want is to enable the "Save" button when this happens.

A possible solution would be to introduce a new piece of state that would host the visualizer settings itself, not polluting the changes object and not triggering the false alarm.

Step-by-step reproduction instructions

1. Enable the full site editor locally;
2. Navigate to full site editor;
3. Select any block that has spatial settings and hover the padding or margin inputs;
4. Check that the "Save" button is enabled.

Screenshots, screen recording, code snippet

Screen.Recording.2021-11-11.at.17.07.48.mov

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

[creativecoder]

FYI @apeatling and friends. I think you may have been working on these kind of controls, recently.

[ramonjd]

I can confirm this happens in the Post Editor as well

Nov-18-2021.09-05-55.mp4

[ramonjd]

I think it's related to the hover callbacks on <BoxControl />.

			onHoverOn={ handleOnHoverOn }
			onHoverOff={ handleOnHoverOff }

After commenting them out I can no longer reproduce the issue.

You can see the effect for margin and padding support controls, but not spacing, which uses <UnitControl />

Edit: The hover callbacks trigger the onChangeShowVisualizer prop. If we don't pass down this prop to <BoxControl /> the effect also doesn't materialize.

[ramonjd]

The hover callbacks trigger the onChangeShowVisualizer prop. If we don't pass down this prop to the effect also doesn't materialize.

I've thrown up a PR just in case doing this is okay (for now). I can't see any side effects so far – I see no usage of Visualizer, but I'm not an expert. cc @aaronrobertshaw for advice.

[aaronrobertshaw]

Other than the spacing block supports, the Cover block also utilizes the box control's Visualizer.

I believe other blocks have already had their visualizers removed in favour of waiting for the general approach to visualizers to be updated. The plan there was to explore using popovers or something along those lines as visualization of margins is problematic at the moment and generally forced a disjoint between the markup in the editor vs frontend.

It's also been highlighted that in the current state of the Cover block, the visualizer is hidden underneath any applied overlay. With all that said, I wouldn't be against temporarily removing all visualizers. Others with a clearer view of the bigger picture might see it differently though.

@youknowriad do you have any thoughts on this? You've advised me previously on the plans for visualizers.

An alternative to removal might be to pursue the approach taken in #33560.

[youknowriad]

I think the main issue here is that the visualizer relies on block attributes to keep its state which should be just local state. I believe @mcsf also had identified this and may have had solutions proposed somewhere.

[aaronrobertshaw]

I think the main issue here is that the visualizer relies on block attributes to keep its state which should be just local state. I believe @mcsf also had identified this and may have had solutions proposed somewhere.

Thanks, agreed. I should have added further detail to my comment, sorry.

The use of block attributes has been discussed as the cause of this issue on @ramonjd's PR: #36587 (comment). It also causes a separate issue (#31839) where that visualizer data is persisted within the block attributes on save.

The alternative approach taken in #33560 attempts to move this visualizer data into component state. It might require some additional tweaks though. So I think the question is, should we push ahead with @ajlende's efforts in #33560 or remove the visualizers given their very limited and slightly buggy use?

I believe @mcsf also had identified this and may have had solutions proposed somewhere.

I took a look around open PRs but didn't spot anything. I'm happy to help out with whichever approach we decide on.

[youknowriad]

For me visualizers are very important in terms of UX and we should look at ways to increase their usage in our code base.

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.

[ramonjd]

So I think the question is, should we push ahead with @ajlende's efforts in #33560 or remove the visualizers given their very limited and slightly buggy use?

I'd lean towards flipping them to "off" until we get #33560 or whichever other solution across the line.

It's a small and inexpensive change to remove incongruent behaviour in the editor.

Moreover, if folks start using the Visualizer in its current state (beyond the current usage in Cover), then it might make implementing a potential, future solution more difficult.

For me visualizers are very important in terms of UX and we should look at ways to increase their usage in our code base.

💯

We could bump the priority of this work, review #33560 and so on... What do folks think?