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

Check that only one resize listener is added to Grid containers #3096

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
2 participants
@joshangell
Copy link
Contributor

commented Jul 14, 2018

This is a bit of a weird one and relates to this issue for the Spoon plugin.

I have narrowed it down to an infinite loop occurring if you open a Craft.FieldLayoutDesigner inside a Garnish.Modal. You can see the loop behaviour here:

https://joshangell.s3.amazonaws.com/share/screencast_2018-07-14_09-56-19.mp4

Narrowing it down further I discovered it was to do with Craft.Grid adding back on the resize event listener. It seems that in this case it adds more than one and they race against each other in some way.

One way around it that I came up with was to only add the resize listener if there weren’t any other listeners already attached to that element, as demonstrated in the PR. However, that doesn’t take into account the fact that other event listeners may have already been added before we get to this point.

The only way around that sort of scenario is to add a library or set of methods that deal with checking what kind of listeners are attached to elements so we can test if there is already specifically a resize listener and thus not be affected by any others. But that seemed a bit beyond the scope of what I was hoping to achieve, so I thought I would reach out now to start the conversation.

Obviously in terms of the plugin I have other options, such as not opening it in a modal, but it would be nice to be able to do so.

Thanks!

@brandonkelly

This comment has been minimized.

Copy link
Member

commented Jul 15, 2018

That change would prevent the resize event from getting registered if any other event listeners were registered on the grid container. And even if it were narrowed down to only check for other resize event listeners, it would prevent the grid from adjusting when its container’s height changes.

Why not just put your field layout designer in a nested div within the modal body?

@joshangell

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2018

Yep I can see it will affect all other event listeners, which is why I think this isn’t the solution.

But I’m confused as to why this is happening, nesting the fld inside another div doesn’t help, which after more testing makes me think its some kind of race condition due to grids being inside each other with resize events attached.

jQuery will de-dupe multiple attempts to attach the same event to the same node, so it shouldn’t be that there are multiple listeners attached as that shouldn’t be possible, I think its more to do with the way the listener gets removed and added in some kind of cycle.

@brandonkelly

This comment has been minimized.

Copy link
Member

commented Jul 16, 2018

Looks like this JS loop happens in Chrome (and probably other browsers too); Chrome is just able to handle it better than Safari.

@joshangell

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2018

Yep I saw that too, I’ve been testing in Chrome so I can see the console output.

@joshangell

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2018

Ah, it was to do with Garnish.Modal - it properly had me scratching my head this one. Thanks for the fix!

@brandonkelly

This comment has been minimized.

Copy link
Member

commented Jul 16, 2018

Me too :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.