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

Don't attach the image resizer to images inside the HTML embed preview #8444

Merged
merged 4 commits into from
Nov 20, 2020

Conversation

psmyrek
Copy link
Contributor

@psmyrek psmyrek commented Nov 10, 2020

Suggested merge commit message (convention)

Fix (image): Don't attach the image resizer to images inside the HTML embed preview. Closes #8433.

@Reinmar Reinmar requested a review from jodator November 16, 2020 12:02
@jodator jodator self-assigned this Nov 19, 2020
Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the trick with matches as we have to somehow detect this case on the view level.

However, I'm worried that this fix will cover only one case of html embed and could fail in other cases:

  • custom image plugin
  • upcoming inline image
  • dunno if media embed could have img inside?

ps.: I feel that I've might miss something so please double check what I've wrote if that makes sense.

@@ -56,6 +56,11 @@ export default class ImageResizeHandles extends Plugin {
editingView.addObserver( ImageLoadObserver );

this.listenTo( editingView.document, 'imageLoaded', ( evt, domEvent ) => {
// The resizer must not be attached to images inside HTML embed preview.
if ( domEvent.target.matches( 'div.raw-html-embed__preview img' ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix will work for html embed preview, but only for that.

On the horizon there's an inline image feature so the check should be specific for the CKEditor's image plugin.

In other words it should check if the target image is the one that we need to attach to not if it is invalid one.

TL;DR: the check should be something like figure.image img (to check).

The inline image would be just an img without a figure, hence the check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good idea to check more than just one unsupported case (the HTML embed image previews) by reversing the logic and allow only known (supported) images to be resized 👍🏻

@psmyrek
Copy link
Contributor Author

psmyrek commented Nov 20, 2020

The media embed can theoretically have an image inside, because it creates an iframe, but it is wrapped by the figure.media rather than figure.image. Anyway, I've proposed stricter rule to match only supported images: figure.image.ck-widget > img.

@psmyrek psmyrek requested a review from jodator November 20, 2020 08:10
Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM 👍

@jodator jodator merged commit f10ee93 into master Nov 20, 2020
@jodator jodator deleted the i/8433 branch November 20, 2020 13:37
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.

The ImageResize plugin crashes if HtmlEmbed plugin has enabled showPreviews option
2 participants