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.
| Comment | File | Size | Author |
|---|---|---|---|
| #43 | interdiff_39_43.txt | 861 bytes | rishabh vishwakarma |
| #43 | 2885755-43.patch | 3.99 KB | rishabh vishwakarma |
| #39 | 2885755-39.patch | 4.54 KB | gauravvvv |
| #38 | 2885755-nr-bot.txt | 144 bytes | needs-review-queue-bot |
| #36 | 2885755-36.patch | 5.42 KB | smustgrave |
Comments
Comment #2
wim leersComment #3
GrandmaGlassesRopeManComment #4
droplet commentedHmm.. 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.
Comment #6
wim leersSame patch as #2, but for 8.3.x.
Comment #7
yogeshmpawarTriggering Bots.
Comment #10
GrandmaGlassesRopeMan- 8.5.x reroll
Comment #11
GrandmaGlassesRopeManComment #13
GrandmaGlassesRopeMan- Find the first item and make sure it has a tray to be opened,
data-toolbar-tray.Comment #14
wim leersWe 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.This looks very weird and brittle, but I'm guessing it's necessary?
I think this might be the scariest line of code I've seen in years… 😱 Yes, it's in compiled code. But still.
Comment #15
dawehnerI 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 === undefinedOn top of this question, here is also a test.
Comment #17
wim leers👏
übernit: s/administration/administration toolbar tab/
Comment #18
GrandmaGlassesRopeMan- The
.attr()method returns a cross browserundefinedfor attributes that have not been set. jQuery Docs. No more scary symbol typeof helper.- Fix übernit.
Comment #19
wim leers👍
Comment #20
gábor hojtsyImprovement looks good, thanks! Only two small things I found:
Can we somehow make this more clear? An item being an item, is well, what an item is.
not => no
Comment #21
GrandmaGlassesRopeMan- #20.1 - I think this is more clear now?
- #20.2 - 👍🏿
Comment #31
smustgrave commentedComment #32
karishmaamin commentedRe-rolled patch againt 9.5.x. Please review
Comment #33
smustgrave commentedLooks like an issue with the patch
Comment #34
akashkumar07 commentedFixing the reported prettier/prettier issue.
Comment #35
smustgrave commentedLooks like the patch is still having issues.
You can run core/scripts/dev/commit-code-check.sh to check the code before making the patch.
Comment #36
smustgrave commentedFixed build errors of #34
Comment #38
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #39
gauravvvv commentedRe-rolled patch for 10.1.x, Please review
Comment #40
borisson_This was already rtbc in #19 and the 2 things that were requested by @Gábor Hojtsy have been fixed. Looks good to me.
Comment #41
luenemannI guess it's not about tour.module.
Comment #42
longwaveStray debug code in the test.
Comment #43
rishabh vishwakarma commentedRemoved the extra code as suggested in #42
Comment #44
smustgrave commentedComment #48
quietone commentedThe Toolbar Module was approved for removal in #3476882: [Policy] Move Toolbar module to contrib.
This is Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.
The deprecation work is in #3484850: [meta] Tasks to deprecate Toolbar module and the removal work in #3488828: [meta] Tasks to remove Toolbar module.
Toolbar will be moved to a contributed project before Drupal 12.0.0 is released.