Keybind Up de-editing issue when using Edit With Cell Nav (scrollToIfNecessary bug) #4411
Description
Hi,
After trying to create our grid using the Edit With Cell Nav feature which is exactly what we need. We noticed that if you move up to something that is new in the viewport it does not correctly scroll which causes that Cell to not be in Edit Mode. This seemed to work fine for going below the bottom bound however going over the top bound did not work. After looking at the UI-Grid code I found the issue.
Reproductions Steps:
http://ui-grid.info/docs/#/tutorial/309_editable_with_cellnav
If you scroll down to bottom in the demo app and then select a cell it will go to edit mode. Now continue to press the up key until you go past the top bound, in the old non fixed code it would incorrectly take you out of edit mode because of the scroll issue.
My Solution to the Problem:
The issue lies within the scrollToIfNecessary method. The calculation for scrolling up is not exactly correct. There were a few minor issues making the scrolling not consistent and one major issue which caused the up scrolling not to work.
The main issue was that the variable pixelsToSeeRow was calculated assuming you always needed an extra row. Here is the line of code.
var pixelsToSeeRow = ((seekRowIndex + 1) * self.options.rowHeight);
As you can see it adds one row to the seekRowIndex. This works perfectly fine when going down as you always move to the next row however for going up this should not include the +1 and should simply be:
var pixelsToSeeRow = (seekRowIndex * self.options.rowHeight);
Doing this fixes the major issue however there was also a few small calculation errors in the scrollLength and topBound variables.
For the scrollLength it is missing the scrollBarHeight which is included in other calculations for scrollLength throughout the code. Here is the before and after:
Code before scrollLength fix:
var scrollLength = (self.renderContainers.body.getCanvasHeight() - self.renderContainers.body.getViewportHeight());
Code after scrollLength fix:
var scrollLength = (self.renderContainers.body.getCanvasHeight() - self.renderContainers.body.getViewportHeight() + self.renderContainers.body.grid.scrollbarHeight);
This fix allows the scrollToIfNecessary method to more accurately scroll to where it needs to scroll.
The last change was in TopBound. In the code the TopBound calculation includes HeaderHeight however from my testing it doesn't seem like headerheight should be included as this throws off the topBound which makes the scrollToIfNecessary scroll prematurely. Here is the before and after:
Code before topBound fix:
var topBound = self.renderContainers.body.prevScrollTop + self.headerHeight;
Code after topBound fix:
var topBound = self.renderContainers.body.prevScrollTop;
Those three fixes make the up movement fluent and continue to keep the cell edited when moving up.
I have tried updating to 3.0.6 however the problem persists there. Below is the beginning of the old scrollToIfNecessary method and the same code with my fix so you can see all my changes. I also created a fork with the exact changes I made #4414.
Beginning of scrollToIfNecessary without my fix.
Grid.prototype.scrollToIfNecessary = function (gridRow, gridCol) {
var self = this;
var scrollEvent = new ScrollEvent(self, 'uiGrid.scrollToIfNecessary');
// Alias the visible row and column caches
var visRowCache = self.renderContainers.body.visibleRowCache;
var visColCache = self.renderContainers.body.visibleColumnCache;
/*-- Get the top, left, right, and bottom "scrolled" edges of the grid --*/
// The top boundary is the current Y scroll position PLUS the header height, because the header can obscure rows when the grid is scrolled downwards
var topBound = self.renderContainers.body.prevScrollTop + self.headerHeight;
// Don't the let top boundary be less than 0
topBound = (topBound < 0) ? 0 : topBound;
// The left boundary is the current X scroll position
var leftBound = self.renderContainers.body.prevScrollLeft;
// The bottom boundary is the current Y scroll position, plus the height of the grid, but minus the header height.
// Basically this is the viewport height added on to the scroll position
var bottomBound = self.renderContainers.body.prevScrollTop + self.gridHeight - self.renderContainers.body.headerHeight - self.footerHeight - self.scrollbarWidth;
// If there's a horizontal scrollbar, remove its height from the bottom boundary, otherwise we'll be letting it obscure rows
//if (self.horizontalScrollbarHeight) {
// bottomBound = bottomBound - self.horizontalScrollbarHeight;
//}
// The right position is the current X scroll position minus the grid width
var rightBound = self.renderContainers.body.prevScrollLeft + Math.ceil(self.gridWidth);
// If there's a vertical scrollbar, subtract it from the right boundary or we'll allow it to obscure cells
//if (self.verticalScrollbarWidth) {
// rightBound = rightBound - self.verticalScrollbarWidth;
//}
// We were given a row to scroll to
if (gridRow !== null) {
// This is the index of the row we want to scroll to, within the list of rows that can be visible
var seekRowIndex = visRowCache.indexOf(gridRow);
// Total vertical scroll length of the grid
var scrollLength = (self.renderContainers.body.getCanvasHeight() - self.renderContainers.body.getViewportHeight());
// Add the height of the native horizontal scrollbar to the scroll length, if it's there. Otherwise it will mask over the final row
//if (self.horizontalScrollbarHeight && self.horizontalScrollbarHeight > 0) {
// scrollLength = scrollLength + self.horizontalScrollbarHeight;
//}
// This is the minimum amount of pixels we need to scroll vertical in order to see this row.
var pixelsToSeeRow = ((seekRowIndex + 1) * self.options.rowHeight);
// Don't let the pixels required to see the row be less than zero
pixelsToSeeRow = (pixelsToSeeRow < 0) ? 0 : pixelsToSeeRow;
var scrollPixels, percentage;
// If the scroll position we need to see the row is LESS than the top boundary, i.e. obscured above the top of the self...
if (pixelsToSeeRow < topBound) {
// Get the different between the top boundary and the required scroll position and subtract it from the current scroll position\
// to get the full position we need
scrollPixels = self.renderContainers.body.prevScrollTop - (topBound - pixelsToSeeRow);
// Turn the scroll position into a percentage and make it an argument for a scroll event
percentage = scrollPixels / scrollLength;
scrollEvent.y = { percentage: percentage };
}
// Otherwise if the scroll position we need to see the row is MORE than the bottom boundary, i.e. obscured below the bottom of the self...
else if (pixelsToSeeRow > bottomBound) {
// Get the different between the bottom boundary and the required scroll position and add it to the current scroll position
// to get the full position we need
scrollPixels = pixelsToSeeRow - bottomBound + self.renderContainers.body.prevScrollTop;
// Turn the scroll position into a percentage and make it an argument for a scroll event
percentage = scrollPixels / scrollLength;
scrollEvent.y = { percentage: percentage };
}
}
Beginning of scrollToIfNecessary with my fix.
Grid.prototype.scrollToIfNecessary = function (gridRow, gridCol) {
var self = this;
var scrollEvent = new ScrollEvent(self, 'uiGrid.scrollToIfNecessary');
// Alias the visible row and column caches
var visRowCache = self.renderContainers.body.visibleRowCache;
var visColCache = self.renderContainers.body.visibleColumnCache;
/*-- Get the top, left, right, and bottom "scrolled" edges of the grid --*/
// The top boundary is the current Y scroll position.
var topBound = self.renderContainers.body.prevScrollTop;
// Don't the let top boundary be less than 0
topBound = (topBound < 0) ? 0 : topBound;
// The left boundary is the current X scroll position
var leftBound = self.renderContainers.body.prevScrollLeft;
// The bottom boundary is the current Y scroll position, plus the height of the grid, but minus the header height.
// Basically this is the viewport height added on to the scroll position
var bottomBound = self.renderContainers.body.prevScrollTop + self.gridHeight - self.renderContainers.body.headerHeight - self.footerHeight - self.scrollbarWidth;
// If there's a horizontal scrollbar, remove its height from the bottom boundary, otherwise we'll be letting it obscure rows
//if (self.horizontalScrollbarHeight) {
// bottomBound = bottomBound - self.horizontalScrollbarHeight;
//}
// The right position is the current X scroll position minus the grid width
var rightBound = self.renderContainers.body.prevScrollLeft + Math.ceil(self.gridWidth);
// If there's a vertical scrollbar, subtract it from the right boundary or we'll allow it to obscure cells
//if (self.verticalScrollbarWidth) {
// rightBound = rightBound - self.verticalScrollbarWidth;
//}
// We were given a row to scroll to
if (gridRow !== null) {
// This is the index of the row we want to scroll to, within the list of rows that can be visible
var seekRowIndex = visRowCache.indexOf(gridRow);
// Total vertical scroll length of the grid
var scrollLength = (self.renderContainers.body.getCanvasHeight() - self.renderContainers.body.getViewportHeight());
// Add the height of the native horizontal scrollbar to the scroll length, if it's there. Otherwise it will mask over the final row
if (self.renderContainers.body.grid.scrollbarHeight && self.renderContainers.body.grid.scrollbarHeight > 0) {
scrollLength = scrollLength + self.renderContainers.body.grid.scrollbarHeight;
}
// This is the minimum amount of pixels we need to scroll vertical in order to see this row.
var pixelsToSeeRow = ((seekRowIndex) * self.options.rowHeight);
// Don't let the pixels required to see the row be less than zero
pixelsToSeeRow = (pixelsToSeeRow < 0) ? 0 : pixelsToSeeRow;
// If we are moving downwards we need to add the pixels for the entire next row.
var pixelsToSeeNextRow = pixelsToSeeRow + self.options.rowHeight;
var scrollPixels, percentage;
// If the scroll position we need to see the row is LESS than the top boundary, i.e. obscured above the top of the self...
if (pixelsToSeeRow < topBound) {
// Get the different between the top boundary and the required scroll position and subtract it from the current scroll position\
// to get the full position we need
scrollPixels = self.renderContainers.body.prevScrollTop - (topBound - pixelsToSeeRow);
// Turn the scroll position into a percentage and make it an argument for a scroll event
percentage = scrollPixels / scrollLength;
scrollEvent.y = { percentage: percentage };
}
// Otherwise if the scroll position we need to see the row is MORE than the bottom boundary, i.e. obscured below the bottom of the self...
else if (pixelsToSeeNextRow > bottomBound) {
// Get the different between the bottom boundary and the required scroll position and add it to the current scroll position
// to get the full position we need
scrollPixels = pixelsToSeeNextRow - bottomBound + self.renderContainers.body.prevScrollTop;
// Turn the scroll position into a percentage and make it an argument for a scroll event
percentage = scrollPixels / scrollLength;
scrollEvent.y = { percentage: percentage };
}
}
If there are any questions please feel free to contact me, I have some issues with adding a new row at the bottom and scrolling to it but I will post that in a different issue thread.
Thanks,
David Pinho
david.pinho@optum.com