Toolbar js errors when you remove the administration menu

@redamakhchan are you sure it is related to the Admin Toolbar module? As the first proposed resolution it involves the core Toolbar.

This looks like an issue introduced by custom code that we could make toolbar more resilient to, which isn't a critical bug.

It might be a duplication of https://www.drupal.org/project/drupal/issues/3305152

The fix has implemented to both 10.4.x and 11.x branch. Please review.

Issue summary mentions 2 solutions but no indication for which solution was chosen.

Not sure about the use case but if it seems like something people do will need test coverage I believe.

Verified the MR !7934 has fixed the Issue. Not changing the issue status until the approach is decided as per #13. Hopes the MR uses approach 2 to solve the issue for specific Menu types.

Before

After

I'd argue approach #1 and #2 are not mutually exclusive. While approach #2 is more of a user experience concern, approach #1 makes the code more resilient and robust either way.

I think that approach #1 is appropriate to fix a JS error. Approach #2 seems like more of a feature request, even if it was previously a feature. Let's not let the possibility of a more robust feature block a simple JS fix for now.

@smustgrave Is it really necessary to add a test based on the addition of an if statement to check if a JS variable is set?

I've run into this issue on a site where a restricted role uses the front-end theme. The toolbar loads, but .toolbar-loading is never removed, and the toolbar appears broken.

After debugging, I confirmed the crash happens in ToolbarVisualView.js when the orientation toggle button is missing:

$orientationToggleButton[0].value

Adding a simple guard resolves the issue cleanly:

// If the toggle button isn't present, exit early to avoid JS crash.
if (!$orientationToggleButton.length) {
  return;
}

This allows the rest of the toolbar JS to run, removes .toolbar-loading, and restores full toolbar visibility for the restricted role.

Setting back to needs review to see if we can get this fix in without adding a test.

#17 is helpful. If #17 is an accurate description of what happens (in all cases) where this issue occurs then test coverage should be simple. There is a FunctionalJavascript test in the toolbar core module named TestIntegration.php? (or similar). The last test function in that file tests the button toggling behaviour and appears relevant to this issue. It looks like we can add an assertion or two to test that the

.toolbar-loading

class described in #17 is removed as it seems (going by #17) it needs to be from the DOM. Testing for console errors would surely be a waste of time because such tests accomplish nothing. But if there is a discernible change to the DOM, however slight, so long as it happens consistently without the fix and the change to the DOM is consistently different as a result of the fix, then test coverage can and should be implemented as normal..

I looked at the Nighwatch tests but not sure that is the place to add new assertions. the FunctionalJavascript test looks like the best place.

Regarding #17 I do not see any changes to the DOM in the before and after screenshots nor signs of breakage to the UI? I know the screenshots were submitted by another user. Just wondering if the screenshots are based on a different manner of reproducing the issue? We need to be sure that the class is present then removed in a consistent way when this issue is reproduced. More step-by-step details on how to reproduce the error and if possible to incorporate #17 in the issue description would help so long as #17 is the right way to reproduce the bug.

Maybe we can get more screenshots showing the browser DOM inspector rather than the console and showing the class present before. the fix and missing after it?

BTW if #17 is an accurate reproduction of the bug/ issue then Proposed resultion no.1 is surely the right one. A follow-up issue can deal with no.2. It seems way outside the scope of this issue: especially if #17 is accurate and, as a result, we can provide test coverage for the bug.

Seems consensus may be for test coverage. Also reading the summary again what's the scenario for calling unset($items['administration']); ?

The 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.

Updating tags per Issue tags field and Issue tags -- special tags and for issue #3565085: Drupal core issue tag cleanup.

Toolbar has moved to contrib