Bubble menu doesn't position correctly if image has alignment

[zcuric]

Description
When using bubble menu with image nodes and if there is image alignment functionality present which is achieved with float, the bubble menu above image is not positioned correctly.

Steps to reproduce the bug
Steps to reproduce the behavior:

1. Go to https://codesandbox.io/s/tiptap-issue-template-forked-p5sny
2. Click on image and see how the bubble menu is position in the middle of the editor
3. Click on alignment button and change to left and see how the bubble menu is outside of editor
4. Click on text nodes, bubble menu works as expetced

CodeSandbox
I created a CodeSandbox to help you debug the issue:
https://codesandbox.io/s/tiptap-issue-template-forked-p5sny

Expected behavior
This should behave same as on text nodes.

Additional context
I did some investigation on the bubble menu and how it calculates position. It looks like the float changes the start and the end positioning of the node.

[joevallender]

I also get inconsistent coordinates from coordsAtPos even when the image aren't floated when the editor alignment is anything other than left (where I believe it is returning the correct left pos by happy accident).

I cloned the repo and logged/traced it, and just got a bit lost seeing exactly what was happening in coordsAtPos (first time I'm digging into this repo so I'm lacking some context for what all the bits being passed in are). While debugging, side was coming out as left for one image node, and then right for another identically sized/positioned/etc. image.

It appears to be a rework of the ProseMirror function and although the tiptap one has been updated more recently time-wise, it might have been based off an older version of the original? I found a few bubble menu position issues from the ProseMirror code while hunting this.

So, no actual help there - sorry! 🙈

[michelson]

Hello, I have a guess here:

• it seems that the coords are retrieved from a selection, but in a non editable context, like image blocks the selection does not exist, but at least we have an active element, the coords in this context will contain width == 0px, so the tippy is relocated to the left at px 0 .
• the solution to this is "detect" the non editable element and call the .getBoundingClientRect() on it, this value will be enough for tippy to display properly.

a proof of concept to fix this:

• in bubble-menu I've changed the logic on update method to this:

    this.tippy.setProps({
      getReferenceClientRect: () => {
        const selectedElement = this.editor.view.dom.querySelector(".is-selected")
        if(selectedElement){
          return selectedElement.getBoundingClientRect()
        } else {
          return posToDOMRect(view, from, to)
        }
      },
    })

I'm not sure how can we get the selected element effectively , in my case I just get the element with is-selected class
I hope this will give some light to fix the issue :)

[joevallender]

That's a cool solution! I tried to set getReferenceClientRect on the bubble menu's tippy-options to keep from editing the original file but it's set explicitly as we can see from the link you shared.

If a solution that covers all use cases could be made in the bubble menu extension, that would be great - but if that's not possible I wonder whether it could be re-jigged slightly so we can pass in our own getReferenceClientRect for each scenario.

[philippkuehn]

I think there are two issues here.

First, we have to provide an option to overwrite the logic for checking if a bubble menu should be shown or hidden. Basically these lines are doing it in the bubble menu plugin:

tiptap/packages/extension-bubble-menu/src/bubble-menu-plugin.ts

Lines 97 to 104 in 2c48bc0

	// Sometime check for `empty` is not enough.
	// Doubleclick an empty paragraph returns a node size of 2.
	// So we check also for an empty text size.
	if (empty || !$anchor.parent.textContent) {
	this.hide()
	return
	}

So I'm thinking of adding something like this (would be available as prop for vue/react components as well):

BubbleMenu.configure({
  // overwrite logic
  validate({ editor }) {
    const { empty, $anchor } = editor.selection
    
    // don’t hide for images
    if (empty || !$anchor.parent.textContent && !editor.isActive('image')) {
      return false
    }

    return true
  },
})

It's still too much code for overwriting in my eyes. Maybe there is some better way of doing this. Also the name validate is bad I think. Anyone an idea for the naming?

Second, there is probably an issue with calculating coords for atom nodes. I have to check that.

[zcuric]

@michelson Very good temporary solution, thanks!

@philippkuehn Thanks for looking into this! Yeah, validate doesn't fit here. Maybe to have something like shouldHideOn which would be array of extensions names and to have some logic in the update menu to check it.

[zcuric]

After looking into official tooltip ProseMirror example for tooltip, I made some changes to the posToDOMRect function. I switched from using custom made coordsAtPos and to regular from and to instead of ranged calculated ones and I got results that are much better. Positioning is mostly correct.

function posToDOMRect(view) {
  const { from, to } = view.state.selection
  const start = view.coordsAtPos(from), end = view.coordsAtPos(to)
  const top = Math.min(start.top, end.top);
  const bottom = Math.max(start.bottom, end.bottom);
// left and right calculation from the example
  const left = Math.max((start.left + end.left) / 2, start.left + 3)
  const right = Math.max((start.right + end.right) / 2, start.right + 3);
  const width = right - left;
  const height = bottom - top;
  const x = left;
  const y = top;
  const data = {
    top,
    bottom,
    left,
    right,
    width,
    height,
    x,
    y,
  };
  return {
    ...data,
    toJSON: () => data,
  };
}

[zcuric]

Also behaviour is different if the image extension is configured if the inline: true. Bounding client is different.

[philippkuehn]

@zcuric I've not tested your code but you can read about the reason of our custom coordsAtPos method here:

#215
ProseMirror/prosemirror-view#47

[zcuric]

@philippkuehn thanks for explanation!

[zcuric]

Also, why is coordsAtPos called 20 times on a simple click? 🤔

EDIT: It's tippy setProps.

[zcuric]

@michelson this actually can work without setting addtional class, because ProseMirror adds .ProseMirror-selectednode class on selected nodes.

   this.tippy.setProps({
      getReferenceClientRect: () => {
        const selectedElement = this.editor.view.dom.querySelector('.ProseMirror-selectednode')
        if (selectedElement) {
          return selectedElement.getBoundingClientRect()
        } else {
          return posToDOMRect(view, from, to)
        }
      },
    });

[zcuric]

I'm spending too much time on this issue 😅.

[zcuric]

The only way I'm getting consistent results is with these changes:

function coordsAtPos(view, pos, end = false) {
  const { offset, node }= view.domAtPos(pos); // view.docView.domFromPos(pos);
  let side = null;
  let rect = null;
  if (node.nodeType === 3) {
    const nodeValue = node.nodeValue || '';
    if (end && offset < nodeValue.length) {
      rect = singleRect(textRange(node, offset - 1, offset), -1);
      side = 'right';
    }
    else if (offset < nodeValue.length) {
      rect = singleRect(textRange(node, offset, offset + 1), -1);
      side = 'left';
    }
  }
  else if (node.firstChild) {
    if (offset < node.childNodes.length) {
      const child = node.childNodes[offset];
      rect = singleRect(child.nodeType === 3 ? textRange(child) : child, -1);
      side = 'left';
    }
    if ((!rect || rect.top === rect.bottom) && offset) {
      const child = node.childNodes[offset - 1];
      rect = singleRect(child.nodeType === 3 ? textRange(child) : child, 1);
      side = 'right';
    }
  }
  else {
    const element = node;
    rect = element.getBoundingClientRect();
    side = 'left';
  }
  if (rect && side) {
    const x = rect[side];
    return {
-     top: rect.top,
-     bottom: rect.bottom,
-      left: x,
-     right: x,
+     ...rect.toJSON(),
    };
  }
  return {
    top: 0,
    bottom: 0,
    left: 0,
    right: 0,
  };
}

function posToDOMRect(view, from, to) {
  const start = coordsAtPos(view, from)
+ if (to - from === 1) return { ...start, toJSON: () => start };
  const end = coordsAtPos(view, to, true)
  const top = Math.min(start.top, end.top)
  const bottom = Math.max(start.bottom, end.bottom)
  const left = Math.min(start.left, end.left)
  const right = Math.max(start.right, end.right)
  const width = right - left
  const height = bottom - top
  const x = left
  const y = top
  const data = {
    top,
    bottom,
    left,
    right,
    width,
    height,
    x,
    y,
  }

  return {
    ...data,
    toJSON: () => data,
  }
}

[philippkuehn]

I changed how posToDOMRect calculates the coords for NodeSelections. a4ec4ff

Your linked CodeSandbox looks good. Can you all please test again? @joevallender @michelson @zcuric?

[michelson]

@philippkuehn I can confirm that it works perfect 👍

[michelson]

@philippkuehn I've found an issue on custom blocks, were the top block has a height 0, that's the react-renderer div, I was able to fix this by getting the BoundingRect on to the div's fist child

  if (isNodeSelection(view.state.selection)) {
    const node = view.nodeDOM(from) as HTMLElement
    if (node && node.getBoundingClientRect) {
      return node.children[0].getBoundingClientRect() // THIS
    }
  }

issue:

Screen.Cast.2021-05-24.at.10.14.49.AM.mp4

[philippkuehn]

I just realized that it was a bad solution. But I’ll look at it again later.

[zcuric]

@philippkuehn It does work! But why it was bad?

[philippkuehn]

@philippkuehn It does work! But why it was bad?

The code was at the wrong place. Fixed it. c0e68d5

[philippkuehn]

@philippkuehn I've found an issue on custom blocks, were the top block has a height 0, that's the react-renderer div, I was able to fix this by getting the BoundingRect on to the div's fist child

  if (isNodeSelection(view.state.selection)) {
    const node = view.nodeDOM(from) as HTMLElement
    if (node && node.getBoundingClientRect) {
      return node.children[0].getBoundingClientRect() // THIS
    }
  }

issue:

Screen.Cast.2021-05-24.at.10.14.49.AM.mp4

I’m not sure if I can write a workaround for this. Why does it have a height of 0px? Because you work with floats?

[michelson]

exactly, floats.
would be a good idea to pass an optional node finder that overrides the default lookup ?

[zcuric]

@philippkuehn Thanks for the updates. One more issues with some custom image extensions. For example, I want to add an image resizer into editor and in that case I have to wrap image in a div:

// ...
  renderHTML({ HTMLAttributes }) {
    return ['div', ['img', mergeAttributes(this.options.HTMLAttributes, HTMLAttributes)]];
  },
//...

The same issue appears.

[michelson]

@zcuric this is because the getBoundingClientRect() is pointing to the wrapper itself, in react-views the wrapper does not have the style. as I commented above the getBoundingClientRect() should be done on the first child and not on the wrapper. Also it would be a good idea to make this configurable

[bdbch]

Is this still relevant or happening for anyone? I'll close this issue since it didn't happen to me personally, the issue is stale and there were a few merged PRs in the meantime.

Let me know if it still is relevant and reopen it if you feel so!

[thefinnomenon]

This is still an issue with react node views.

[michelson]

it is, I've solved that on my implementation on https://github.com/michelson/dante

[thefinnomenon]

@michelson I am still having the same issue in your editor as in mine where it doesn't center on the image node.

[gethari]

@thefinnomenon / @bdbch I have used debounce as suggested here in #1493 (comment) & it is working fine. Can we add debounce in the lib so that we don't have to copy paste the plugin code again in consumers source ?

[bdbch]

I created a pull request for this in #3385

[thefinnomenon]

#3385 does not fix this issue.

[romansp]

I believe there's a problem in debouncing implementation added in #3385. Every time update is called a new debounced function is created here:

tiptap/packages/extension-bubble-menu/src/bubble-menu-plugin.ts

Lines 161 to 165 in 2b6e4e3

	if (this.updateDelay > 0 && hasValidSelection) {
	debounce(this.updateHandler, this.updateDelay)(view, oldState)
	} else {
	this.updateHandler(view, oldState)
	}

When instead an instance of debounced function should be saved and reused.

Issue can be easily confirmed using this example:

function shouldShowHandler() {
  console.log('should show')
  return true;
}

<BubbleMenu
    :editor="editor"
    :should-show="shouldShowHandler"
    :update-delay="1000"
  >
  // ...
</BubbleMenu>

shouldShowHandler will be called on each selection change and won't respect updateDelay (actually more calls will be done after 1 second).

I tried fixing debouncing in my local clone, but there's another issue where oldState.selection inside debounced handler will be the same as new selection, thus isSame is true and it blocks bubble menu from showing up.

I will try to investigate a little bit more and will hopefully submit a PR.

[HAOUEHF]

When I use custom components, I also encounter issues with floating to the left and right, and incorrect position of the BubbeMenu component. Has this issue been fixed