tradingtechnologies/SlickGrid — pull request #6 by wwilfingertt (2014-07-25 20:23:05+00:00)
Title: Fix where getVisibleRange().top could return -1

@sergiopereiraTT Non-urgent. Please review when you can. Explanation is longer than the code change.

`th` is only assigned where I made the code change. It was always subtracting the height of the horizontal scroll bar, even if no horizontal scroll bar was drawn. Later, in scrollTo(), when there is no scroll bar `scrollTop` is set to `th - viewportH`. That will be exactly negative the scroll bar height. `scrollTop` is then used in calculating the getVisibleRange.top value by floor(scrollTop / rowHeight) = -1.

With some numbers. On my Chrome the horizontal scroll bar height is 17. Widget is sized so that there isn't a horizontal scroll bar.

rowHeight = 25, numberOfRows = 5, viewportH = 417, scrollbarDimensions.height = 17
th = max(rowHeight * numberOfRows, viewportH - scrollbarDimensions.height)
th = max(25 * 5, 417 - 17) = max (125, 400) = 400

Fill Window calls scrollTo(y = 0) on resize, `scrollTop` gets eventually set to

y = 0
scrollTop = Math.min(y, th - viewportH + (viewportHasHScroll ? scrollbarDimensions.height : 0));
scrollTop = min(0, 400 - 417 + (0)) = min(0, -17) = -17

When you call getViewport() it calculates the `top` property in `getRowFromPosition`

// y is scrollTop, offset is 0
return floor((y + offset) / options.rowHeight) = floor((-17 + 0)/ 25) = -1

In LedgerER this messed with the 'show a red line if we are scrolled away from the top' because we show the line if viewport.top !== 0. It also messed with 'go out of live mode if you scroll from the top' because we compare viewport.top to determine which direction we scrolled. The correct value is 'we didn't scroll' but this slickgrid bug made it 'we scrolled up'.