enzedonline / quill-blot-formatter2

A module for Quill that allows image and iframe embeds to be resized, repositioned, etc. Support for image alt/title/caption editing and relative sizing. Compress embedded images.
Apache License 2.0
9 stars 4 forks source link

iFrame video not resizeable? #3

Closed bubbleheadinc closed 2 months ago

bubbleheadinc commented 3 months ago

Hi, thanks for your work on this lib!

Image resizing is working great, but iframe videos don't. Should videos be able to be resized? I see data-blot-formatter-unclickable-bound="true" is set on the iframe. Is that to prevent play so it can be resized or is that to prevent resize? Am I missing something obvious?

Screenshot 2024-08-03 at 11 19 23 AM

Thanks for your time!

enzedonline commented 3 months ago

Hi. Are you using the v2.1.2 code from the repository or the published version (v2.1.1)? I didn't quite get to finishing testing the v2.1.2 code yet (which is why it isn't published yet). v2.1.1 should resize iframes fine (Code Pen).

There is a change in 2.1.2 that dismisses the overlay if you click outside of the editor, but I missed the fact that the thumbnail overlay for iframes is created at the end of the doc body - this is what gets clicked on when you click the video embed. I'll update it first thing Monday morning before I publish the new version.

The data-blot-formatter-unclickable-bound="true" is just for handling the thumbnail click events.

I'll let you know here when it's up.

enzedonline commented 3 months ago

I've updated the repository with the beta fix - I just need to do a complete round of testing before it's published. There's a small tweak to the css included that I also picked up, taking fit-content out of iframe-align-center - some YouTube videos weren't resizing properly with this (it's only needed for the inline image format).

bubbleheadinc commented 3 months ago

@enzedonline Hey thanks for the quick reply and details. I'm using the npm version 2.1.1.

Ok, so yes it does work as you showed in your CodePen. I was able to get it to work in my app, except for when I render the editor in a fixed position modal. It looks like the dynamically created element with blot-formatter__proxy-image at the bottom of the body is what does the resize overlay trick, but it is below the fixed element. So I added a high z-index to the class and now it works! Maybe it would be helpful to add that to the docs and/or CSS.

Also, I noticed that those blot-formatter__proxy-image elements aren't removed from the DOM and accumulate as I render, destroy, render the editor:

Screenshot 2024-08-04 at 12 25 51 PM

One more question...is there a way to have the overlay track the img or vid element on scroll? As the my content is scrolled in the editor window, the overlay stays in place as the content moves:

https://github.com/user-attachments/assets/54b7b86b-0393-4a78-ade3-9842df6fa4b0

Thanks again for the great lib and help!

enzedonline commented 3 months ago

Good points! I've included those in the new version.

  1. I've added a note to the readme re the proxy image - I didn't include it in the css as it has the potential to interfere with other elements, particularly with toolbars (it needs a lower z-index than the toolbar in case the proxy image scrolls up, or it needs the quill container to have overflow hidden). I've left it as a conscious design decision.
  2. I've changed the proxy image logic to only create an image if one doesn't already exist. In your case, it will leave one proxy image once the modal is dismissed but no more. I looked at removing if the editor is destroyed - too much potential for breakage and it means expanding the scope to introduce a document mutation observer. Better to keep it simple. If you need the image gone completely you can add a call to your modal dismissal to remove any 'img.blot-formatter__proxy-image' remaining.
  3. I could replicate your overlay issue by making the quill root scrollable with a max-height. I've added listeners for scroll and resize on the quill.root element (.ql-editor) which will reposition the overlay in response. I couldn't see which was your scrolling element in the vid - in case it's not the quill.root, you can add your own scroll listener and call repositionOverlay() on the blot formatter instance. The overlay sits in the quill.container element - I'd recommend setting overlay: hidden; on that one to ensure the overlay isn't displayed beyond the editor.

That's all published in 2.1.2 now.

https://github.com/user-attachments/assets/b0c39080-e7a9-4b34-9376-4f6126aaebdf

bubbleheadinc commented 3 months ago

@enzedonline

Awesome!

All those changes make sense and work for me, except for one wrinkle...

I'm using a blockEmbed extension to wrap iFrames in two divs in order to make them responsive. But it looks like your script may be looking for iFrames that are direct children of the editor root, so the iFrames are never detected and the blot-formatter__proxy-image is never is able to be positioned. Is there any way around that?

Screenshot 2024-08-04 at 11 01 21 PM

Thanks again!

enzedonline commented 3 months ago

Hmmm, I can't find anything in the code that expects the iframe to be a direct child of the editor. The overlay is positioned relative to the quill container:

    const parent: HTMLElement = this.quill.root.parentNode;
    const specRect = overlayTarget.getBoundingClientRect();
    const parentRect = parent.getBoundingClientRect();

    Object.assign(this.overlay.style, {
      display: 'block',
      left: `${specRect.left - parentRect.left - 1 + parent.scrollLeft}px`,
      top: `${specRect.top - parentRect.top + parent.scrollTop}px`,
      width: `${specRect.width}px`,
      height: `${specRect.height}px`,
    });

The proxy is repositioned whenever the mouse hovers over an iframe. The mouse click event that activates the overlay is dependent on the clicked target being an image:

  onClick = (event: MouseEvent) => {
    const el = event.target;
    if (!(el instanceof HTMLElement) || el.tagName !== 'IMG') {
      return;
    }

    this.img = el;
    this.formatter.show(this);
  };

If you add a visible border to the proxy image, do you see it align with the iframe on mouse hover?

enzedonline commented 3 months ago

I think this is it (I haven't noticed this before, it's from the original code).

export default class IframeVideoSpec extends UnclickableBlotSpec {
  constructor(formatter: BlotFormatter) {
    super(formatter, 'iframe.ql-video');
  }
}

It causes the IframeVideoSpec to look for elements matching 'iframe.ql-video' and sets the mouseenter event listeners on elements that match that. https://github.com/enzedonline/quill-blot-formatter2/blob/cf4587a27154a1299ac4b08c275f3bd0b9a4a726/src/specs/UnclickableBlotSpec.ts#L94C1-L101C5

onTextChange = () => {
    Array.from(document.querySelectorAll(this.selector))
      .filter((element): element is HTMLElement => !(element.hasAttribute(MOUSE_ENTER_ATTRIBUTE)))
      .forEach((unclickable) => {
        unclickable.setAttribute(MOUSE_ENTER_ATTRIBUTE, 'true');
        unclickable.addEventListener('mouseenter', this.onMouseEnter);
      });
  };

Since your iframe is missing the ql-video class it won't match.

You can either add the ql-video class to your blockEmbed, or extend UnclickableBlotSpec into a custom class with a suitable selector and include that with the ImageSpec in your options as per docs

Something like:

import BlotFormatter2, { ImageSpec, UnclickableBlotSpec } from 'quill-blot-formatter2'

Quill.register('modules/blotFormatter2', BlotFormatter2);

class CustomIframeSpec extends UnclickableBlotSpec {
  constructor(formatter: BlotFormatter) {
    super(formatter, 'div.ql-editor iframe');
  }
}

const quill = new Quill(..., {
  modules: {
    ...
    blotFormatter2: {
      image: {
        registerImageTitleBlot: true
      },
      specs: [
        ImageSpec,
        CustomIframeSpec 
      ],
    }
  }
});

^ I haven't tested that but I'm 90% sure it's right 😉

I'll be changing that document.querySelectorAll to quill.root.querySelectorAll next update, shouldn't be searching the whole document.

bubbleheadinc commented 3 months ago

@enzedonline thanks for digging into it!

I've tried both methods: adding ql-video to the iframe element and extending with the custom class. Both now trigger the overlay placement, but resizing doesn't trigger. On the body I can see the style for the cursor being added on handle click. But drag doesn't seem to be firing. I peeked at the code, but don't see why.

Maybe it's returning early? const target = this.formatter.currentSpec.getTargetElement(); if (!target) { return; }

Surely others would have this issue as responsive iframes are pretty important these days. I don't know any other way to make an iframe responsive other than wrap with other controlling elements.

Just a quick other suggestion that I noticed, currently you can't scroll if the cursor is on top of the overlay. Maybe a way to watch scroll and add pointer-events: none; while scrolling to allow the element beneath to scroll?

Thanks again!

enzedonline commented 3 months ago

Just a quick other suggestion that I noticed, currently you can't scroll if the cursor is on top of the overlay. Maybe a way to watch scroll and add pointer-events: none; while scrolling to allow the element beneath to scroll?

The more I dig into this, the more I see that this package wasn't designed with scrollable editors in mind. The proxy was also blocking scroll and not updating position after scroll so that it ended up in the wrong position. Then it could also scroll beyond the end of the document causing the window to start scrolling. This also made me see that the proxy is overlaying areas outside of the editor.

The only way to prevent that was to move the proxy to the quill container then rewrite the positioning formula (did my head in) then listen for wheel events and pass that into the quill root. Scroll doesn't help as it's not fired if the window/ancestor is not in overflow. Can't use pointer events as this enables click events on the iframe. I have my doubts about the necessity of the proxy image, but that's for another time.

I can't replicate your resize issue though and can't see what would cause it. getTargetElement() would only return null if the overlay had been deactivated.

When you say responsive, what is it you want to do? Different behaviour for different screen widths? I'm not sure what you need that can't be addressed with media queries. Maybe send me your custom format, I can try to replicate it and see what's happening.

bubbleheadinc commented 3 months ago

@enzedonline

Gotcha. Yeah I find it odd that an editor wouldn't be considering scroll as a common setup. I'd say most would need to scroll.

As far as my resize issue, here is the custom class (along with your class from above to target the iframe differently) I am using to make iframes responsive along with then necessary CSS. This is how most would make iframes responsive where they resize fluidly with their parent like an image with max-width: 100% without any need for media queries, which are cumbersome.

The iframe is selectable and the overlay finds it, but something is not allowing it to drag and resize with the handles. See two videos below with and without the VideoResponsive Class.

Custom class:

class CustomIframeSpec extends UnclickableBlotSpec {
  constructor(formatter) {
    super(formatter, "div.ql-editor iframe");
  }
}

const BlockEmbed = Quill.import("blots/block/embed");
const Link = Quill.import("formats/link");

class VideoResponsive extends BlockEmbed {
  static blotName = "video";
  static tagName = "div";

  static create(value) {
    const node = super.create(value);
    node.classList.add("ql-video-wrapper");

    const innerChild = document.createElement("div");
    innerChild.classList.add("ql-video-inner");
    node.appendChild(innerChild);

    const child = document.createElement("iframe");
    child.setAttribute("frameborder", "0");
    child.setAttribute("allowfullscreen", true);
    child.setAttribute("src", this.sanitize(value));
    innerChild.appendChild(child);

    return node;
  }

  static sanitize(url) {
    return Link.sanitize(url);
  }

  static value(domNode) {
    const iframe = domNode.querySelector("iframe");
    return iframe ? iframe.getAttribute("src") : "";
  }
}

Quill.register("formats/video", VideoResponsive);

CSS:

.ql-video-wrapper {
  width: 100%;
  margin: auto;
}

.ql-video-inner {
  padding-top: 56.25%;
  position: relative;
  height: 0;
}

.ql-video-inner iframe,
.ql-video-inner object,
.ql-video-inner embed {
  position: absolute;
  top: 0;
  left: 0;
  width: 100%;
  height: 100%;
}

Without VideoResponsive class https://github.com/user-attachments/assets/3003ae89-a415-4c3b-ae2b-13856fba6dc5

With VideoResponsive class that wraps the iframe to apply responsive classes https://github.com/user-attachments/assets/0af8a728-2b36-417a-a874-e02049cdd394

Yet again, thanks for all the help!

enzedonline commented 3 months ago

This is what's preventing the resize:

.ql-video-inner iframe,
.ql-video-inner object,
.ql-video-inner embed {
  position: absolute;
  top: 0;
  left: 0;
  width: 100%;
  height: 100%;
}

You're setting the iframe width & height to 100% in css so the height/width element attributes have no effect.

I could achieve the same behaviour with this though without needing wrappers. It assumes you want aspect ratio 16:9 (YouTube default):

const VideoEmbed = Quill.import("formats/video");

class VideoResponsive extends VideoEmbed {
    static create(value) {
        const node = super.create(value);
        node.setAttribute('width', '100%');
        node.style.aspectRatio = "16/9 auto";
        return node;
    }
}

Quill.register(VideoResponsive);

https://github.com/user-attachments/assets/55470cc3-6b24-4f70-a732-65ae058c1413

(My wifi is snail pace ...)

If you want the width to remain relative to the container after resizing instead of px, you could extend your video BlotSpec.onHide() - delete the height attribute as set the width attribute as a percentage of the current quill.root width if it has a px value.

This also allows the alignments to work with the iframe - Quill block formats expect them to be immediate children of the Quill root. It was being ignored as a nested element. Failing that, you'd need to register your own iframeAlign class that deals with the nested structure.

bubbleheadinc commented 3 months ago

@enzedonline Thanks for the detailed info and ideas.

Using your suggestion with onHide(), this is what I'm doing and it seems to be working well and no longer needs the wrapping divs and CSS:

class CustomIframeSpec extends UnclickableBlotSpec {
  constructor(formatter) {
    super(formatter, ".ql-editor iframe");
  }
  onHide() {
    if (this.unclickable) {
      // remove height attribute so iframe is responsive
      this.unclickable.removeAttribute("height");
    }
  }
}

const VideoEmbed = Quill.import("formats/video");
class VideoResponsive extends VideoEmbed {
  static create(value) {
    const node = super.create(value);
    node.setAttribute("width", "100%");
    node.style.aspectRatio = "16/9 auto";
    return node;
  }
}
enzedonline commented 3 months ago

Great, glad that's working now.

You should really call super in your onHide() as this method takes care of hiding the proxy image and clearing out the unclickable variables.

When I get the v2.1.3 out this week there are a couple of enhancements you can take advantage of - the unclickable selector can be set in the options, the height attribute won't be set if there's an aspect ratio specified (either via style or css) and the responsive class is included in the package and can be registered via options. This would probably eliminate the need for your custom code there.

The onHide() change I was suggesting was about keeping width relative toquill.root width instead of fixed pixel. You could do that with something like:

editorStyle = getComputedStyle(this.formatter.quill.root);
editorWidth = this.formatter.quill.root.clientWidth - 
              parseFloat(editorStyle.paddingLeft) -parseFloat(editorStyle.paddingRight);
this.unclickable.width=`${100 * this.unclickable.clientWidth / editorWidth | 0}%`

Maybe I should just add relative resizing as an option.

bubbleheadinc commented 3 months ago

Ok gotcha thanks. That works. Resizing option would be great. Looking forward to the next version!

enzedonline commented 2 months ago

I'm still getting there ... I've ended up on a major rewrite to flush out a lot of the buggy behaviour from the original, and also sorting out the iframe proxy situation so that it can work with touch screen. Mostly done, hopefully by this weekend if other demands stay away ...

enzedonline commented 2 months ago

There was a lot more to rewrite than first thought, plus other stuff getting in the way. Not quite finished testing, but I think it's more or less done. Relative sizing mode (px or %) can be set per blot via toolbar, or for every resize action on every blot. Each iframe gets its own proxy now - eliminates all the bugs with the mouse enter event and tracking the cursor, also means it can be used with touch screens now. There's an option to set the style on the proxies which would let you add the z-index there. You can test the compiled version if you like, downloadable from here. Happy to know if you can find any bugs. The updated readme is there for reference.

enzedonline commented 2 months ago

v2.2.0 released now.

Scroll by wheel or touch now sorted.

You can amend video selector and proxy image style with

  video: {
    selector: 'iframe.ql-video',
    proxyStyle: {}
  }

Relative sizing option available (never, by default, by option, or always). For 'by default' or 'by option', a % button is added to the toolbar to change modes.

There is a ton of other updates, worth looking at the change log and readme for stuff that might be useful.