Users/status: don't accumulate a click handler on every refresh - #17
Open
zattak1 wants to merge 1 commit into
Open
Users/status: don't accumulate a click handler on every refresh#17zattak1 wants to merge 1 commit into
zattak1 wants to merge 1 commit into
Conversation
refresh() starts with $(tool.element).empty(), which removes the children but
not handlers bound on tool.element itself -- and CASE 1 (logged in) and
CASE 2 (optimistic placeholder) both bind Q.Pointer.click there.
Passing `tool` to .on() does not help: that registers the bind in
Q.Event.jQueryForTool, which is flushed when the tool is REMOVED, and a
refresh is not a removal. So every refresh leaves another live handler on the
same element, and one click fires state.onInvoke once per refresh so far.
CASE 3 (logged out) escapes it only because it binds on a freshly-built
child, not on tool.element.
Measured via CASE 2, counting jQuery handlers on tool.element:
before: refresh Qbix#1..Qbix#5 -> click:2,3,4,5,6; one click fired onInvoke 5x
after: refresh Qbix#1..Qbix#5 -> click:2,2,2,2,2; one click fired onInvoke 1x
Unbind before rebinding, at the .empty() that makes it look already handled.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
refresh()starts with$(tool.element).empty(), which removes the children but not handlers bound ontool.elementitself — and CASE 1 (logged in) and CASE 2 (optimistic placeholder) both bindQ.Pointer.clickthere.Passing
toolto.on()doesn't help here: that registers the bind inQ.Event.jQueryForTool, which is flushed when the tool is removed, and a refresh is not a removal. So every refresh leaves another live handler on the same element, and one click firesstate.onInvokeonce per refresh so far.CASE 3 (logged out) escapes it only because it binds on a freshly-built child rather than on
tool.element.The tool refreshes on
Users.onLogin,Users.onLogoutand each of the threeQ.Optimisticavatar events, so this accumulates in normal use.Measured via CASE 2, counting jQuery handlers on
tool.element:Unbind before rebinding, at the
.empty()that makes it look already handled.Found while fixing the same class of bug in Communities (Qbix/Communities#7), where the accumulated handler stacked one login dialog per page visited.