Toolbar js errors when you remove the administration menu

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 🐛 Toggle icon for toolbar menu missing for all tabs except Manage Needs work

yookoala → changed the visibility of the branch 3310075-toolbar-js-errors to hidden.

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.

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

#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?

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 we can provide test coverage for the bug.