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

BUG Properly handle empty image attributes #474

Conversation

maxime-rainville
Copy link
Contributor

Fixes silverstripe/silverstripe-cms#2489

The regenerate_shortcode handler is used to normalise the data sent to the WYSIWYG. Looks like it was set up to handle boolean attributes like <input required /> by copying empty the name of the attributes to the value when the value is blank. The problem is that this would render out [img title="title"] for empty attributes.

Empty attributes would normally be stripped out anyway ... this is only a problem when the short code has been generated from outside the WYSIWYG.

Image tag don't normally have "boolean" attributes like required, so I don't think we need to worry about those. When I tried manually putting one in, it stop recognising the short code altogether ... so if you did put a boolean attribute in there, it wouldn't ever hit this bit of code anyway,

@michalkleiner
Copy link
Contributor

Could we add the required attribute to the test, too? Just to see that value-less attributes are still handled correctly, whichever way 'correctly' means (strip/leave/turn into empty value string).

@maxime-rainville
Copy link
Contributor Author

When I add [image src="/assets/success.png" id="1" width="128" height="128" class="leftAlone ss-htmleditorfield-file image" title="" alt="" required], it does not get recognised has a shortcode at all ... with or without a PR. So regenerate_shortcode won't get called.

If we want to support boolean attributes in shortcodes, we probably need to fix it in ShortcodeParser first.

@michalkleiner
Copy link
Contributor

That's slightly annoying — thanks for confirming that @maxime-rainville. All good here then.

@michalkleiner michalkleiner merged commit 9548372 into silverstripe:1 Jan 18, 2022
@maxime-rainville maxime-rainville deleted the pulls/1.9/fix-empty-image-attribute branch January 18, 2022 02:39
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

Successfully merging this pull request may close these issues.

WYSIWYG fails to render image shortcode if title parameter is empty
3 participants