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

Delayed grid rendering causing errors #3536

Closed
iorzetic opened this issue May 20, 2015 · 16 comments
Closed

Delayed grid rendering causing errors #3536

iorzetic opened this issue May 20, 2015 · 16 comments
Assignees

Comments

@iorzetic
Copy link

In the setup that we have we need a grid container node as a parent of the grid, node which has a ngIf condition on it. This condition becomes true at some point so the parent node gets rendered and implicitly the grid starts rendering. In this setup, we get an error from the grid because it cannot find the function "focus" inside the uiGridCtrl.

The line that triggers the error is line 13446: uiGridCtrl.focus();

This line is executed in the post compile function for the uiGridViewport directive, but the focus function of the uiGridCtrl is defined in the post compile function for the uiGridRenderContainer directive.
Running a debug in both situations (with ngIf and without ngIf on the grid container element) we have found that the order in which these post compile functions of these two directives are executed is reversed when we have an ngIf on the container, therefore the function "focus()" is not found because it has not yet been defined.

I have created a plunkr by doing some minimal changes to the example from the cellNav tutorial page: http://plnkr.co/edit/QnTO3mOrhAkE762lpzXc?p=preview
When you hit the "Toggle ngIf condition" the grid will start to render and you will get the error. If the grid is rendered directly, the error cannot be reproduced because the order of execution for the post compile functions seems to be correct.

Did we do any setup mistakes with our grid instance or is this a bug somewhere?

@raysuelzer
Copy link

I'm getting the same error. I'll let you know if I figure out how to resolve it.

@PaulL1 PaulL1 assigned PaulL1 and swalters and unassigned PaulL1 Jun 5, 2015
@PaulL1
Copy link
Contributor

PaulL1 commented Jun 5, 2015

@swalters: looking at the code, it looks like we call uiGridCtrl.focus() from the viewPort. uiGridCtrl.focus is defined by the render container, and in this particular instance that line of code is running after the viewPort directive does.

Have you come across this before - is it expected?

@jmcmichael
Copy link

For what it's worth, I noticed this bug after I updated from Angular 1.3.15 to 1.4.1. I also updated several other packages at the same time, including ui-router and angular-formly but those don't seem to be involved. Fortunately it doesn't seem to cause any issues with the app other than the error msg in the console, perhaps b/c I'm using pagination instead of infinite scroll.

@iorzetic
Copy link
Author

The main problem that we have with this is that we are using cellNav, and when we update to the latest version (that contains the problem causing changes) the focus is not set on the first cell anymore.

@iorzetic
Copy link
Author

Just updated to the latest version. Awesome fixes all-around, but this fix is still missing. The regression that this "bug" introduces is major for us; we cannot use cellNav anymore.
Is there anyway we can get around this situation?

@PaulL1
Copy link
Contributor

PaulL1 commented Jun 22, 2015

I'd guess maybe some timeouts before setting focus to the first cell would give you what you need? Perhaps wait for data to load, then a timeout, then scollToIfNecsesary?

@iorzetic
Copy link
Author

We do wait for the data to load and do set the focus ourselves, so that is not the problem. The problem is with the line "uiGridCtrl.focus();" from the grid code, which causes an error because the function is not yet defined.
I was forced to comment out that line (I am completely unsure of what problems that may cause) because I needed to get the latest version installed.
Maybe we can add a simple test there if the function exists before executing it?

@MattiJarvinen
Copy link

Experiencing this with angular-1.2.28

@rangoy
Copy link
Contributor

rangoy commented Jun 24, 2015

I'm also experiencing this with angular-1.3.16, so I think that it's not related to angluar-1.4 compatibility.

This bug has made me downgrade to ui-grid 3-rc21. I do think that this should be fixed before 3.0 is released

@rangoy
Copy link
Contributor

rangoy commented Jun 24, 2015

@iorzetic you could add if(angular.isFunction(uiGridCtrl.focus)), but then you will not know wheter it runs or not.

Maybe it could be possible to change the priority for the directives in question?
I see that both of these has "priority: -99999, //this needs to run very last"
uiGridViewport <- usese focus()
uiGridRenderContainer <- creates focus()

Or could it be possible to do the focus() in the linking function and not the compile function?

@iorzetic
Copy link
Author

@rangoy Thanks for the suggestion. I know I can manually do that but I would rather not modify 3rd party libraries, that is usually not a wise path to go on.
I think that this change that you mentioned could be directly added into the ui-grid codebase.

By the way, I am able to reproduce this both on AngularJS 1.3.16 and 1.3.4

@rangoy
Copy link
Contributor

rangoy commented Jun 24, 2015

@iorzetic yes, this should be fixed and a pull request should be made.

@swalters - could you take a look at the change you did in aa56355 and see if you can take some measures to ensure that foucus() is registered before use?

@rangoy
Copy link
Contributor

rangoy commented Jun 25, 2015

I tried to change the priority, but it did not seem to have the desired effect.

Would this workaround be acceptable? I tested it a bit, and it seems to work. I think it will work reliable as it won't execute until next digest cycle, right?

//Wait for it to be defined
$timeout(function(){uiGridCtrl.focus()});

@rangoy
Copy link
Contributor

rangoy commented Jul 23, 2015

Any progress on this?

Tried 3.0.1, and I see the same issue there.

Seems like the change referenced from #3896 changes the code a bit, and maybe this will solve the issue?

@tomyam1
Copy link

tomyam1 commented Jul 24, 2015

This is probably due to a change made between rc21 and rc22.

I've started getting these errors after updating from rc 20 directly to 3.0.1.
I then downgraded to rc22 and still got those errors.
I then downgraded to rc21 and everything is fine now.

@mportuga
Copy link
Member

This is no longer reproducible in the latest version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants