zurb / tribute

ES6 Native @mentions
https://zurb.github.io/tribute/example/
MIT License
2.04k stars 304 forks source link

Conflict between menuContainer and contenteditable in scrolling div #430

Open piloos opened 4 years ago

piloos commented 4 years ago

How can we reproduce this bug?

Use Tribute on a contenteditable div which is present in a scrollable div

Tribute version 5.1.2

What did you expect to happen?

Using menuContainer I should be able to get the tribute-container correctly positioned somewhere inside the scrollable div so that it scrolls together with the contenteditable element where it was attached to.

What happened instead?

The tribute-container did appear inside the scrollable div, but does not appear at the right location when typing '@'.

Link jsfiddle https://jsfiddle.net/piloos/t70szjv4/

More info and questions

I have made a comparison between 4 situations:

  1. Tribute attached to textarea within a scrollable div. The complete scrollable div is used as menuContainer

  2. Tribute attached to textarea within a scrollable div, but a wrapper around the textarea is used as menuContainer

  3. Tribute attached to contenteditable within a scrollable div. The complete scrollable div is used as menuContainer

  4. Tribute attached to contenteditable within a scrollable div, but a wrapper around the contenteditable is used as menuContainer

From these tests I have the following findings:

piloos commented 4 years ago

With the assumption that the menuContainer should be a close wrapper around the Tribute-attached element, I found the following issues in the two different code paths (the one for input/textarea and the one for contenteditable)

In this code path, the coordinates are calculated relative (menu vs. element) and then converted to absolute coordinates. When using a menuContainer, the coordinates should remain relative. No need to take into account the position of the element relative to the viewport and the position of the viewport relative to the window:

diff --git a/src/TributeRange.js b/src/TributeRange.js
index e2756dc..c93ddf1 100644
--- a/src/TributeRange.js
+++ b/src/TributeRange.js
@@ -496,16 +497,16 @@ class TributeRange {
         let windowLeft = (window.pageXOffset || doc.scrollLeft) - (doc.clientLeft || 0)
         let windowTop = (window.pageYOffset || doc.scrollTop) - (doc.clientTop || 0)

-        let top = 0;
-        let left = 0;
+        let top = 0
+        let left = 0
         if (this.menuContainerIsBody) {
-          top = rect.top;
-          left = rect.left;
+            top = rect.top + windowTop
+            left = rect.left + windowLeft
         }

         let coordinates = {
-            top: top + windowTop + span.offsetTop + parseInt(computed.borderTopWidth) + parseInt(computed.fontSize) - element.scrollTop,
-            left: left + windowLeft + span.offsetLeft + parseInt(computed.borderLeftWidth)
+            top: top + span.offsetTop + parseInt(computed.borderTopWidth) + parseInt(computed.fontSize) - element.scrollTop,
+            left: left + span.offsetLeft + parseInt(computed.borderLeftWidth)
         }

         let windowWidth = window.innerWidth

In this code path, the coordinates are calculated relative to the current viewport. In case of using menuContainer, we need to change the reference to relative vs. menuConainer by subtracting menuContainer's position relative to the viewport. In case of not using menuContainer we need to adjust for the viewport not being at the top-left of the windown (adding windowLeft/windowTop):

diff --git a/src/TributeRange.js b/src/TributeRange.js
index e2756dc..c93ddf1 100644
--- a/src/TributeRange.js
+++ b/src/TributeRange.js
@@ -569,10 +570,25 @@ class TributeRange {
         let left = rect.left
         let top = rect.top

+        if (!this.menuContainerIsBody) {
+            // coordinates are referenced absolutely to the viewport position
+            // we need to make them relative by subtracting the menuContainers viewport position
+            let menuContainerRect = this.tribute.menuContainer.getBoundingClientRect()
+            left = left - menuContainerRect.left
+            top = top - menuContainerRect.top
+        }
+        else {
+            // coordinates are referenced absolutely to the viewport position
+            // we need to take the viewport position into account
+            left = left + windowLeft
+            top = top + windowTop
+        }
+
         let coordinates = {
-            left: left + windowLeft,
-            top: top + rect.height + windowTop
+            left: left,
+            top: top + rect.height
         }
+
         let windowWidth = window.innerWidth
         let windowHeight = window.innerHeight

@@ -612,11 +628,6 @@ class TributeRange {
             delete coordinates.bottom
         }

-        if (!this.menuContainerIsBody) {
-            coordinates.left = coordinates.left ? coordinates.left - this.tribute.menuContainer.offsetLeft : coordinates.left
-            coordinates.top = coordinates.top ? coordinates.top - this.tribute.menuContainer.offsetTop : coordinates.top
-        }
-
         return coordinates
     }

With these calculations and with isMenuOffScreen disabled, I get correct positioning in scrollable divs.

@mrsweaters , can you confirm my assumptions and my possible fix. If this is the correct way forward, I can check what is going wrong with the isMenuOffScreen check.

mtomov commented 4 years ago

I can confirm that I'm experiencing the same issues described at 3 and 4. Thank you @piloos for the detailed research into the problem, and the proposed solutions. I hope they'll be incorporated into the library.

Meanwhile I'll give your branch a try. Hope that's all right. Thank you!

mrsweaters commented 4 years ago

@piloos This looks like it would be an improvement to me. Would you mind making a PR for this?

piloos commented 4 years ago

@mrsweaters , sure. Can you just quickly confirm or reject my assumptions:

ivanbozic commented 4 years ago

I have this exact same issue. @piloos, did you ever solve this issue apart from your recommended changes?

piloos commented 4 years ago

I have this exact same issue. @piloos, did you ever solve this issue apart from your recommended changes?

@ivanbozic I am currently still using my personal branch https://github.com/piloos/tribute/tree/issue-430. This branch hasn't been updated with changes on master (if any?), but is still working...

ivanbozic commented 4 years ago

Did you think about making a PR for this? I see there hasn't been any new releases since March. @mrsweaters, are you still interested in taking a look at this and merging into a new release?

mrsweaters commented 4 years ago

@piloos @ivanbozic Sure thing! If you submit a PR i'll review it and get it merged. Sorry I've been a bit distracted recently.

ivanbozic commented 4 years ago

No problem @mrsweaters, life does tend to get in the way of things. 😊

@piloos, I'll pull your personal branch and test out whether it solves my use case and get back to you here. Are you fine with submitting the PR / need any help on that?

piloos commented 4 years ago

@mrsweaters @ivanbozic , in my branch the complete heuristic to determine if the popup is placed correctly is commented out completely. Since I just started a new business, I can't really work on it now.