Problem/Motivation

If you customize the toolbar (for a particular site or a distribution), and the first toolbar item is not a tab, then toolbar.js will always mark the first toolbar item as being active.

hook_toolbar() (see core/modules/toolbar/toolbar.api.php) speaks of toolbar items. But then core/modules/toolbar/templates/toolbar.html.twig marks every toolbar item as a tab:

    {% for key, tab in tabs %}
      {% set tray = trays[key] %}
      <div{{ tab.attributes.addClass('toolbar-tab') }}>

This is wrong.

Worse, core/modules/toolbar/js/toolbar.js then does this:

        // If the toolbar's orientation is horizontal and no active tab is
        // defined then show the tray of the first toolbar tab by default (but
        // not the first 'Home' toolbar tab).
        if (Drupal.toolbar.models.toolbarModel.get('orientation') === 'horizontal' && Drupal.toolbar.models.toolbarModel.get('activeTab') === null) {
          Drupal.toolbar.models.toolbarModel.set({
            activeTab: $('.toolbar-bar .toolbar-tab:not(.home-toolbar-tab) a').get(0)
          });
        }

This means that by default, if there is no active tab, it will automatically set the first toolbar item as active. Even if it's just a link, and not actually a tab.

This means that if the first toolbar item is a link, it is automatically marked as active, and will continue to be so until the first "actual" tab (with a tray, e.g. the "user" tab+tray) is clicked. But until you do, that means the first toolbar item (a link) will remain active, even while you click other toolbar items that are links, to navigate around. This is super confusing behavior.

Proposed resolution

  1. Better distinguish between "tab" items and "link" items.
  2. Stop assuming the default toolbar in the codebase.

Remaining tasks

User interface changes

API changes

Data model changes

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Status: Active » Needs review
FileSize
1.72 KB
drpal’s picture

Issue tags: +JavaScript
droplet’s picture

hook_toolbar() (see core/modules/toolbar/toolbar.api.php) speaks of toolbar items. But then core/modules/toolbar/templates/toolbar.html.twig marks every toolbar item as a tab:

Hmm.. can be improved but not a problem for most CSS guy I think. `.toolbar-tab` is a helper class for tab-like style. And for the customized site, changing the Twig markup to remove `.toolbar-tab` for LINK also makes sense. (Toolbar default to support Tab-like style only)

The patch removed the default active Tab. I'm not so sure it's a Bug or Task. (Or incomplete patch?) I don't like its default to active also but I thought the UX guy has diff thoughts...

** Note: I can see the buggy on default active tab to `$('.toolbar-bar .toolbar-tab:not(.home-toolbar-tab) a').get(0)` on frontend is a problem. But @wim's IS mixed few requests as I seen.

Status: Needs review » Needs work

The last submitted patch, 2: 2885755-2.patch, failed testing. View results

Wim Leers’s picture

Yogesh Pawar’s picture

Status: Needs work » Needs review

Triggering Bots.

Status: Needs review » Needs work

The last submitted patch, 6: 2885755-2-8.3.x.patch, failed testing. View results

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

drpal’s picture

- 8.5.x reroll

drpal’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 10: 2885755-10.patch, failed testing. View results

drpal’s picture

Status: Needs work » Needs review
FileSize
2.42 KB
2.38 KB

- Find the first item and make sure it has a tray to be opened, data-toolbar-tray.

Wim Leers’s picture

  1. +++ b/core/modules/toolbar/js/toolbar.es6.js
    @@ -136,6 +136,19 @@
    +        if (Drupal.toolbar.models.toolbarModel.get('orientation') === 'horizontal' && Drupal.toolbar.models.toolbarModel.get('activeTab') === null) {
    

    We shouldn't special case .home-toolbar-tab. It should definitely be possible to not special case it, because that one doesn't even have a tray.

  2. +++ b/core/modules/toolbar/js/toolbar.es6.js
    @@ -136,6 +136,19 @@
    +          if (typeof attr !== typeof undefined && attr !== false) {
    

    This looks very weird and brittle, but I'm guessing it's necessary?

  3. +++ b/core/modules/toolbar/js/toolbar.js
    @@ -93,6 +94,16 @@
    +          if ((typeof attr === 'undefined' ? 'undefined' : _typeof(attr)) !== (typeof undefined === 'undefined' ? 'undefined' : _typeof(undefined)) && attr !== false) {
    

    I think this might be the scariest line of code I've seen in years… 😱 Yes, it's in compiled code. But still.

dawehner’s picture

+++ b/core/modules/toolbar/js/toolbar.es6.js
@@ -136,6 +136,19 @@
+          if (typeof attr !== typeof undefined && attr !== false) {

I was wondering: is this the only/best way to check for typeof. According to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operat... the only value which returns undefined for typeof is undefed, so why can't you use attr === undefined

On top of this question, here is also a test.

The last submitted patch, 15: 2885755-14-test.patch, failed testing. View results

Wim Leers’s picture

On top of this question, here is also a test.

👏

+++ b/core/modules/toolbar/tests/src/FunctionalJavascript/ToolbarIntegrationTest.php
@@ -30,6 +30,11 @@ public function testToolbarToggling() {
+    // Ensure that the administration is active by default.

übernit: s/administration/administration toolbar tab/

drpal’s picture

- The .attr() method returns a cross browser undefined for attributes that have not been set. jQuery Docs. No more scary symbol typeof helper.
- Fix übernit.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

👍

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

Improvement looks good, thanks! Only two small things I found:

  1. +++ b/core/modules/toolbar/tests/src/FunctionalJavascript/ToolbarIntegrationTest.php
    @@ -48,4 +54,25 @@ public function testToolbarToggling() {
    +   * Tests a toolbar with the first item being only an item.
    

    Can we somehow make this more clear? An item being an item, is well, what an item is.

  2. +++ b/core/modules/toolbar/tests/src/FunctionalJavascript/ToolbarIntegrationTest.php
    @@ -48,4 +54,25 @@ public function testToolbarToggling() {
    +    // Ensure that there is not active tab by default.
    

    not => no

drpal’s picture

Status: Needs work » Needs review
FileSize
5.44 KB
916 bytes

- #20.1 - I think this is more clear now?
- #20.2 - 👍🏿

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.