Popover position broken when <popover> is inside a relatively positioned element

[diachedelic]

The issue was introduced here (I've tested this), which was released in v1.1.7 (I had no issue in prior versions).

I have the issue that my popovers now appear hundreds of pixels away from their handle.

Specific change was this:

-      let { offsetLeft, offsetTop } = target
+      let offsetLeft = trRect.left
+      let offsetTop = trRect.top

[rehfres]

I believe the problem is here:

// Position within the parent
      let offsetLeft = trRect.left
      let offsetTop = trRect.top

where trRect.left and trRect.top are taken with .getBoundingClientRect method, which returns left and top values from viewport - not from the parent. So they are not relative to parent, but are maybe used as such? I may be wrong tho

[diachedelic]

Sounds about right. According to the commit message, the change was supposed to "fix scrolling behaviour", but I do not really understand the original issue was.

[DrewBarclay]

As @150grism said, the problem is that getBoundingClientRect returns coordinates relative to the viewport. This means if you scroll down and then open a popout, it'll be off by a distance equal to the scrolling.

One fix is to change the popover from position: absolute to position: fixed. But then the popout will stay on the screen while you scroll, which could be an issue. (Fine for my use cases, though.)

Edit: Fixed it more thoroughly, though the code will still break with relatively position ancestor elements.

Grab the document's top/left with:

      // now, transform to document coordinates
      // https://stackoverflow.com/a/26230989
      var body = document.body
      var docEl = document.documentElement

      var scrollTop = window.pageYOffset || docEl.scrollTop || body.scrollTop
      var scrollLeft = window.pageXOffset || docEl.scrollLeft || body.scrollLeft

Now take the coordinates being returned in getDropdownPosition and just add scrollTop to the top and scrollLeft to the left to transform from viewport coordinates to document coordinates. No other changes needed; keep the position: absolute.

[diachedelic]

Prior to the commit fixing the scrolling, getBoundingClientRect was not being used - target.offsetLeft was used instead (the offset from the closest relatively positioned parent element).

@DrewBarclay seeing as the popover is absolutely positioned relative to its parent element, shouldn't getBoundingClientRect be avoided?

[DrewBarclay]

@diachedelic You should avoid it if you can, yes. There's no reason here to go into viewport units. The original version of the code, before the change to use getBoundingClientRect, seems like the way to go - I haven't looked into why the change was made.

However, for some other local changes I was making, I found using viewport units to be convenient so I didn't bother fixing that. It's ultimately not a huge deal to use getBoundingClientRect and then convert from viewport to document coordinates via scrollTop/scrollLeft, it's just kind of awkward. My fix works fine, as far as I can tell.

[diachedelic]

@DrewBarclay Doesn't that wreak havoc when the popover is inside a relatively positioned parent element not located at the origin? Does the parent offset have to be accounted for somewhere?

One solution is using position: fixed (to change the coordinate system to the viewport) but I'm not to keen on that because it will look weird to have the popover fixed while scrolling...

[DrewBarclay]

@diachedelic Sorry, misunderstood before - yes, you're right, you'd have to account for the parent offset, since the popover is going to be relative to it with position: absolute. In which case viewport units will screw everything up and it won't be easy to fix. getBoundingClientRect is just flat out not the way to go in this case - you would be best off using an old version of this library, I think.

[diachedelic]

@DrewBarclay Hmm, I don't think my use case is an edge case...surely others are going to insert their <popover> deep in the document tree (inside a bunch of nested Vue components) and there very well may be some ancestor element which is absolutely/relatively positioned.

I'm using v1.1.6, and it works, so I'm happy for now, but it would be a shame if people installed this module and immediately got weird positioning bugs.

[DrewBarclay]

@diachedelic I agree with everything you said.

For my particular use case, I'm not concerned about that. I'll make a note on my previous comment.

[euvl]

Cool bananas, happy to see conversation going on guys 👍 😄

It might be reasonable to implement a flag for switching between relative and global position.

If anyone want to have a crack at fixing this issue, you are very welcome to 👍

[diachedelic]

How about getDropdownPosition (target, dropdown, direction, origin) { ... } where origin is 'parent', 'viewport' or 'global' - does that work for you @DrewBarclay ?

[DrewBarclay]

@diachedelic No need. I would just base it off the parent in all cases, as was the behavior in 1.1.6 as I understand it. I'm not sure why that was changed.

Edit: I've reviewed the commit log, and it seems the code always used getBoundingClientRect. I'm not sure what broke things in 1.1.7. Was it always broken? I only started using the library recently.

[diachedelic]

@DrewBarclay it looks like prior to the commit I linked to at the start of this issue, getBoundingClientRect was only used for width/height of the dropdown, not for positioning (for that, target.offsetTop/Left was used, which works and is why I'm sticking to 1.1.6).

[DrewBarclay]

@diachedelic I see. But what was broken about scrolling? Why was the change made, considering it actually broke scrolling in 1.1.7?

[diachedelic]

@DrewBarclay I would love to know

[aschempp]

Combining the code from #27 (comment) this seems to work:

let scrollTop = window.pageYOffset || document.documentElement.scrollTop || document.body.scrollTop
let scrollLeft = window.pageXOffset || document.documentElement.scrollLeft || document.body.scrollLeft

// Position within the parent
let offsetLeft = trRect.left + scrollLeft
let offsetTop = trRect.top + scrollTop

[euvl]

Can you create PR pls @aschempp ? Just make sure that demo still works :)

[aschempp]

PR in #34

[absolutehype]

@euvl any news on when this will be released?