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

Bugs with Image component referencing Page featured image #2741

Open HitmanInWis opened 2 months ago

HitmanInWis commented 2 months ago

Bugs current as of: 2.24.7-SNAPSHOT

HitmanInWis commented 1 month ago

For the bug: "On the Image policy dialog, a JS error causes "Get alternative text from DAM" selection to be hidden until a different checkbox is clicked"

The fix for this one is simple. https://github.com/adobe/aem-core-wcm-components/pull/2249 was the commit that added the Image editor JS lib to the design dialog (which causes the error), but https://github.com/adobe/aem-core-wcm-components/pull/2288 fully reverted the JS changes that triggered the need to add the lib to the design dialog in the first place.

As such, I believe we can simply remove extraClientlibs="[core.wcm.components.image.v3.editor]" from the Image design dialog, resolving this bug.

HitmanInWis commented 1 month ago

For the bug: "When you add a Link to an image component when inheriting featured image from the page, a JS error prevents from saving "Alternative text for acessibility" even if a value is in the field"

The issue is in apps/core/wcm/components/image/v3/image/clientlibs/editor/js/image.js. What's happening is the logic that is added to display a nice error message for missing alt text ("Error: Please provide an asset which has a description that can be used as alt text.") is checking the wrong checkbox when using a page image. When using a page image, if the (hidden) checkbox for "altValueFromDAM" is checked, the validation think you're still trying to default from DAM value and throws an error for that value being empty, regardless of you having entered your own alt text.

The following code:

        validate: function() {
            var seededValue = $(altInputSelector).attr("data-seeded-value");
            var isAltCheckboxChecked = document.querySelector('coral-checkbox[name="./altValueFromDAM"]').checked;
            var isDecorativeChecked = document.querySelector("coral-checkbox[name='./isDecorative']").checked;
            var assetWithoutDescriptionErrorMessage = "Error: Please provide an asset which has a description that can be used as alt text.";
            if (isAltCheckboxChecked && !seededValue && !isDecorativeChecked) {
                return Granite.I18n.get(assetWithoutDescriptionErrorMessage);
            }
        }

Needs to be updated to have isAltCheckboxChecked choose appropriately between altValueFromDAM and altValueFromPageImage based on whether the component is using a page image or not.

HitmanInWis commented 1 month ago

For the bug: "When image component references a page image, "alternative text for accessibility" on dialog loads as empty unless you uncheck/check the "alternative text for accessibilty" box"

The issue is in apps/core/wcm/components/image/v3/image/clientlibs/editor/js/image.js. After the dialog loads and the thumbnail is updated to show the referenced page image, there is logic that hides the checkbox for "get alt text from DAM" (altTuple checkbox in code). When that checkbox is hidden, the ./alt field value is reset to the original value of of the field (i.e. emtpy). Since this text field is the same one used by the altFromPageTuple checkbox, it unintentionally clears the value that should be present for when we are getting the alt text from the page image.

To remedy this, the following code snippet should be updated to re-update the ./alt field after the altTuple checkbox is hidden:

            updateImageThumbnail().then(function() {
                $cqFileUploadEdit = $dialog.find(".cq-FileUpload-edit[trackingelement='edit']");
                if ($cqFileUploadEdit) {
                    fileReference = $cqFileUploadEdit.data("cqFileuploadFilereference");
                    if (fileReference === "") {
                        fileReference = undefined;
                    }
                    if (fileReference) {
                        retrieveDAMInfo(fileReference);
                    } else {
                        altTuple.hideCheckbox(true);
                        altFromPageTuple.update(); // ADD THIS LINE
                        captionTuple.hideCheckbox(true);
                    }
                }
            });
HitmanInWis commented 1 month ago

For the bug: "When image component references a page image, "alternative text for accessibility" can be saved as empty"

There's a couple issues, which I think stem from https://github.com/adobe/aem-core-wcm-components/pull/1995/files which added a check to verify the fileupload field is visible. The "required" value for the alt field is incorrect in two scenarios:

To fix the issue on initial dialog load, we can add a call in updateImageThumbnail to toggle the alt fields in the promise

        // Get the updated page image thumbnail HTML from the server
        return $.ajax({
            url: thumbnailConfigPath + ".html" + thumbnailComponentPath,
            data: {
                "pageLink": linkValue
            }
        }).done(function(data) {
            toggleAlternativeFieldsAndLink(imageFromPageImage, isDecorative); // ADD THIS
            if (data) {

To fix the issue on toggle of the "image from page" checkbox, update togglePageImageInherited

    function togglePageImageInherited(checkbox, isDecorative) {
        if (checkbox) {
            // toggleAlternativeFields(checkbox, isDecorative); REMOVE THIS LINE
            if (checkbox.checked) {
                $cqFileUpload.hide();
                $pageImageThumbnail.show();
            } else {
                $cqFileUpload.show();
                $pageImageThumbnail.hide();
            }
            toggleAlternativeFieldsAndLink(checkbox, isDecorative); // ADD THIS, *AFTER* UPLOAD FIELD IS SHOWING
        }
    }

There may be more ideal/elegant fixes, but this seems to work for me.