Last modified: 2013-09-09 15:17:52 UTC
As of recently the toolbar no longer goes in/out of floating mode after opening an inspector. It will stay in whatever position it was (fixed/absolute, top/left/right/height)
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.
Change 77274 had a related patch set uploaded by Krinkle: ve.ui.Toolbar: Use closure instead of ve.bind for event handlers https://gerrit.wikimedia.org/r/77274
Change 77274 merged by jenkins-bot: ve.ui.Toolbar: Use closure instead of ve.bind for event handlers https://gerrit.wikimedia.org/r/77274
Now fixed in the code; next scheduled deployment is not until 15 August, however. :-(
*** Bug 52326 has been marked as a duplicate of this bug. ***
*** Bug 52433 has been marked as a duplicate of this bug. ***
*** Bug 52789 has been marked as a duplicate of this bug. ***
*** Bug 52739 has been marked as a duplicate of this bug. ***
