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
724 stars 737 forks source link

[Teaser V2] Invalid HTML if teaser has link + image but no Title #2660

Open ky940819 opened 5 months ago

ky940819 commented 5 months ago

Bug Report

Current Behavior When a teaser has:

  1. The Link field set
  2. An image set
  3. Blank title

then the resulting HTML is invalid because of nested anchor tags - which is prohibited.

Expected behavior/code The component should not create HTML that is invalid.

Environment

Possible Solution The issue stems from the v2/TeaserImpl.java model initImage() method:

    protected void initImage() {
        ...
        if (StringUtils.isNotEmpty(getTitle()) || getTeaserActions().size() > 0) {
            overriddenImageResourceProperties.put(Teaser.PN_IMAGE_LINK_HIDDEN, Boolean.TRUE.toString());
        }
        ...
    }

We see here that the link on the image is suppressed if there is a non-empty title or there are teaser actions. However, if we look at the component html teaser/v2/teaser/teaser.html we see the following:

    <a class="cmp-teaser__link"
       data-sly-attribute="${teaser.link.htmlAttributes}"
       data-sly-unwrap="${!teaser.link.valid || !teaser.actions.empty}"
       data-cmp-clickable="${teaser.data ? true : false}">
        <div class="cmp-teaser__content">
            <sly data-sly-call="${pretitleTemplate.pretitle @ teaser=teaser}"></sly>
            <sly data-sly-call="${titleTemplate.title @ teaser=teaser}"></sly>
            <sly data-sly-call="${descriptionTemplate.description @ teaser=teaser}"></sly>
            <sly data-sly-call="${actionsTemplate.actions @ teaser=teaser}"></sly>
        </div>
        <sly data-sly-call="${imageTemplate.image @ teaser=teaser}"></sly>
    </a>

Here we see that the outer anchor is unwrapped (removed) if the link is invalid or there are CTA links.

The problem is that the logic for if the image should be linked, and the logic for if the entire Teaser should be linked are not in agreement. This can lead to situations where both the Teaser and the Image are linked - which causes broken HTML because links are not allowed inside of links.

The solution to this is to make sure that the case for when an image is linked and when the teaser is linked are mutually exclusive. I'm not entirely sure what the blankness of the title has to do with if the image should be linked or not, removing that condition from the V2 model implementation would probably resolve the issue.

A more sustainable approach, that would require a change to the model interface, would be moving the logic for the data-sly-unwrap into a method called something like boolean getLinkTeaser() and then the HTL becomes data-sly-unwrap="${!teaser.linkTeaser}" and then this method can be re-used inside models initImage() method as such:

        if (this.getLinkTeaser()) {
            overriddenImageResourceProperties.put(Teaser.PN_IMAGE_LINK_HIDDEN, Boolean.TRUE.toString());
        }

or, better yet

        overriddenImageResourceProperties.put(Teaser.PN_IMAGE_LINK_HIDDEN, Boolean.valueOf(this.getLinkTeaser()).toString());