Problem/Motivation

The user menu link of the toolbar is not working properly with the BigPipe module. I installed Drupal 8.5.0 with the standard profile and when I click the user menu link of the toolbar the page refreshes instead of opening the toolbar menu tray. When BigPipe is not installed everything is working fine.

When BigPipe is enabled, the toolbar username/My account tab title is rendered via #lazy_builder => ['user.toolbar_link_builder:renderDisplayName', []], (line 1373 of user.module) which, once rendered, wraps the username in a div tag. This new child element of the anchor tag prevents the toolbar menu tray from opening when directly clicked on. Clicking around the username text works since that bypasses the div element.

Proposed resolution

The JavaScript to trigger the menu tray opening uses event.target (which is the div tag when clicking on the username text) instead of event.currentTarget (the anchor tag). Changing the target fixes the issue and the tab behaves like all other toolbar tabs with menu trays whether children exist or not.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#18 2952626-18.patch1.52 KBtstoeckler
#7 toolbar-username-reloads-page-2952626-7.patch1.57 KBDaniel Korte
#2 drupal850bigpipe.gif284.56 KBromainj
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

romainj created an issue. See original summary.

romainj’s picture

romainj CreditAttribution: romainj as a volunteer commented
FileSize
drupal850bigpipe.gif284.56 KB
cilefen’s picture

cilefen CreditAttribution: cilefen at Institute for Advanced Study commented
Issue summary: View changes
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Assigned: Unassigned » Wim Leers

Thanks for reporting! I will look into this.

Daniel Korte’s picture

Daniel Korte
he/him
Primary language English
Location Brooklyn, NY
CreditAttribution: Daniel Korte commented

I missed this issue because it's an issue with the toolbar module rather than BigPipe, but they are related. Thanks @cilefen

I created a patch for this issue here: https://www.drupal.org/project/drupal/issues/2957975

cilefen’s picture

cilefen CreditAttribution: cilefen at Institute for Advanced Study commented
Component: big_pipe.module » toolbar.module

I duplicated the other issue and based on its patch, this is toolbar module.

Daniel Korte’s picture

Daniel Korte
he/him
Primary language English
Location Brooklyn, NY
CreditAttribution: Daniel Korte commented
Version: 8.5.0 » 8.5.1
Issue summary: View changes
Status: Active » Needs review
FileSize
toolbar-username-reloads-page-2952626-7.patch1.57 KB
cilefen’s picture

cilefen CreditAttribution: cilefen at Institute for Advanced Study commented
Issue tags: +JavaScript
slasher13’s picture

slasher13
Primary language German
CreditAttribution: slasher13 as a volunteer and at Wolters Kluwer commented
Status: Needs review » Reviewed & tested by the community

Applied patch and it works!

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Related issues: +#736066: ajax.js insert command sometimes wraps content in a div, potentially producing invalid HTML and other bugs
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: The user menu link of the toolbar not working with Bigpipe » The user menu link of the toolbar not working
Assigned: Wim Leers » Unassigned

Ahhhh, that makes much more sense! I'm glad it wasn't a BigPipe bug after all :)

romainj’s picture

romainj CreditAttribution: romainj as a volunteer commented

Many thanks to all of you.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Yeah I noticed that this was broken. Do we know what broke it? Also given that this is something we can break we should have a JavascriptTestBase test for it. Probably can add something to ToolbarIntegrationTest.

In order to commit a bug fix we need an automated to test to prove that we've fixed the bug and ensure that we don't break it again in the future. For more information about writing tests in Drupal 8 see the following links:

  1. https://www.drupal.org/docs/8/testing
  2. https://api.drupal.org/api/drupal/core%21core.api.php/group/testing/8.5.x
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and commented
Version: 8.5.1 » 8.5.x-dev
John Pitcairn’s picture

John Pitcairn CreditAttribution: John Pitcairn commented
Version: 8.5.x-dev » 8.6.x-dev

Gonna need a re-roll for 8.6 I think.

Hmm ... I can't reproduce the problem in 8.6.0-rc1. Looks like #736066: ajax.js insert command sometimes wraps content in a div, potentially producing invalid HTML and other bugs might have fixed it?

John Pitcairn’s picture

John Pitcairn CreditAttribution: John Pitcairn commented
Version: 8.6.x-dev » 8.5.x-dev
Daniel Korte’s picture

Daniel Korte
he/him
Primary language English
Location Brooklyn, NY
CreditAttribution: Daniel Korte commented
Version: 8.5.x-dev » 8.6.x-dev

I haven’t been able to test in 8.6.0-rc1, but either way the current implementation of event.target is incorrect. This needs to use event.currentTarget since the actions should be performed on the anchor tag and not any child of it, such as a SVG icon or a div.

  • target is the element that triggered the event (e.g., the user clicked on)
  • currentTarget is the element that the event listener is attached to.

via Stack Overflow

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
FileSize
2952626-18.patch1.52 KB

Here's a re-roll.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

As #15 said, maybe the change still makes sense, but I'd actually expect that the div is no longer there now per the issue referenced in #15.

Daniel Korte’s picture

Daniel Korte
he/him
Primary language English
Location Brooklyn, NY
CreditAttribution: Daniel Korte commented

The div is not there, but that is no reason to leave shaky code in place IMHO.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Title: The user menu link of the toolbar not working » Toolbar tab click potentially accessing wrong element due to JavaScript event delegation
Version: 8.6.x-dev » 8.7.x-dev
Category: Bug report » Task
Issue tags: -Needs tests

I tried to rewrite the title since the actual user-facing bug has been fixed. It probably still makes sense to make this change there shouldn't be any downsides in making it.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs work » Needs review

This probably should be moved back to needs review status since I removed the needs tests tag in the previous comment.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs review » Reviewed & tested by the community

The change looks good for me so I'm moving this to RTBC.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented

👍 from me!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed d2989e4 and pushed to 8.7.x. Thanks!

Crediting @John Pitcairn for finding out that the bug has been fixed and referencing the issue that fixed it.

  • alexpott committed d2989e4 on 8.7.x 
    Issue #2952626 by Daniel Korte, tstoeckler, romainj, John Pitcairn:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.