adobe / aem-core-wcm-components

Standardized components to build websites with AEM.
https://docs.adobe.com/content/help/en/experience-manager-core-components/using/introduction.html
Apache License 2.0
733 stars 740 forks source link

Default values for fields in image component dialog are not considered after drag&drop of asset on image component #2556

Open henrykuijpers opened 1 year ago

henrykuijpers commented 1 year ago

Bug Report

Current Behavior

  1. Configure the "Dynamic Media" functionality (namely the feature flag should be turned on to let the dialog fields render)
  2. Drag&drop an image component (v2, v3, ...) on the page
  3. Open the dialog, notice that the default values for the radiogroup field is taken into consideration
  4. Close the dialog without saving
  5. Drag&drop an asset on the image component (which triggers the cq:dropTargets functionality, which fills in the fileReference property)
  6. Open the dialog again
  7. Notice that the default values for the radiogroup field is not taken into consideration anymore

Expected behavior/code The default values for the dialog fields should be considered at all times: Those values are there for a reason of course.

Environment

Possible Solution Remove the "freshness" feature (or at least remove the 5msec "rule").

Additional context / Screenshots The problem appears to be in the "freshness"-feature of the TouchUI: The default mode is "CHECK_FRESHNESS", which does the following: It calculates the difference between the jcr:created value vs the jcr:lastModified value. If that difference is 5msec or less, then the values are considered "fresh" and the default values will be filled in. Otherwise, the default values will not be used and all fields will not show a default value (and thus show "no value", which of course is wrong).

It appears that a workaround is already in place in /libs/cq/gui/components/authoring/dialog/dialog.jsp for the "forms" feature:

        // for forms, we ignore freshness
        if(StringUtils.startsWith(dataPath, "/content/forms/af/")) {
            FormData.push(slingRequest, data.getValueMap(), NameNotFoundMode.IGNORE_FRESHNESS);
        } else {
            FormData.push(slingRequest, data.getValueMap(), NameNotFoundMode.CHECK_FRESHNESS);
        }

It also appears that it's impossible to change this behavior, the only component where this behavior can be changed is the "select" coral component, all the other components don't have that "forceIgnoreFreshness"-property that the "select"-component supports.

henrykuijpers commented 1 year ago

This issue goes even further:

There are 3 cases where this can be an issue:

Case 1: SlingPostProcessor

  1. Create a SlingPostProcessor implementation that runs for more than 5ms (i.e. with a simple sleep of 5ms, but, of course, this should be related to the processing that you need to do that can take a variable amount of time)
  2. The jcr:created-property will be put before the postprocessor, the jcr:lastModified will be put after the postprocessor has finished
  3. The diff is more than 5ms; The dialog will not be considered "fresh".

Case 2: Drag&drop of asset on component

  1. Add an image/v2 component to the page
  2. Drag&drop an asset onto the image/v2 component
  3. The jcr:lastModified-property will have a diff much bigger than 5ms
  4. The dialog will not be considered "fresh".

Case 3: Existing component gets a new field with a default value

  1. Have hundreds of existing components that have been modified etc etc
  2. As a developer, add a new field with a default value
  3. Open the dialog of one of those existing instances of those components
  4. The dialog will not be considered "fresh", thus, the new field will not have its default value applied

There is a "fake" defaults thing going on: When saving a "fresh" dialog, actually, the field's default values will be saved into the repository, making it seem like the default values are applied.

This whole thing is a serious flaw in the AEM product and should be fixed.

henrykuijpers commented 1 year ago

Hi @gabrielwalt , @vladbailescu , @bpauli , @raducotescu , @richardhand , @jckautzmann ,

Sorry to tag you guys, but requesting your attention to this issue, as this is a serious flaw in how TouchUI (and ClassicUI) are working at this moment. Also, this is not an issue that can be fixed with the Core Components, it requires a fix in the AEM product.

If we are wrong, considering the explanation provided, we'd love to hear this too. :)