Unset properties in style plugins

[bkrmendy]

Problem

The tailwind style plugin can't differentiate between a prop being removed from the inline style prop, and that prop never having been there at all. For example, when a flex gap is removed by dragging it in the negative direction, it goes from positive -> 0 -> being removed from the style prop. When the normalisation step kicks in, there's no gap prop in the inline style, so the tailwind plugin doesn't do anything. To make matters worse, when the gap property is removed while the interaction is in progress, the value coming from the generated tailwind class takes precedence, so the flex gap jumps back to its original size.

Fix: propertiesToUnset + cleanup

The fix for this is to create a piece of state in the editor canvas state, propertiesToUnset, which contains the props to unset in the normalization step, and contain the prop value that needs to be written to the inline style prop during an interaction to simulate that prop being removed (and preventing any other definition for that given prop to take effect).

propertiesToUnset is written to by the DeleteProperties command. Unless the command is in on-complete mode, the command records which properties are to be removed, sets them in propertiesToUnset, and re-applies them with their "zero" value. Since we need to manually define a zero value for each prop that's removed by any of the strategies, propertiesToUnset will be expanded incrementally (right now it only has gap, which is set/unset by the plugin-aware flex gap strategy)

Details

transformJSXComponentAtElementPath throws an error when it doesn't find an element to transform

For example, if an element had gap set to 0px, and then it moved under a new parent, when propertiesToUnset is cleaned up (after the child element is already under the new parent), transformJSXComponentAtElementPath will throw an error, because it can't find the child element at its old element path (which was valid when it was a sibling of the new parent). This manifested itself in breaking tests, where an element that had a property removed and then got reparented seemingly couldn't be reparented.

I removed the exception, because the function is correct without it (if there's no element to transform, we might as well regard it as already transformed).

deleteProperties is used from the inspector, so the normalization step needs to be run after inspector edits too

Concretely, since deleteProperties is used from the inspector too, it set the zero value for gaps too, which had to be cleaned up. I fixed this by creating the runNormalizationStep action and calling it in executeFirstApplicableStrategyForContinuousInteraction. This code could also live in the main dispatch flow, with the tradeoff that there it's harder to find out which elements to normalize. The implementation is a stub for now, it'll be fleshed out in a follow-up PR, when we get to adding tailwind/plugin support to the inspector

[github-actions]

Try me

[relativeci]

#14783 Bundle Size — 57.96MiB (+0.01%).

b9d46eb(current) vs 916e8b9 master#14782(baseline)

Warning

Bundle contains 70 duplicate packages – View duplicate packages

Bundle metrics   2 changes 1 regression

	Current #14783	Baseline #14782
Initial JS	40.95MiB(+0.01%)	40.94MiB
Initial CSS	0B	0B
Cache Invalidation	18.25%	0%
Chunks	20	20
Assets	22	22
Modules	4148	4148
Duplicate Modules	213	213
Duplicate Code	27.34%	27.34%
Packages	477	477
Duplicate Packages	70	70

Bundle size by type   2 changes 1 regression 1 improvement

	Current #14783	Baseline #14782
JS	57.96MiB (+0.01%)	57.95MiB
HTML	7.37KiB (-0.25%)	7.39KiB

Bundle analysis report Branch fix/properties-to-delete Project dashboard

---

^{Generated by RelativeCI Documentation Report issue}

[github-actions]

Try me

(building...)

[github-actions]

Try me

(deploying...)

[github-actions]

Try me

[balazsbajorics]

I have a feeling that runNormalizationStep should be its own distinct step in the editor.tsx outer dispatch flow, similar to the group true up step. but for now, this is good

[Rheeseyb]

I have a feeling that runNormalizationStep should be its own distinct step in the editor.tsx outer dispatch flow, similar to the group true up step. but for now, this is good

Yeah I very strongly feel the same way. This PR introduces an extra field into the EditorState specifically for providing data to this normalisation step, and I'm wondering if actually the solution here should be to store enough data in the editor state to be able to run the normalisation step as part of the dispatch flow. I think this is worth a chat in the morning, because it's better for us to invest time in doing this correctly now rather than accepting what we have "for now"

[Rheeseyb]

As for the comment about transformJSXComponentAtElementPath - we have an internal function transformAtPathOptionally which that calls. It sounds like maybe we should introduce a transformJSXComponentAtElementPathOptionally specifically for the case where it's fine for the transformation to fail silently if the path no longer exists. That being said, I'm not sure I'd agree with "if there's no element to transform, we might as well regard it as already transformed" - doesn't that just mean that we'd fail to unset the property, meaning it would remain on the element?

[bkrmendy]

closing in favor of #6549