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
726 stars 735 forks source link

Teaser Image policies ignored #2458

Open icaraps opened 1 year ago

icaraps commented 1 year ago

Bug Report

Context The aem-site-template-standard is being improved in terms of responsiveness and performance as part of https://github.com/adobe/aem-site-template-standard/pull/100 (notice the PR number is 100 for 100 LH performance :) ).

To improve performance, images are delivered and optimised via Dynamic Media.

Current Behavior

The teaser that is part of the template structure is not delivering the DM optimised image.

Screenshot 2023-03-02 at 15 00 18

So far, I tracked the issue down to the Image v3 component which is looking up the teaser policy instead of the image policy i.e. in https://github.com/adobe/aem-core-wcm-components/blob/main/bundles/core/src/main/java/com/adobe/cq/wcm/core/components/internal/models/v3/ImageImpl.java

currentStyle.getPath() will return the teaser policy /conf/my-site/settings/wcm/policies/core/wcm/components/teaser/v2/teaser/policy_default while I was expected the image policy path /conf/my-site/settings/wcm/policies/core/wcm/components/image/v3/image/policy_default for aem-site-template-standard.

Therefore, a simple workaround to enable DM images for that teaser is to set the specific DM image props inside the teaser policy e.g.

<teaser jcr:primaryType="nt:unstructured">
  <v2 jcr:primaryType="nt:unstructured">
    <teaser jcr:primaryType="nt:unstructured">
      <policy_default
       ...
        enableAssetDelivery="true"
        enableDmFeatures="true"
        sizes="(max-width: 500px) 50vw, 100vw"
        allowedRenditionWidths="[320,480,600,800,1024,1200,1600]">
        ...

Is this workaround acceptable ? It feels a bit wrong especially since the other teasers that are not part of the template structure but added directly in the page are rightfully picking up the image policy.

Screenshot 2023-03-02 at 14 42 47

Any others pointers to resolve the issue would be highly appreciated!

Expected behavior/code

The teaser image which is in the template structure should pick the image policies to enable DM image delivery and optimisation.

Environment

cc @vladbailescu @bpauli

vladbailescu commented 1 year ago

The teaser leverages/embeds the image component to do the rendering by means of the imageDelegate property: https://github.com/adobe/aem-core-wcm-components/blob/main/content/src/content/jcr_root/apps/core/wcm/components/teaser/v2/teaser/.content.xml#L8

The proper fix would be to do the full resolution of content policies when delegating.

rubnig commented 1 year ago

@vladbailescu Why not wrapping it in a (synthetic-) resource that lives below the teaser itself (like you normally do with composite components)?

Example:

If you use this approach, you don't have to add any additional features and it just works with all SlingBindings that could be used, because it is now bound to the right resource type and has all the necessary properties in the image resource itself.

koenkicken commented 1 year ago

@vladbailescu any update on this? I've been encountering the same issues

mmange commented 1 year ago

A workaround for this issue that I experimented with and seems to work is this

  1. Take the properties from Image component's design dialog and copy them into the design dialog of your component that is delegating the image (teaser in this case)
  2. Configure the image related policies of your component - set the lazy loading, responsive sizes properties

That's it, this is the quickest work around that seems to do the trick and not a bad solution as you can configure image related policies for your teaser differently than the default image component using this approach.

rubnig commented 1 year ago

@mmange copying them is a bad idea. That means that if a new property will be added, will change, will be removed in the Image Policy Settings, it is not reflected to the Teaser, which potentially breaks the image again.

Including the settings is also not the preferred way to go forward (core/wcm/components/image/v3/image/cq:design_dialog/content/items/tabs/items/properties/items/content/items), because you also need to include the additional editor client libs etc.

The delegation pattern is just not setup in the right way now. For example by just wrapping the List resource for Teasers, properties like the ID are also automatically reflected from the List to the Teaser. See https://github.com/adobe/aem-core-wcm-components/issues/2506. It just gives additional issues, which can be circumvented by using unique resources, containing only the properties needed with their specific resource type.