Refactor the Toolbar JavaScript to use Backbone; fix several poorly functioning behaviors

I wonder if it'd be appropriate to leverage Backbone here? What's currently in the issue summary obviously related to state, and reacting to state. If captured in a Backbone Model, with the toolbar just being a View, wouldn't that simplify things a lot?

Many parts of the back-end are slow anyway. Backbone + Underscore is 10.3 KiB. If you use Edit (which you likely do if you're an administrator), you're going to need Backbone + Underscore anyway.
So: does it really outweigh that?

I agree with your POV, I'm just not sure if it's really such a big difference overall.

added a bit about an a11y consideration

Tagging as something we're working on this sprint.

Please let me know when it is in a good place to test it. Also, any reason not to mark it needs review to get the bot to look it over?

Changes

• Doxygen fixes.
• Drupal.behaviors.toolbar.attach() was very dense due to zero blank lines. It may well be that Drupal uses too much blank lines too often, but here, especially combined with indented chaining, the code becomes very hard to parse for humans.
• toolbar.js file-level closure didn't include all used global objects (missing: Backbone, document, window). Similar for toolbar.menu.js.
• I changed StateModel's mql to mqMatches, so that it's a simple boolean instead of a complex object. This simplifies code elsewhere (e.g. no more need for manual triggering of the change event), and reduces the amount of data in the model. Since Toolbar's JS is using only a single media query (to determine tray orientation), I think this should be okay.
• Drupal.behaviors.toolbar.options was labeled as being "default options", hence I renamed it to Drupal.behaviors.toolbar.defaults.
• When the tray was oriented vertically manually (i.e. "locked"), and then the page was reloaded, then the tray orientation toggle would still indicate you could switch to vertical. In other words, the locking was not being respected and was being overridden by the media query-based tray orientation.
• model and view were "toolbar.js closure variables", which means they were available as "kinda globals" for any piece of code within toolbar.js. IMHO this is a bad practice. Especially if you take into account that 1) not all code within toolbar.js needs access to them, 2) the two Backbone Views in particular can then access the model via both this.model and model. This was trivial to fix for view, for model I had to move some things around.

Feedback

• I think the concept of "expose a limited API via Drupal.toolbar" is good, but I'm wondering why we should enforce this limitedness? For the "handler" functions, wouldn't it be more elegant if we would just have a publicly available Backbone View. E.g., you could add the following to any JS file that loads after contextual.toolbar.js (e.g. tour.js):
  

  Drupal.contextualToolbar.views.EditToggleView.prototype.onClick = function (event) {
    alert('overridden');
    this.model.set('isViewing', !this.model.get('isViewing'));
  };
  More importantly, are there even valid use cases for the three "handler" functions?
  I think <code>Drupal.toolbar.setActiveItem

  is fine though.
  

• AFAICT the only media query that is needed in the JS and must be reacted to, is the one that determines whether the tray orientation should change. This implies the JS can be simplified further, less JS settings can be passed to it (i.e. drupalSettings.toolbar.trayOrientationMQ = "" instead of drupalSettings.toolbar.breakpoints = {})
• The premise of Drupal.toolbar.setActiveItem does not match with how it's used in tour.module nor contextual.module. In those two modules, making the tab/toggle "active" means activating a mode that does not preclude the menu/shortcuts/user tabs from being active. ToolbarView.refreshTabs() already deals with this correctly (but only by accident), but that's besides the point. The point is that neither of those two modules should be calling setActiveItem() in the first place, AFAICT.
• ToolbarView.refreshTabs() and ToolbarView.changeOrientation() should be prefixed with underscores, because they're internal/helper methods. This makes it easier for others to navigate the code. Furthermore, I think _render(Tabs|Orientation)() would be better names. In particular changeOrientation() is a poor name because it suggests it will make changes, whereas it's only rendering what the current model state indicates. The comments for them in render() can then also go away; the docs on these private functions should be sufficient.
• I dislike how the "subtrees" stuff is being handled. It's too vague and disconnected, IMO. Plus, no JSON requests are being made for sublevels — the PHP says "To conserve bandwidth, we only include the top-level links in the HTML.", but in reality, it's all being fetched at once.

+++ b/core/modules/shortcut/shortcut.moduleundefined
@@ -568,7 +568,7 @@ function shortcut_toolbar() {
+            'class' => array('icon', 'icon-shortcut', 'toolbar-tab'),

Why are we having shortcut.module (and user.module, and …) having add a toolbar.module-owned class?

+++ b/core/modules/toolbar/config/toolbar.breakpoints.ymlundefined
@@ -1,3 +1 @@
-narrow: 'only screen and (min-width: 16.5em)'
-standard: 'only screen and (min-width: 38.125em)'
 wide: 'only screen and (min-width: 50em)'

There now is a discrepancy between the (sole) breakpoint in this .yml file versus the ones in toolbar.base.css.

+++ b/core/modules/toolbar/js/toolbar.jsundefined
@@ -46,269 +22,446 @@ var mql = {
+        // Setting the mql won't trigger a change in the model. It needs to be
+        // triggered manually.
+        model.trigger('change:mql');

This was only necessary because mql is an object.

+++ b/core/modules/toolbar/js/toolbar.menu.jsundefined
@@ -12,7 +12,7 @@
   $.fn.toolbarMenu = function () {

Shouldn't this be Drupal-prefixed, i.e. $.fn.drupalToolbarMenu?

tag

In talking to Jesse today, it sounded like this was pretty necessary refactoring, so escalating to major.

- Error on line 47 of toolbar.js - you didn't remove ;
- For wide screen toolbar doesn't push the content to the right ( but it used to )
- On mobile screen toolbar tray doesn't stick to the top, so there appears a white space.
- For my local build it takes about a second for script to run, and you get a fast toolbar pop, when js is executed. Can we replace it with some smoother animation ( if it won't kill the performance )
- And the toolbar switches to mobile for 285px, thought iPhone has 320 and it's more convenient to make that width for breakpoint :) And for mobile screen toolbar now stick to the bottom and there's even a larget gap. (see screenshots)

Code:
- Drupal.toolbar.setActiveItem($('.js .toolbar .bar .contextual-toolbar-tab'));
better reduce to $('.toolbar .contextual-toolbar-tab'). this is already js you don't need js class.
- for // Broadcast model changes to other modules // can you cache the $(document) variable? you use it 4 times
- for // Implements Backbone.View.prototype.initialize() // may be get rid of this.model repetition?
- var barHeight = this.$el.find('.bar').outerHeight(); change to var barHeight = this.$el.find('.bar').outerHeight(true);
I think overwise it won't work properly in IE

Everything else looks great! But it's hard to review large patches )

@jessebeach, it took me some time to return and review the patch.
But now i can't apply it - probably somebody changed the js of the toolbar )
Can you please reroll for latest core changes? Thx!

+++ b/core/modules/toolbar/js/toolbar.jsundefined
@@ -44,290 +16,473 @@ var mql = {
-            $('#toolbar-link-' + id).after(subtrees.id);

It only doesn't apply because this line changed, it went from subtrees.id to subtrees[id]

Moving back to needs review.

Awesome!
Works perfectly now! Covered all issues from #17.

Really hard to review this patch, mixed jQuery event, Drupal behaviors and Backbone way :)

Anyway, after applied patch:

DrupalBehaviorError: attach ; contextualToolbar: Drupal.toolbar.setActiveItem is not a function
http://127.0.0.1/drupal8x/core/modules/contextual/contextual.toolbar.js?...
Line 64

+++ b/core/modules/toolbar/toolbar.moduleundefined
@@ -628,6 +630,7 @@ function toolbar_library_info() {
       array('system', 'drupal.debounce'),

when and where use denounce ? (and why not just use underscore _.debounce)

+++ b/core/modules/toolbar/js/toolbar.jsundefined
@@ -1,358 +1,493 @@
+        locked: JSON.parse(localStorage.getItem('Drupal.toolbar.trayVerticalLocked')) || false,

default value set in Model.defaults ?

+++ b/core/modules/toolbar/js/toolbar.jsundefined
@@ -1,358 +1,493 @@
+        .on('drupalViewportOffsetChange.toolbar', function (event, offsets) {
+          model.set('offsets', {
+            top: offsets.top,
+            right: offsets.right,
+            bottom: offsets.bottom,
+            left: offsets.left

model.set('offsets', offsets) ???

+++ b/core/modules/toolbar/js/toolbar.jsundefined
@@ -1,358 +1,493 @@
+          $('body').css('padding-top', offsets.top);
...
+        });

shouldn't it a change event in Backbone.view ? (above code trigger a change event)

+++ b/core/modules/toolbar/js/toolbar.jsundefined
@@ -1,358 +1,493 @@
+      model
+        .on('change:orientation', function (model, orientation) {
+          $(document).trigger('drupalToolbarOrientationChange', orientation);
+        })
+        .on('change:activeTab', function (model, tab) {
+          $(document).trigger('drupalToolbarTabChange', tab);
+        })
+        .on('change:activeTray', function (model, tray) {
+          $(document).trigger('drupalToolbarTrayChange', tray);

Why not inside View.initialize

and more..

I'm all for using backbone if it makes the code better and if it doesn't increase the size too much.

I minified toolbar.js before and after the patch (so no whitespace or comments). Before patch: 4.3kb, after: 6.0kb. that's a 130% increase. I would have expected backbonned code to be smaller. I haven't looked too much into it yet. If someone can point out the reason of this increase before I get to it that'd be nice.

#31: minified size delta confirmed. When gzipped, the difference is "only" 21% though, not 30% (when not gzipped, I get 37–39% depending on the minifier, so I'd love to know what minifier you're using :)).
Let's see if we can reduce that difference.

Currently working on a deep review, hence assigning to myself.

Changes (in attached reroll)

1. Simple 80 cols docs fixes and minor code improvements.
2. Reordered the code in attach() a bit, so that all initializations happen first, and all event binding/model updating at the end.
3. Stop using jQuery.extend({}, options) in BB view initializations; instead explicitly pass in options.strings</strong>; the views don't need the <code>options.breakpoints.
4. The current calls to Drupal.announce are broken in many ways: when the page is loaded, the orientation is announced, while it should not, and if it's vertical upon page load, it's announced twice. The tray announcement is also broken in that it announced something like toolbar-item--2-tray tray opened, which is of course not very informative. I introduced a Drupal.Toolbar.AuralToolbarView to cleanly implement this, made the #heading property for the tray required instead of optional and am using that as the tray name instead, which is more consistent with what the screen reader user gets to hear *and* it's much easier to understand.
   In doing so, I'm afraid the minified code size has increased by another 500 bytes, to 6.5 KB.

Concerns

1. I understand that setting the top padding on the body element is essential, but all the classes that we're setting there, are all those also really necessary?
2. In contextual.toolbar.js, Drupal.MODULENAME.(models|views) refers to the "classes", in toolbar.js they currently refer to "instances". We should standardize on one.
3. (Continuing on the previous.) Generally, I think it makes sense for "external" code to be able to find BB models "instances" and listen to them. This makes sense, especially because that means we'd rely less on DOM events, which are inherently slower and less granular. However, I don't see a clear rationale for being able to access BB views "instances". What's the rationale for this? If another module wants to override BB views, then it would probably override the view "class"? (That's also what I've done in contextual.toolbar.js.)

Explanation for the increased size (even after minification)

1. The previous codebase was impossible to test. The current codebase is very much testable. Instead of having everything "hardcoded", the code now consists of independent components with very clearly separated responsibilities (the BB models & views). Jesse already explained this at the bottom of #29. Because of the better separation, an increase in size is unavoidable.
2. Many small things have been fixed (as per the issue title), like buggy orientation toggle, etc. That means more code.
3. However, the nice thing is: the Backbone models by definition contain only *essential* data. For example, Drupal.Toolbar.ToolbarModel contains only 7 "attributes". If you think you can build the same functionality with *less* code, then instead of rewriting everything, you can just write more efficient Backbone views :)

As per change 4 in #33, this is probably warranted.

Test failures due to #heading still being assumed to be optional in the tests. Easy fix. Let's first see whether Jesse agrees with my direction :)

#37: Fair enough :)

my thoughts:

- Keep all defaults value in the Backbone Model.
- Passing all of them into Model rather than Views. So, you can add more custom value and make use of BB.
- MenuView, I converted to use a Backbone.Collection. It lets you easily to extend and interact with BB , eg. reordering (collection.sort)
- move some events to ToolbarView view. This is teh main view of Toolbar, it should take over of events and let you override stuff with BB.
- minor code improvements.

// Add subtrees.
// @todo Optimize this to delay adding each subtree to the DOM until it is
// needed; however, take into account screen readers for determining
// when the DOM elements are needed.

I planed to drop a placeholder at beginning, and add a event handler to insert HTML code.
However seems like jQuery.once working based on element ID. I couldn't find a PHP way to change the subtress menu ID. any hint to me, thanks :)
For any purpose, I though it should be a unique ID/Class for each menu.

Furthermore changes:
- hack PHP toolbar_subtrees_jsonp and simplify JS Drupal.toolbar.setSubtrees.done

Cheers :)

I'd like to understand better what advantage a Collection gives us in the case of the Toolbar. We only have 2 models and they not really related -- not in the way that a Library Collection would be related to Book Models. I don't believe we need to act on model changes as a group, which is what a Collection would do for us. What use cases are you anticipating that this change would allow us to support?

OLD PATCH: All menu sections is one model
NEW PATCH: each menu section is a model

the menuCollection is a set of models of menu sections (Content, Structure, Appearance..etc).

- Collection is more flexible than model (http://backbonejs.org/#Collection)
Lets say when I dynamically add new menu section to toolbar via JS, and sort by "X". I can use http://backbonejs.org/#Collection-comparator directly.

I can find a menu item via http://backbonejs.org/#Collection-findWhere

More than that.

Ideally, Drupal backend should output an array of menu items rather than rendered HTML. Menu items html control by BB.render() & BB templates. smaller JSON response, easier to change the html structure

(Shortcuts-like modules may take advantage of it)

One of the reasons we've been keeping the defaults in the behaviors object is our recent pattern of splitting the visual view from, let's say, aural views or keyboard views, which might need those strings as well. This patch really lays out the division of Views well #1971108-20: Convert contextual.js to use Backbone (and support dynamic contextual links).

All views sharing SAME toolbarModel. I see no reason to pass it into View directly.

I don't think we can change the code in this way. The subtrees are loaded through an asynchronous AJAX request. We can't guarantee that we'll have the link data when this code is executed. Using the jQuery Promise object let's us set up the framework for inserting the links once we have the data, but it doesn't matter when that data arrives. Do the changes you support here still support that asynchronous loading? Maybe I'm just not grokking it.

It's same.

Old way: Drupal.toolbar.setSubtrees.done() trigger Model change event -> Render()
New way: Drupal.toolbar.setSubtrees.done() trigger Collection.reset event -> Render().

+++ b/core/modules/toolbar/js/toolbar.jsundefined
@@ -1,333 +1,547 @@
+      // Render the main menu as a nested, collapsible accordion.
+      this.on('drupalToobarMenu', function(){
+        if ('drupalToolbarMenu' in $.fn) {
+        this.$el
+          .find('> .menu')
+          .drupalToolbarMenu();
+        }
+      });

Developers may change the render() function. Recopy the code to execute $.drupalToolbarMenu(); is meaningless.
This is the main reason I move it out.

Ideally, I'd split Render() into addOne & addAll.

addOne, dynamically insert a menu section
addAll, optimized for render all items at beginning (no reflow)

However, it's too much in CORE, so i leave as it :)

I tested all permutations of actions I could think of, in combination with the Overlay module, but only in Chrome. The changes here should work cross-browser.

UX review

Found a bug: 0. do this on a laptop/desktop, 1. make viewport as wide as possible, 2. open "menu" tray, 3. toggle tray orientation to "vertical" (now Drupal.toolbar.trayVerticalLocked = true is set in localStorage), 4. reload the page (note that the localStorage entry remains unchanged), 5. attempt to toggle back from vertical to horizontal: the first try will fail, the second will succeed.

What I also think to be a bug: when the viewport is as narrow as possible and the tray orientation *must* be vertical, the orientation toggle is still visible and allows me to switch the orientation to horizontal. When I do that, the end result looks broken. IMHO this should be forbidden.

Finally, I find it rather confusing that the "Home" tab is always pressed on the home page. If you click the "Menu" tab while you're on the homepage, it looks like two tabs are active simultaneously.

Code review

In the attached reroll: fixed a bunch of typos in the docs, made some minor changes and renamed a few functions for better consistency.

Further feedback:

+++ b/core/modules/toolbar/js/toolbar.jsundefined
@@ -45,329 +51,520 @@ var mql = {
+          // Curry the model and the label of the media query breakpoint to the
+          // mediaQueryChangeHandler function.
+          mql.addListener(Drupal.toolbar.mediaQueryChangeHandler.bind(null, model, label));

I was going to say this won't work in IE9, but AFAICT it will: http://kangax.github.io/es5-compat-table/#Function.prototype.bind

+++ b/core/modules/toolbar/js/toolbar.jsundefined
@@ -45,329 +51,520 @@ var mql = {
+    // Update the page and toolbar dimension indicators.
+    //updatePeripherals();

This needs to be either removed or used.

+++ b/core/modules/toolbar/js/toolbar.jsundefined
@@ -45,329 +51,520 @@ var mql = {
+      // The active toolbar item. All other items should be inactive under

s/item/tab/ ? The description does not match the variable name.

+++ b/core/modules/toolbar/js/toolbar.jsundefined
@@ -45,329 +51,520 @@ var mql = {
+      // Indicated whether the toolbar is positioned absolute (false) or fixed
+      // (true).
+      isPaneled: false,

"isPaneled" hardly matches the description. Can you think of a more descriptive name?

+++ b/core/modules/toolbar/js/toolbar.jsundefined
@@ -45,329 +51,520 @@ var mql = {
+      // @todo Optimize this to delay adding each subtree to the DOM until it is
+      //   needed; however, take into account screen readers for determining
+      //   when the DOM elements are needed.

… but AFAIK it is *impossible* to detect screen readers. So, do we instead mean "if we detect keyboard navigation"? Or, how is this planned to be done?

In any case, any remaining @todos must have issues for them.

+++ b/core/modules/toolbar/js/toolbar.jsundefined
@@ -45,329 +51,520 @@ var mql = {
+    /**
+     * {@inheritdoc}
+     */
+    render: function () {
+      var $body = $('body');
+      var orientation = this.checkOrientationLock(this.model.get('orientation'));
+      var isOriented = this.model.get('isOriented');
+      var isViewportOverflowConstrained = this.model.get('isViewportOverflowConstrained');
+
+      $body
+        // Apply classes to the body element that reflect the current
+        // orientation of the active toolbar.
+        .toggleClass('toolbar-vertical', (orientation === 'vertical'))
+        .toggleClass('toolbar-horizontal', (isOriented && orientation === 'horizontal'))
+        .toggleClass('toolbar-paneled', (isViewportOverflowConstrained || this.model.get('isPaneled')))
+        // Toggle the toolbar-tray-open class on the body elment. The class is
+        // applied when a toolbar tray is active.
+        .toggleClass('toolbar-tray-open', !!this.model.get('activeTray'))
+        // Apply padding to the top of the body to offset the placement of the
+        // toolbar bar element.
+        .css('padding-top', this.model.get('offsets').top);

This is all rather puzzling at first sight: why would the toolbar need to touch the body element?

I think we must very explicitly document here that the overall architecture for the toolbar's CSS requires these classes to be set on the body element. It should possibly here, and otherwise elsewhere, also explain why we let the JS handle media queries rather than the CSS.

Based on (apply from top to bottom):
37e0bf87ed682b4de3b8570416d98c226c912a87
#1847314-24: Reduce the dependency on JavaScript for the toolbar to display properly
#1938044-66: Prefix all toolbar classes to prevent theme clashes

I was going to test this patch, but it doesn't apply! :(

In the last few days, I've noticed something weird with the toolbar: after expanding an item (let's say "menu"), the drupal.announce works as expected, but Voiceover does not intercept the element that appears in the page unless you reload it. I'm not sure that this is not a bug in my setup, tough. I'm using Mac OS X 10.8.3+Safari.

@Wim Leers, @Jessebeach, what do you think about the bug I mentioned? Could it be related to the toolbar JS? If so, will it be fixed in this issue or should I file another (at that point critical) bug report against the toolbar module?

#48 addresses all questions I had in #45.

Only one point I disagree with:

What I also think to be a bug: when the viewport is as narrow as possible and the tray orientation *must* be vertical, the orientation toggle is still visible and allows me to switch the orientation to horizontal. When I do that, the end result looks broken. IMHO this should be forbidden.

I'm inclined to leave this because (1) it's not destructive and easily reversible and (2) some trays with very little content might display well on a narrow screen in a horizontal orientation.

Your points are right, but you omit one big negative point: the fact that in most cases, it looks terribly broken. IMHO it's very simple: when the media query makes the tray default to vertical orientation, then also remove the toggle completely.
Better to prevent "WTF THIS LOOKS WEIRD AND KINDA BROKEN!?!!" than to allow for a feature that would probably be used only very rarely.

+++ b/core/modules/toolbar/js/toolbar.jsundefined
@@ -235,6 +213,44 @@ Drupal.toolbar = {
+     * Call to set() are not automatically validated in Backbone 1.0. The option

This refers to Backbone 1.0, but we're using Backbone 0.9.2 in current D8.

Is this future proofing?

Also: that's a whole bunch of code. Why not just do set(attributes, {validate:true}) to avoid all this?

Right, lots of comments added. We're using JS to handle media-query handling for two reasons: (1) Using JS let's us leverage the breakpoint configurations and (2) the CSS is really complex if we try to hide some styling from browsers that don't understand media queries. If we drive the CSS from JS, the CSS becomes more robust.

I see the additional comments, but they lack the clarity that this explanation has in #48. Could you maybe make the docs in the JS read more like this explanation?

I again tested this manually, and it's still working great. The bug I found in #45 has indeed been fixed in #48.

I think this is RTBC now :)

I can't RTBC until #1938044: Prefix all toolbar classes to prevent theme clashes is committed though, until it is committed, this issue needs to continue to be marked as postponed.

#1938044: Prefix all toolbar classes to prevent theme clashes landed. Sending to testbot.

+++ b/core/modules/toolbar/css/toolbar.icons.cssundefined
@@ -1,5 +1,5 @@
- * @file toolbar.toolbar-icons.css
+ * @file toolbar.icons.css

This hunk will make the patch no longer apply to HEAD.

Fix that, and this will be RTBC.

My bad #53 was a hunk that only existed in the interdiff; it is not in the patch itself. Which means the patch should work just fine. RTBC as per #51.

#50: toolbar-backbone-1860434-50.patch queued for re-testing.

These look like legitimate test exceptions. Jesse, can you reroll?

Appeared that the toolbar_test module wasn't setting #header which was causing the tests to throw an exception. Went ahead and refactored that theme function to check if #header isset too though.

wh00ps! Didn't mean to change the logic :)

Interdiff for #59 -> #61, sorry using drush_iq... it happens sometimes :)

Here's the interdiff.txt for #50 -> #61.

+++ b/core/modules/toolbar/js/toolbar.js
@@ -324,6 +318,12 @@ Drupal.toolbar = {
+      window.setTimeout(function () {
+        Drupal.displace(true);
+      });

Delay milliseconds are required on setTimeout(), (not sure about newer browsers/js engines, but for BC this should probably be set to a minimum of 10).

window.setTimeout(function () {
  Drupal.displace(true);
}, 10);

Reviewed with Dreditor, looks good. Nothing standouts out to me, patch passed :)

Removing tag since #1800614: Improve the responsive toolbar accessibility exists.

Did some quick manual testing. Everything seems to be working fine!

Quick reviews:

+++ b/core/modules/toolbar/css/toolbar.module.cssundefined
@@ -55,7 +55,7 @@
+body.toolbar-fixed .toolbar-oriented,

@@ -69,13 +69,13 @@ body.toolbar-paneled .toolbar-oriented,
+body.toolbar-fixed .toolbar-oriented .toolbar-bar {
...
+body.toolbar-tray-open.toolbar-fixed.toolbar-vertical .toolbar-oriented {

@@ -175,13 +175,13 @@ body.toolbar-tray-open.toolbar-paneled.toolbar-vertical .toolbar-oriented {
+body.toolbar-fixed .toolbar .toolbar-tray-horizontal {
...
+body.toolbar-fixed .toolbar .toolbar-tray-vertical {

@@ -200,11 +200,11 @@ body.toolbar-paneled .toolbar .toolbar-tray-vertical {
  * the page content away from the edge of the viewport. */
...
+[dir="rtl"] body.toolbar-tray-open.toolbar-vertical.toolbar-fixed {

Can we remove "body." ?

+++ b/core/modules/toolbar/js/toolbar.jsundefined
@@ -45,329 +31,547 @@ var mql = {
+      var $tray = $();

What is it?

+++ b/core/modules/toolbar/js/toolbar.jsundefined
@@ -45,329 +31,547 @@ var mql = {
+      if ($tab.length > 0) {

`if($tab.length)` is fine ?

+++ b/core/modules/toolbar/js/toolbar.jsundefined
@@ -45,329 +31,547 @@ var mql = {
+        var id = $tab.get(0).id;

Access DOM directly ? $tab[0].id

+++ b/core/modules/toolbar/js/toolbar.jsundefined
@@ -45,329 +31,547 @@ var mql = {
+        this.$el.find('.toolbar-bar').attr('data-offset-top', '');

Not sure if it is essential. It sets an empty value.

#70: please don't do "just stopping by for a quick review" reviews on patches that are already RTBC, especially if you're not even bothering to understand what's going on.

RE: "body": this is for good reason, see the comments above.
RE: "$tray": that's for defaulting to an empty jQuery set, it gets filled later on in that function.
RE: "length > 0": besides your suggestion containing a coding standard violation, using "length > 0" is fine: it is more explicit.
RE: "$tab[0] rather than $tab.get(0)": they're identical…
RE: "empty data-offset-top": this is for good reason, see elsewhere in the patch where "data-offset-top" appears.

These sorts of reviews are useful when a patch has not yet been vetted, but at this point in the lifecycle of a patch, these superficial reviews are most unhelpful.

This looks good but can we please document the API changes in the issue summary per https://drupal.org/node/2045345. Given that we are past API freeze, API changes need to be approved and documented.

If another Javascript maintainer could review this, that would be great, but not necessary.

I reviewed a bunch of times, I'm happy with the code overall.

I'd agree with droplet that tray = $() isn't needed here. It was relevant in previous versions of the code but now the patch sets $tray = this.$el.find('[data-toolbar-tray="' + name + '"].toolbar-tray'); in all cases so the initialisation is not needed.

for .length and .get(0), it's a little different from the rest of core but it's valid use of jQuery API so no big deal.

for the data-offset-top, Drupal.displace uses the fact that the attribute exists and figure out itself the height of the element so even an empty attibute is used. I'm not really found of the behavior but it was discussed in another issue.

maybe another minor nitpick:

  mediaQueryChangeHandler: function (model, label, mql) {
    switch (label) {
      case 'module.toolbar.narrow':
        model.set({
          'isOriented': mql.matches,
          'isTrayToggleVisible': false
        });
        // If the toolbar doesn't have an explicit orientation yet, or if the
        // narrow media query doesn't match then set the orientation to
        // vertical.
        if (!mql.matches || !model.get('orientation')) {
          model.set({'orientation': 'vertical'}, {validate: true});
        }
        break;
      case 'module.toolbar.standard':
        model.set({
          'isFixed': mql.matches
        });
        break;
      case 'module.toolbar.wide':
        model.set({
          'orientation': ((mql.matches) ? 'horizontal' : 'vertical')
        }, {validate: true});
        // The tray orientation toggle visibility does not need to be validated.
        model.set({
          'isTrayToggleVisible': mql.matches
        });
        break;
      default:
        break;
    }
  },

it ought to be an if/else instead of a switch.

RTBC +1

Thanks @nod_! Do you think the minor cleanups of #74 merit a followup?

We just need the API changes documented, then.

It's either in this patch or we forget about it.

added some use case notes.

Issue summary was updated by @jessebeach, no API changes.

+++ b/core/modules/toolbar/js/toolbar.jsundefined
@@ -45,329 +31,547 @@ var mql = {
+      var $tray = $();

If this is unnecessary as suggested by #70 and #74 let's remove it... extra lines of code need to maintained and in this case delivered to the client...

Alright. I went ahead and removed it and initalized it farther down. TBH it really doesn't matter and this is some serious over-nitpicking. The amount of "client performance" saved by this, is extremely negligible. This issue has been set as RTBC several times now with minor (uncessary) nitpicks, setting back to RTBC. As far as the if/else/switch issues, it's fine the way it is (more than two and less than 10 use switch: http://oreilly.com/server-administration/excerpts/even-faster-websites/w...). No need to change it.

Committed f6ac79e and pushed to 8.x. Thanks!

+ opened: Drupal.t('opened'),
+ closed: Drupal.t('closed'),

Need to be Ucfirst per #421118: [Meta] Standardize capitalization on actions and consistency.

But "opened" and "closed" aren't actions, they're states. Here's the code that uses those strings:

onActiveTrayChange: function (model, tray) {
      var relevantTray = (tray === null) ? model.previous('activeTray') : tray;
      var state = (tray === null) ? this.strings.closed : this.strings.opened;
      Drupal.announce(Drupal.t('"@tray" tray @state.', {
        '@tray': relevantTray.querySelector('.toolbar-tray-name').textContent,
        '@state': state
      }));
    }

In other words "Foo tray opened." and "Foo tray closed.".

Do not be so nitpicking, please. It's not really about actions only. We are changing all lcfirst strings. :-)

In other words "Foo tray opened." and "Foo tray closed.".

That's not the point. A string Foo tray opened is ucfirst and therefore correct. A string like foo tray opened needs work, too.

@effulgentsia is correct. #421118: [Meta] Standardize capitalization on actions is about single worded "actions" or "operations" (commonly found in tables), this is a completely different scenario and is located towards the end of a string where it shouldn't be ucfirst. If you really feel this should be done differently, we can can come up with a different solution in a separate issue. This was already committed in #80.

Sigh, I retract my statement, @nod_ pointed out that this is only used for accessibility (screen-readers) so it really doesn't matter if it's capitalized in the middle. Created new issue to follow up this "bug": #2052973: Improve translatability of strings in toolbar.js.

I don't like to translate both and this is why we need to be consistent.

.