underscore.js fails to compare DOM nodes correctly; Toolbar JS will fail when upgraded to Backbone 1.0.0 [#2139571]

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
#3
#1	toolbar-underscore-2139571.patch	1.89 KB	jessebeach
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

jessebeach CreditAttribution: jessebeach commented 19 November 2013 at 22:30

Status:	Active	» Needs review
Issue tags:		+Spark, +sprint

File	Size
toolbar-underscore-2139571.patch	1.89 KB

In this patch, we store a reference to the active tab by its auto-generated ID.

Comment #2

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 20 November 2013 at 10:52

Assigned:	Unassigned	» jessebeach
Status:	Needs review	» Needs work

Works as advertised.

Before: Load page that has the Toolbar, click on any of the tabs, look at Drupal.toolbar.models.toolbarModel.attributes.activeTab in the console, you'll see a DOM node.
After: Load page that has the Toolbar, click on any of the tabs, look at Drupal.toolbar.models.toolbarModel.attributes.activeTab in the console, you'll see a selector like #toolbar-item--3.

Just one nitpick:

+++ b/core/modules/toolbar/js/toolbar.js
@@ -203,10 +203,10 @@ Drupal.toolbar = {
+      // active item is stored as an ID selector e.g. '#navbar-item--1'.
...
+      // '#navbar-item--1-tray'.

s/navbar/toolbar/