Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugs with Image component referencing Page featured image #2741

Open
HitmanInWis opened this issue Apr 30, 2024 · 4 comments
Open

Bugs with Image component referencing Page featured image #2741

HitmanInWis opened this issue Apr 30, 2024 · 4 comments

Comments

@HitmanInWis
Copy link

HitmanInWis commented Apr 30, 2024

Bugs current as of: 2.24.7-SNAPSHOT

  • 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
  • When image compnoent references a page image, "alternative text for accessibility" can be saved as empty
  • 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
  • On the Image policy dialog, a JS error causes "Get alternative text from DAM" selection to be hidden until a different checkbox is clicked
@HitmanInWis
Copy link
Author

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. #2249 was the commit that added the Image editor JS lib to the design dialog (which causes the error), but #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
Copy link
Author

HitmanInWis commented May 31, 2024

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
Copy link
Author

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
Copy link
Author

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:

  • On initial dialog load with "image from page" checked
  • On checking the "image from page" checkbox after opening the dialog with no image specified

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant