verbb / vizy

A flexible visual editor for Craft CMS
Other
43 stars 8 forks source link

Vizy is wiping out other field values #224

Closed brandonkelly closed 1 year ago

brandonkelly commented 1 year ago

Describe the bug

This method:

https://github.com/verbb/vizy/blob/a3ae1d8fdc2a68c17eaa2736a36591ce2d50ef65/src/web/assets/field/src/js/components/VizyInput.vue#L515-L538

is effectively wiping out other field values, by telling the form that their current values are set to the same value they were on the initial page load—which ElementEditor refers to when figuring out which field values it should be submitting.

This is especially pronounced when the autosaveDrafts config setting is disabled, but looks like it could impact autosaves as well, if the timing is just right.

Steps to reproduce

  1. Set autosaveDrafts to false in config/general.php
  2. Create a section/entry type with a Vizy field with a nested Vizy field, as well as some other fields.
  3. Create a new entry for that entry type, and fill in various field values, then add some nested Vizy field blocks to the Vizy field.
  4. Save the field. All other fields will be wiped out.

Craft CMS version

4.4.9

Plugin version

2.1.4

Multi-site?

No response

Additional context

No response

engram-design commented 1 year ago

I can't seem to replicate this, sorry. Here's a video of me reproducing it (with autosaveDrafts => false), unless something is wrong on my end. https://share.cleanshot.com/4vvczwwv

The reason we do this (and for any other Vue-based fields like Hyper and Icon Picker), is that because Vue kicks in after jQuery it'll often mistake the mounted values as updated, and trigger a draft to be saved, because it thinks there's been a change.

In almost all cases, we use Vue's models to store the value of a field internally for components, which is then output with the correct namespace in an <input> element, so it essentially acts like any other field. It's this population of the <input> after jQuery has started and your ElementEditor serialization has happened triggers the unload warning, as your unload function thinks something has changed.

So to get around this, this function essentially just does what your own function does and grabs all the existing data stored in the <form> element's initialSerializedValue data store and re-serializes it. It shouldn't wipe out anything, unless I'm serializing it incorrectly?

brandonkelly commented 1 year ago

Sorry, looks like this is only a bug when the Vizy field is nested within another Vizy field (or theoretically, within a Matrix/Neo/Super Table field). The same JS will be executed each time the field is initialized. I’ve updated the original steps to reproduce to clarify that.

engram-design commented 1 year ago

Ah, gotcha - yes I can reproduce that. But that doesn't really make sense. No matter how many times this is run, it should just be essentially running $('form[data-confirm-unload]').serialize() and storing that to initialSerializedValue.

Is there something I'm missing internally with the ElementEditor and messing around with initialSerializedValue isn't a good idea?

brandonkelly commented 1 year ago

ElementEditor compares the current form values to whatever’s in initialSerializedValue, to determine which fields have been edited since the last save.

By changing initialSerializedValue to the current form’s values, you are effectively telling ElementEditor that nothing in the form has changed at all. So the post data won’t include any field values that were edited before the new Vizy field was added.

engram-design commented 1 year ago

So the requirement to run this code is actually for saved blocks when the field loads. After that, it doesn't need to be run, because it does wipe out un-saved content. This is shown when you add content, then add a nested Vizy field in a Vizy block, it'll wipe out that unsaved content and it doesn't need to be run at all at that point.

Sorry, finally got a grasp of the actual issue 😅

Fixed for the next release. To get this early, run composer require verbb/vizy:"dev-craft-4 as 2.1.4"

brandonkelly commented 1 year ago

Thanks for looking into it!

and for any other Vue-based fields like Hyper and Icon Picker

I haven’t tested, but guessing a similar fix will need to be made for those.

engram-design commented 1 year ago

Shouldn't be a problem for those, as they are only run when the page is loaded before the user has a chance to actually make any changes to content. It's really a unique issue with nested Vizy blocks, which fire this code after content changes have been made first, and then nested Vizy blocks are created.

Thanks for letting me know.

engram-design commented 1 year ago

Fixed in 2.1.5

brandonkelly commented 1 year ago

Thank you sir!