Vertical tabs result in jQuery error when overlay-context hashtag is added to URL

I have created a patch with the fix.

Thanks, approach makes sense to me. Just one nitpick:

+++ b/misc/vertical-tabs.js	(date 1684929665835)
@@ -50,8 +50,9 @@
+        var hash = window.location.hash.replace(/[=%;,\/]/g, "");

Please add a code comment here like "// Remove any characters that jquery does not accept in find()."

After that I think we can commit this. I don't see a good way how we would write an automated test for this, so I think that is fine.

Updated patch #2 by addressing #4, please review it.

Thanks!

Thanks, then I think we are ready to commit this.

Thanks for working on this! I have tested the patch manually and it seems to work correctly.

For the record, I was able to simulate the JS error most easily by these steps (but there are probably other ways to run into this):

1. clean D7 install with Bartik theme
2. enable the Overlay module and jQuery Update module
3. change default jQuery for frontend theme to 1.12
4. disable Use the administration theme when editing or creating content in appearance settings (admin/appearance)
5. create a node and visit a link like node/1/edit#overlay-context=node/1/edit (you can end up here when clicking thru overlay, but I think it is sufficient to open the link manually for this testing)
6. the error will appear in the console

After applying the patch the error is gone. Adding a tag for a final review.

Initially I was worried about window.location.hash.replace without checking the window.location.hash first, but it seems like it is not a case and that window.location.hash will return an empty string in case that it is not in URL, see: https://developer.mozilla.org/en-US/docs/Web/API/Location/hash

Adding the comment to explain the removal of the characters is good.

However, the patch also removes the comment that explains what this code is actually doing.

I'd think we want to combine the two.

We could probably do this on commit.

There's also an argument that this belongs in jquery_update rather than Drupal core if the error doesn't come about with core's default jQuery.

I restored the original comment on commit.

Thanks everybody!

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