https://bugzilla.wikimedia.org/show_bug.cgi?id=52441
--- Comment #1 from Krinkle <[email protected]> --- After much debugging (filed the bug only now), I figured out what caused it: The target toolbar continues to have toolbar.floatable === true. That never gets disabled by anything, so that's good. What happens is that the toolbar inside the context menu, twice, gets called disableFloatable on. Though that is a separate instance, independent, there is 1 place where it effects global state unfortunately: the $window handlers. The prototype is inherited by each instance, naturally, which means toolbarA.onWindowScroll === toolbarB.onWindowScroll. So when we pass them to $window.off to unbind by reference, it does exactly what you (now) expect, thus also breaking it for other toolbars. However that's not what's happening, actually, because we (of course) want each toolbar to have the event bound to the exact instance of ve.ui.Toolbar, and we do so by using ve.bind. Which means toolbarA.windowEvents.scroll is a unique closure, separate from toolbarB.windowEvents.scroll. That is what makes it work. So why does it fail? Well, jQuery.proxy (=== ve.bind) is so nice for us that it tacks a guid property on the function reference to make sure that when binding a function to a scope, we can still identify it. This e.g. makes the following possible: 1: $foo.on( 'scroll', $.proxy( myHandler ) ); 2: $foo.off( 'scroll', myHandler ); 3: // or alternatively, though useless: 4: $foo.off( 'scroll', $.proxy( myHandler ) ); jQuery's event system uses this same guid (when present) to unbind a method. So the first time we call ve.bind in a ve.ui.Toolbar construction, a guid is added to #onWindowScroll.. And the second time it just uses the same guid again (which makes line 4 above work). It does still make a new closure, so there's no problem with the second one being given a cached closure or something that refers to the first instance, nothing like that. However we very much don't want this. We need each one to be unique not just by reference and context bound, but no shared guids. Finally, why did this break only recently? Well, before 14343c7bf7 toolbar.$window was null by default and stayed that way until enableFloating was called. So the call to disableFloating for the context menu toolbar did nothing. 14343c7bf7 refactored the code to always need a toolbar.$window during initialisation, and for efficiency kept the instance around as to not create/destroy it each time. So though 14343c7bf7 introduced the bug being more visible, the problem was already in the code. Both then and now, when enableFloating is called and then disableFloating, the disableFloating unbinds all window events for effectively all toolbars. Also: - During debugging I found that we're calling disableFloatable *twice* on the context menu toolbar (once when the inspector is opened and again when we close it). That should be optimised to one. Or better, don't call it at all since it is disabled by default. -- You are receiving this mail because: You are on the CC list for the bug. _______________________________________________ Wikibugs-l mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
