Problem/Motivation
This bug was discovered in the Navbar backport: #1994834: Changing main tabs in firefox and ie9 not working properly, explained in comment #7.
Ok, so a couple things going on here. Navbar uses Backbone 1.0. Toolbar uses 0.9.2. So we often see bugs in Navbar that we will eventually encounter in Toolbar. The nice part is we get to fix them here first :).
The second part, is that this isn't really a Backbone bug...it's an Underscore bug. Here's the boiled-down proof:
The code from the image above:
current[attr] === val -> false val -> a#navbar-item--4.navbar-icon /en/user current[attr] -> a#navbar-item--3.navbar-icon /en/admi...shortcut _.isEqual(current[attr], val) -> true
_.isEqual
is incorrectly true when comparing two different DOM nodes. But this only happens in Firefox (~26) and IE (~9), not Chrome (~32).So, we'll need to store a string or some value that isn't a DOM node as the value of the 'activeTab' model attribute.
The reason we don't see this issue in Toolbar yet is that we're running Backbone 0.9.2 in Toolbar and 1.0.0 in Navbar. We will see this bug when we upgrade, so it's better just to deal with it now.
Proposed resolution
This was solved in Navbar by using the tab's id, rather than the DOM node itself.
User interface changes
None.
API changes
None.
Original report by @jessebeach
Comment | File | Size | Author |
---|---|---|---|
#3 | interdiff.txt | 1.34 KB | jessebeach |
#3 | toolbar-underscore-2139571-3.patch | 1.91 KB | jessebeach |
#1 | toolbar-underscore-2139571.patch | 1.89 KB | jessebeach |
Comments
Comment #1
jessebeach CreditAttribution: jessebeach commentedIn this patch, we store a reference to the active tab by its auto-generated ID.
Comment #2
Wim LeersWorks as advertised.
Drupal.toolbar.models.toolbarModel.attributes.activeTab
in the console, you'll see a DOM node.Drupal.toolbar.models.toolbarModel.attributes.activeTab
in the console, you'll see a selector like#toolbar-item--3
.Just one nitpick:
s/navbar/toolbar/
Comment #3
jessebeach CreditAttribution: jessebeach commentedMade the string changes of 'navbar' to 'toolbar'.
Comment #4
Wim LeersComment #5
nod_Comment #6
webchickCommitted and pushed to 8.x. Thanks!
Comment #8
Wim Leers