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

getBounds(true) and async content #1588

Closed
buffpojken opened this issue Jul 16, 2017 · 9 comments
Closed

getBounds(true) and async content #1588

buffpojken opened this issue Jul 16, 2017 · 9 comments

Comments

@buffpojken
Copy link

I'm having some trouble asynchronous content (oembeds, larger images - anything where the true size of the content is not known when adding the element, but rather once data has been loaded) versus the sizes reported by getBounds()

I've implemented an oembed-blot, basically using the architecture of the TweetBlot in the "Cloning Medium..."-example with some additional window dressing - and also the side-menu. However, content that has an unknown height results in the side-menu being placed "one linebreaks height" from the insertion point, rather than the true content height, and some further investigation has proven that (of course), this is due to the BlockEmbed being one linebreak high when the "editor-change" event is posted, and then expanding once the content loads.

What's the best approach for fixing this? Is there some proper way of notifying Quill about the updates in size? Another thought is whether or not it should reside within the Blot-definition, and explicitly setting the height once it's known, though that would still not AFAIU let the Quill-instance know anything about the change?

@jhchen
Copy link
Member

jhchen commented Jul 16, 2017

If I understand correctly the issue is:

  1. The application calls quill.getBounds()
  2. At that moment in time, the height of the bounds is ~15px, which Quill returns as the height
  3. The application saves / uses this value
  4. The embed changes its height

Is this correct?

@buffpojken
Copy link
Author

buffpojken commented Jul 16, 2017

Well, sort of.

  1. The application calls quill.getBounds() on editor-change in order to position some chrome (the sidebar "+").
  2. The user performs an embed (done via a paste-matcher, but that ought not to be relevant)
  3. The application, on a performed embed, adds a newline and changes the selection using the following code:
    that.editor.insertText(range.index+1, '\n', Quill.sources.SILENT) that.editor.setSelection(range.index + 2, Quill.sources.API); ,
    in order to provide the user with a new line to continue writing, and move the caret. So far, so good.
  4. The editor-change event is triggered, due to the code above, in which the handler calls quill.getBounds() to update the chrome-position.
  5. At this point, things go wrong. The reported bounds will report the embed as being one single "line" high, while in fact the embed may or may not have changed bounds (due to network conditions etc.)

I'm thinking that this may be intended behavior, since - at the explicit point in time getBounds() is called - the embed may be a single, empty block-element high. If this is the case, I'm guessing the Blot-implementation should take care to alert quill that the dimensions have changed or something similar?

@jhchen
Copy link
Member

jhchen commented Jul 16, 2017

I'm not clear on how this list is different from except your steps 1-3 sets up for 4 which is where my list's 1 starts.

The point is though at the moment the application calls quill.getBounds() the dimensions are correct. Quill cannot "know" this value will change and even if it could there is no way to inform the application. Without this latter part what you are suggesting with the Blot-implementation is not enough because it will never get to your application which would need to reposition the sidebar.

The suggestion I would make is for your application to listen on when the image is loaded and call getBounds then.

@buffpojken
Copy link
Author

buffpojken commented Jul 16, 2017

Hmm, thanks for the clarification!

So, in essence - let the application fetch the domNode created by the blot in question using regular means, observe any changes directly on that and don't involve quill at all regarding notifications on changed dimensions?

@jhchen
Copy link
Member

jhchen commented Jul 16, 2017

No I would let the image be added normally but just add a DOM onload handler to it. It could get complex finding the Blot then image domNode so you could use event delegation like so:

document.body.addEventListener('load', function(e) {
  if (e.target.tagName === 'IMG') {
    // an image has loaded
  }
}, true);

If it were my application I would probably just add a 1 second timeout before calling getBounds since the ratio of effectiveness to effort is very high.

@buffpojken
Copy link
Author

Thanks for the suggestion, I'll kick it around!

Also- thanks for an awesome editor!

@dhamaniasad
Copy link

@buffpojken Would it be possible for you to share some code on your implementation for oembed support? I'm evaluating Quill currently and oembed support is the main thing holding me back.

@buffpojken
Copy link
Author

Sure thing, it was quite trivial to implement a very solid support for oembed, including the fix discussed above.

We're using a matcher to capture any probable embeds, and using a little proxy checks for tags. If any are found, pass the data long. The renderer defines a custom Blot for embeds, as follows. Works extremely well.

class EmbedBlot extends BlockEmbed {
  static create(url) {
    let node = super.create();
    node.dataset.url = url;

    oembed.embed(url).then((res) => {
      if(res.result == "OEMBED_MISSING"){
        node.classList.add('lp--activityEditorLinkBox');
        node.setAttribute('contenteditable', false)
        node.innerHTML = `<a href="${url}" target="_blank">${url}</a>`   
      }else{
        node.innerHTML = res.html;
        node.style.height = res.height;
        let iframe = node.querySelector('iframe'); 
        if(iframe){
          iframe.addEventListener('load', function(){
            let ev = new CustomEvent('quill-embed-load', {bubbles: true});
            node.dispatchEvent(ev);
          })
        }
      }
    })

    return node;
  }

  static value(domNode) {
    return domNode.dataset.url;
  }
}

This implementation assumes a lot based on the proxy, but that should be workable anyway.

@tboevil
Copy link
Contributor

tboevil commented Apr 9, 2019

Whether the implementation of 2x images should also use this method? like this:

class ImageBlot extends BlockEmbed {
  static create(src) {
    const node = super.create()

    node.setAttribute('src', src)
  //node.setAttribute('onload', () => invokeformat(node))
    return node
  }

  static value(node) {
    return node.getAttribute('src')
  }

  static formats(node) {
    // We still need to report unregistered embed formats
    let format = {}
    if (node.naturalWidth || node.width) {
      format.width = (node.naturalWidth || node.width ) / 2
    }
    if (node.naturalHeight || node.height ) {
      format.height = (node.naturalHeight || node.height) / 2
    }

    return format
  }

  format(name, value) {
    // Handle unregistered embed formats
    if (name === 'height' || name === 'width') {
      if (value) {
        this.domNode.setAttribute(name, value)
      } else {
        this.domNode.removeAttribute(name, value)
      }
    } else {
      super.format(name, value)
    }
  }

}
ImageBlot.blotName = 'imageBlot'
ImageBlot.tagName = 'img'
Quill.register('blots/imageBlot', ImageBlot)

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

4 participants