In file toolbar.menu.js line 137 you find:
// Bind event handlers.
$(document)
.on('click.toolbar', '.toolbar-handle', toggleClickHandler);
Currently this binding is executed once only and so there is no problem. With multiple toolbars however the handler toggleClickHandler is registered multiple and on click executed multiple.
jQuery('.some-wrapper').drupalToolbarMenu();<div class="some-wrapper">
<ul class="toolbar-menu">
<li><a href="node/1">Menu item 1</a>
<ul class="toolbar-menu">
<li><a href="node/1">Sub menu item 1</a></li>
<li><a href="node/2">Sub menu item 2</a></li>
</ul>
</li>
<li><a href="node/2">Menu item 2</a></li>
<li><a href="node/1">Menu item 3</a>
<ul class="toolbar-menu">
<li><a href="node/1">Sub menu</a></li>
<li><a href="node/2">Another sub menu</a></li>
</ul>
</li>
</ul>
</div>
Proposed code:
// Bind event handlers.
$(this)
.on('click.toolbar', '.toolbar-handle', toggleClickHandler);
With this little change my attempts work fine. Maybe it causes another problem I do not see.
| Task | Novice task? | Contributor instructions | Complete? |
|---|---|---|---|
| Create a patch | Instructions | Done | |
| Update the issue summary | Instructions | Done | |
| Update the issue summary noting if allowed during the beta | Instructions | ||
| Manually test the patch | Novice | Instructions | Done |
| Add steps to reproduce the issue | Novice | Instructions | Done |
| Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards | Instructions | Done |
None.
None.
None.
| Issue category | Bug because our jQuery plugin is not reusable |
|---|---|
| Issue priority | Normal because there is no API change |
| Unfrozen changes | Unfrozen because it only changes JS and make code reusable |
| Comment | File | Size | Author |
|---|---|---|---|
| #22 | core-js-toolbar-menu-2341267-22.patch | 518 bytes | nod_ |
| #9 | interdiff-2341267-3-8.txt | 866 bytes | tohesi |
| #8 | bind_event_handlers_for-2341267-8.patch | 870 bytes | tohesi |
| #3 | core-js-toolbarmenu-2341267-3.patch | 436 bytes | nod_ |
Comments
Comment #1
nod_I don't understand what you mean by multiple toolbars. There should be only one anyway, no?
Comment #2
gendoas commentedHi nod_, I no longer expected a reaction.
I'm creating some more toolbars like the admin toolbar in my web-app using drupal's toolbar, I do not want to explain why. For each of this toolbars $.fn.drupalToolbarMenu is called. Because of '$(document).on('click.toolbar', '.toolbar-handle', ...' (toolbar.menu.js, line 137) the handler is registered and so executed multiple. On my installation I changed it to '$(this).on('click.toolbar', '.toolbar-handle', ...'. This solves my problems and I don't see any problem with this change, maybe you do.
For me it is ok to hack toolbar on my installation, but maybe I will publish my app sometimes, and then it would be better, if there were no need to hack. For now I'm far away from publishing the app because it is far away from being worth to publish it.
Comment #3
nod_My comment was eaten so I'll give the summary.
I agree, this is even a bug because our code is not reusable and our jquery plugin assumes it's only used once on a page.
Comment #4
borisson_This is a bug so according to the beta evaluation we should allow this change.
I tested this is on a local install and it doesn't break the current functionality.
Comment #5
droplet commentedumm.. put it inside initItems() is a better place I think.
Above change will work on case like:
but won't work in following case perfectly I think
Or simply calling `$('.first-toolbar-menu').drupalToolbarMenu()` twice it won't work anymore
Comment #6
droplet commentedComment #7
tohesi commentedI'll start working with this in DrupalCon Barcelona, mentored by DuaelFr. Following the workflow as guided in https://www.drupal.org/contributor-tasks/create-patch.
Comment #8
tohesi commentedMoved the event handler binding inside the jQuery.each method. Didn't put it in initItems() because the context didn't quite feel right.
Comment #9
tohesi commentedFixing the interdiff file name.
Comment #11
duaelfrI'm mentoring this issue at DrupalCon Barcelona :)
Comment #13
tohesi commentedComment #14
bartvig commentedI'm working on this now at DrupalCon Barcelona, trying to reproduce the issue.
Comment #15
bartvig commentedI updated the issue summary, added the step to reproduce, and added the table of remaining tasks.
Comment #16
gaas commentedI'm reviewing the proposed patch (#8) at DrupalCon Barcelona. Following the guidelines in https://www.drupal.org/contributor-tasks/review.
Comment #17
gaas commentedI've reproduced the failure using the steps in the summary without the patch and seen that the patch makes the admin menu still functional after the second toolbar is intialized. I've inspected the patch and agree that it works because it registers handlers separately for each menu. Alternative approach could be to keep a single handler at the document level, but protect the initialization with
once()to make sure it's not registered multiple times.Comment #18
gaas commentedComment #19
duaelfrEverything looks good here now.
Removing the Needs manual testing tag as it has been done.
Thank you all for your work!
Comment #20
alexpottCommitted e8b5f46 and pushed to 8.0.x. Thanks!
Comment #22
nod_Nitpick. That's why we prefix jquery stuff with $, so we don't need extra jquery nesting.
Comment #23
alexpott@nod_ good to know. Committed ede4d4a and pushed to 8.0.x. Thanks!