Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JavaScript error if dropbutton library is loaded when there are no dropbuttons on the page #6257

Closed
argiepiano opened this issue Oct 8, 2023 · 6 comments · Fixed by backdrop/backdrop#4538

Comments

@argiepiano
Copy link

argiepiano commented Oct 8, 2023

Description of the bug

This bug happens in combination with #6256. As reported there, the dropbutton JS library is loaded even when the Manage taxonomies Dashboard block is not actually rendered.

When the dropbutton library is loaded, and when there are no dropbuttons on the page, there is a JS failure in the newly introduced method Backdrop.behaviors.dropButtonWidths - see #6180.

This was first reported in Zulip

Steps To Reproduce

To reproduce this, we'll force an error that happens because of the combination of this bug, and another reported in #6256.

To reproduce the behavior:

  1. Be sure you have a vanilla, clean install of Backdrop 1.26.1
  2. Create an Editor user
  3. Remove all taxonomy privileges from the Editor role
  4. Log in as Editor, head to the Dashboard, and inspect the browser dev console
  5. You should see jQuery.Deferred exception: Cannot read properties of undefined (reading 'split') TypeError: Cannot read properties of undefined (reading 'split')

The admin bar stops working, since JS crashed

Actual behavior

JS crashes, admin bar stops working.

Expected behavior

Things should still work even if there are no dropbuttons in the page.

Additional information

Add any other information that could help, such as:

  • Backdrop CMS version: 1.26.1
  • PHP version: 8.1
  • Browser(s) and their versions: Chrome latest

I believe the solution is simple: after doing this in that method

    var link = $('.dropbutton li a').first();

We should check to see if link actually has content.

By the way, following an unwritten Backdrop convention, the variable should be $link, not link, since this holds a jQuery object.

@olafgrabienski
Copy link

@argiepiano thank you for filing the report, and @bugfolder thanks for the quick pull request! I've applied the PR to the site where I discovered the bug, and it fixes the issue for me.

@avpaderno
Copy link
Member

As a side note, The JavaScript variable names to use are described in JavaScript Coding Standards / Variables.

Prefix variables that point to jQuery objects with a dollar sign ($). In any part of your code it should be easy to understand which variables are jQuery objects and which are not.

It is a rather written Backdrop convention.

@bugfolder
Copy link

@kiamlaluno, would you be willing to do the code review on this PR?

@avpaderno
Copy link
Member

For what I can see, the code is correct. It also uses length(), which is correct, as size() has been deprecated since jQuery 3.0 and length() exists since jQuery 1.0.

@argiepiano
Copy link
Author

Changing the label to reflect the code review. A small sidenote: there is no length() method in jQuery. There is a length property of the object, which, as @kiamlaluno pointed out, is the correct way to check.

@quicksketch
Copy link
Member

Thanks @bugfolder, @argiepiano, @olafgrabienski, and @kiamlaluno! I have merged backdrop/backdrop#4538 into 1.x and 1.26.x.

@quicksketch quicksketch changed the title Regression: JS libraries crash if dropbutton library is loaded when there are no dropbuttons on the page JavaScript error if dropbutton library is loaded when there are no dropbuttons on the page Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants