Comment positioning wrong when Hot inside any scrolling region

jon · 4 November 2016 10:19

Summary:

Comment tooltip mis-positioned when Hot inside scrolling region/DIV

I have a Hot with comments enabled. The page has some scrolling <DIV> region, inside which everything including the Hot is placed. Note that it is not the Hot, nor the page/body, itself which has scrolling (that may or may not work, not my situation).

See https://jsfiddle.net/vdow00w1/ for a simple example.

Detail:

When you initially hover over the comment in cell (4,4) the tooltip is correctly placed next to that cell. But if you scroll the enclosing DIV the tooltip stays in exactly the same position relative to the page, whereas it should be relative to the cell. In this particular case that is not the end of the world, but in more complex example it actually gets placed below very bottom left corner of the page, causing redrawing & flickering and being inaccessible.

I have had a look at code and handsontable.full.js. The tooltip is positioned with position: absolute. Calculation of coordinates is in refreshEditorPosition. I think the problem may come from var cellOffset = offset(TD);. function offset() does some kind of walking up the container hierarchy till it reaches document.body looking at offsets, and does something if body’s child is position: fixed. But I do not see it taking into account a scrolled DIV ancestor, as you have to whenever using position: absolute?

The code in refreshEditorPosition looks too scary for me to understand (e.g. it looks at getBoundingRect() to decide whether to hide the tooltip depending on its location, and I’m not sure whether that is right/what coordinates it uses).

Is there anything I can do myself to rectify this problem? For example, if there was any positioning style I could place on the scrolled ancestor DIV I might consider that, but I did not see anything in my investigations.

Thanks in advance.

P.S.
FWIW, the Context Menu also uses position: absolute but is not confused/mis-positioned when inside a scrolled DIV. I think it gets its position right via pageY(event), so off the mouse pointer coordinates of the right-click. But I guess that is not suitable for a comment’s tooltip, as that needs to find the cell is comment is in?

aleksandra_budnik · 4 November 2016 10:19

Hi @jon

I can see that we can always count on your detailed descriptions. Thanks!

I think that this issue https://github.com/handsontable/handsontable/issues/3081 shows the same behavior. How you think?

I’m waiting for an update from our developer Jan as I was informed that the fix for it is already made and Jan proceeds with some tests. It may even get to the newest version of Handsontable - 0.29.0

I left a note on #3081 to let you know about the fix.

jon · 4 November 2016 12:54

Hi @aleksandra_budnik

I can see that we can always count on your detailed descriptions. Thanks!

I think that this issue https://github.com/handsontable/handsontable/issues/3081 shows the same behavior. How you think?

Yes & No. I did look at that one. In that case it is the Hot itself which is scrolled. That is why I said in my case it is not the Hot that has scrolling, but simply that some parent(s) on the page are scrolled (e.g. in may case everything is enclosed in a <DIV> somewhere up the hierarchy, and that is scrolled). That is what my JSFiddle shows.

However, the problem and/or solution may be the same both for Hot scrolling and Hot parent-ancestor scrolling. I am currently debugging and looking into solution…

jon · 4 November 2016 16:51

OK, I believe I have completed my work to solve this issue.

In handsontable.js, Comments class, refreshEditorPosition function, just after where it calls offset() to calculate the cell (TD)'s position, I have inserted so it now reads:

var cellOffset = offset(TD);

// Adjust cellOffset to take into account if the Hot itself is inside a scrolled region.
var cellOffsetScroll = this.offsetScroll(this.hot.rootElement);
cellOffset.top -= cellOffsetScroll.top;
cellOffset.left -= cellOffsetScroll.left;

var lastColWidth = this.hot.getColWidth(this.range.from.col);

And just above refreshEditorPosition function I have inserted:

offsetScroll: function (elem) {
  // Return the total amount an element has been scrolled by.
  // This is called from refreshEditorPosition() after it calls offset(), which is trying to position the comment popup window correctly,
  // but gets it wrong when elements have been scrolled.
  // Note this definition complements the current definition of function offset(), and the way code in refreshEditorPosition() works,
  // and might need altering if those change.
  // This function seems to need visit elem.parentElement and not just the more expected elem.offsetParent to get it right.
  var scrollLeft, scrollTop;
  scrollLeft = scrollTop = 0;
  while (elem && elem != document.documentElement)
  {
    scrollLeft += elem.scrollLeft;
    scrollTop += elem.scrollTop;
    if (elem.style.position == 'fixed')
      break;
    elem = elem.parentElement;
  }
  return {
    left: scrollLeft,
    top: scrollTop
 };
},

There are a few things to note:

The basic issue is that you are trying to turn the position of a cell into coordinates suitable for position: absolute; left: ??px; top: ??px. Your code is taking the element’s hierarchy’s offsets into account, correctly, but you need to subtract all the amounts that enclosing regions (DIVs etc.) have been scrolled by to correctly calculate the absolute position.
As per the comments, this is quite dependent on the how the existing code in refreshEditorPosition() and in offset() work. If they are changed, this must be re-examined.
If writing it yourselves you might want to merge this into a new function based off offset() which does both the calculations for offsets and scrolls at the same time. I/you would not want to change offset() itself as that is used in many other places where the code would not want to take scrolling into account.
Note that the scroll adjustment offsetScroll() is passed parameter this.hot.rootElement, not the TD. This is because later on in refreshEditorPosition() you call this.hot.view.wt.wtOverlays.{top,left}Overlay.getScrollPosition(), which I believe does the scrolling adjustment for scrolls within (as opposed to outside) the Hot.
If you have a Hot with stretch: "all", the tooltip is likely to come up a little to the left of where it should do. It goes to the correct position if the user resizes the column explicitly, and can be seen to revert to “off-position” if user double-clicks column resize handler to reset to default. This is down to your line var lastColWidth = this.hot.getColWidth(this.range.from.col);. I suspect that at this stage in the code an non-explicit column width has not yet been allocated the extra stretching it will get later and so your calculation thinks the column is narrower than it will end up being, resulting in the tooltip overlapping it. That is not in my code, so I am leaving that alone!

I have tested in a variety of situations, including enclosing elements having position: relative/absolute/fixed, and nested scrolling regions. You would want to do the same! If you think some of my code is incorrect/not needed, I urge you to test with nested scrolling regions, as some bits were required to get that right.

According to me, this seems to solve both the situation where the Hot itself is inside enclosing scrolled regions and the situation where the Hot has scrolling rows/columns inside itself (so should solve https://github.com/handsontable/handsontable/issues/3081).