Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

4.4.5: Infinite loop with cellnav and enableCellEditOnFocus #6653

Closed
5 tasks done
m4m4m4 opened this issue Apr 3, 2018 · 6 comments · Fixed by #6659 or #6669
Closed
5 tasks done

4.4.5: Infinite loop with cellnav and enableCellEditOnFocus #6653

m4m4m4 opened this issue Apr 3, 2018 · 6 comments · Fixed by #6659 or #6669

Comments

@m4m4m4
Copy link
Contributor

m4m4m4 commented Apr 3, 2018

In ui-grid 4.4.5, with cell-nav and cell-edit features enabled with enableCellEditOnFocus set to true.
Navigating to the last row in the grid gets stuck in an infinite loop (cpu in the tab goes often to 100%) or becomes un-editable

I am able to reproduce the error in the following plunker for both chrome and firefox.
Steps to reproduce: expand all rows, scroll to the bottom, select a cell in the bottom row
http://plnkr.co/edit/cVIvvBzZ6clPMEZzWAjr?p=preview

  • This is a bug report, not a question on how to use the grid.
    Use Stack Overflow or Gitter for questions.
  • You have searched the open issues to see if this bug has been filed before. We don't want duplicate issues.
  • You have reviewed the tutorials and documentation to ensure you are using the grid correctly. ui-grid.info
  • You are using the latest version of the grid. Older versions are not patched or supported.
  • You have provided steps to recreate your bug. A plunkr is even better.
@mportuga
Copy link
Member

mportuga commented Apr 4, 2018

This was most likely caused by the following commit:
87c9eed

We need to check it and see what needs to be done to resolve this issue without reintroducing the bugs that it fixed.

@m4m4m4
Copy link
Contributor Author

m4m4m4 commented Apr 5, 2018

The problem seems to go away if I remove the shorter "rowHeight: 23" and in my local environment I also need to remove "enableHorizontalScrollbar: 2".

mportuga pushed a commit that referenced this issue Apr 5, 2018
… changed.

Doing this will prevent us from getting stuck in an infinite loop of triggering the scrollEnd event.

fix #6653, fix #4221
mportuga pushed a commit that referenced this issue Apr 5, 2018
… changed.

Doing this will prevent us from getting stuck in an infinite loop of triggering the scrollEnd event.

fix #6653, fix #4221
@m4m4m4
Copy link
Contributor Author

m4m4m4 commented Apr 6, 2018

Thanks for being so fast with this
Something is still not 100% right, if i click on a top cell where it is only half visible due to scroll, the infinite loop often reappears. It's easy to reproduce in the same plunker but the latest version:
http://plnkr.co/edit/PS3lTnLrqiXSE58iHFhC?p=preview, try scrolling down a bit and then select a cell in the top row
Also in my local environment the bottom cell bug is still there if I use "enableHorizontalScrollbar: 2", I am not able to reproduce that error in the plunker however.

@mportuga mportuga reopened this Apr 6, 2018
@mportuga
Copy link
Member

mportuga commented Apr 6, 2018

Alright, thanks for the information. I am going to have to do some more analysis to figure out why the loop reappears.

m4m4m4 pushed a commit to m4m4m4/ui-grid that referenced this issue Apr 11, 2018
Adding headerHeight to topBoundary calculation makes it always larger than pixelsToSeeRow causing an infinite loop.

fixes angular-ui#6653
@m4m4m4
Copy link
Contributor Author

m4m4m4 commented Apr 11, 2018

Nevermind.. this doesnt solve when navigating by keys, the loop is still there but not visible.

m4m4m4 pushed a commit to m4m4m4/ui-grid that referenced this issue Apr 12, 2018
…ectly

Since scrollIfNecessary is called multiple times when enableCellEditOnFocus is true we need to make sure the scrollbarWidth and footerHeight is accounted for to not cause a loop.

fixes angular-ui#6653
@m4m4m4
Copy link
Contributor Author

m4m4m4 commented Apr 12, 2018

The following commit should do it for now. It however feels like i am fixing a symptom and not the underlying cause.

Since cellNav feature calls ScrollIfNecessary when navigating, I am not sure why the method needs to be called again from edit feature when enableCellEditOnFocus is active (which requires cellnav).
I would like to implement a fix like below, but there are a couple of tests failing that stops me from doing so.

function beginEdit(triggerEvent) { 
	if (gridCol.colDef.enableCellEditOnFocus === true) { 
		//If enableCellEditOnFocus is enabled cell will already be focused 
		beginEditAfterScroll(triggerEvent); 
	} 
	else { 
		//we need to scroll the cell into focus before invoking the editor 
		$scope.grid.api.core.scrollToIfNecessary($scope.row, $scope.col) .then(function () { 
			beginEditAfterScroll(triggerEvent); 
		}); 
	} 
}
`

mportuga pushed a commit that referenced this issue Apr 16, 2018
…ectly

Since scrollIfNecessary is called multiple times when enableCellEditOnFocus is true we need to make sure the scrollbarWidth and footerHeight is accounted for to not cause a loop.

fixes #6653
mportuga pushed a commit that referenced this issue Apr 23, 2018
…ontalScrollbar: NEVER (#6690)

* fix(Grid.js): ScrollIfNecessary does not account for scrollWidth correctly

Since scrollIfNecessary is called multiple times when enableCellEditOnFocus is true we need to make sure the scrollbarWidth and footerHeight is accounted for to not cause a loop.

fixes #6653

* Add check for gridCol not null

Make sure gridCol is not null before checking for enableCellEditOnFocus

* fix(Grid.js) Vertical scroll calculates height wrong with enableHorizontalScrollbar: NEVER

Use scrollbarHeight instead of scrollbarWidth for vertical scroll calculations. scrollbarHeight has the value 0 when enableHorizontalScrollbar is set to NEVER.

Round calculated boundary values as computed style may have decimal number which will not match pixelsToSeeRow
mportuga pushed a commit that referenced this issue Apr 24, 2018
* fix(Grid.js): ScrollIfNecessary does not account for scrollWidth correctly

Since scrollIfNecessary is called multiple times when enableCellEditOnFocus is true we need to make sure the scrollbarWidth and footerHeight is accounted for to not cause a loop.

fixes #6653

* Add check for gridCol not null

Make sure gridCol is not null before checking for enableCellEditOnFocus

* fix(Grid.js) Vertical scroll calculates height wrong with enableHorizontalScrollbar: NEVER

Use scrollbarHeight instead of scrollbarWidth for vertical scroll calculations. scrollbarHeight has the value 0 when enableHorizontalScrollbar is set to NEVER.

Round calculated boundary values as computed style may have decimal number which will not match pixelsToSeeRow

* Fix(cellnav.js), do not trigger edit on undefined event

Do not trigger a cell edit when the event is undefined, if needed through API pass a null object instead.
defields923 pushed a commit to defields923/ui-grid that referenced this issue Oct 30, 2018
… changed.

Doing this will prevent us from getting stuck in an infinite loop of triggering the scrollEnd event.

fix angular-ui#6653, fix angular-ui#4221
defields923 pushed a commit to defields923/ui-grid that referenced this issue Oct 30, 2018
…ectly

Since scrollIfNecessary is called multiple times when enableCellEditOnFocus is true we need to make sure the scrollbarWidth and footerHeight is accounted for to not cause a loop.

fixes angular-ui#6653
defields923 pushed a commit to defields923/ui-grid that referenced this issue Oct 30, 2018
…ontalScrollbar: NEVER (angular-ui#6690)

* fix(Grid.js): ScrollIfNecessary does not account for scrollWidth correctly

Since scrollIfNecessary is called multiple times when enableCellEditOnFocus is true we need to make sure the scrollbarWidth and footerHeight is accounted for to not cause a loop.

fixes angular-ui#6653

* Add check for gridCol not null

Make sure gridCol is not null before checking for enableCellEditOnFocus

* fix(Grid.js) Vertical scroll calculates height wrong with enableHorizontalScrollbar: NEVER

Use scrollbarHeight instead of scrollbarWidth for vertical scroll calculations. scrollbarHeight has the value 0 when enableHorizontalScrollbar is set to NEVER.

Round calculated boundary values as computed style may have decimal number which will not match pixelsToSeeRow
defields923 pushed a commit to defields923/ui-grid that referenced this issue Oct 30, 2018
…6691)

* fix(Grid.js): ScrollIfNecessary does not account for scrollWidth correctly

Since scrollIfNecessary is called multiple times when enableCellEditOnFocus is true we need to make sure the scrollbarWidth and footerHeight is accounted for to not cause a loop.

fixes angular-ui#6653

* Add check for gridCol not null

Make sure gridCol is not null before checking for enableCellEditOnFocus

* fix(Grid.js) Vertical scroll calculates height wrong with enableHorizontalScrollbar: NEVER

Use scrollbarHeight instead of scrollbarWidth for vertical scroll calculations. scrollbarHeight has the value 0 when enableHorizontalScrollbar is set to NEVER.

Round calculated boundary values as computed style may have decimal number which will not match pixelsToSeeRow

* Fix(cellnav.js), do not trigger edit on undefined event

Do not trigger a cell edit when the event is undefined, if needed through API pass a null object instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment