fix: replace calls to toggle(fn, fn) with click(fn)

[trivikr]

Background

The API .toggle(fn, fn) was removed in jQuery v1.9.x

While bumping jQuery to v3.6.0 in #1397, we noticed that jquery-migrate does not provide 100% wrapper for .toggle(fn, fn)

• API reference preview: https://lsegal-yard-jquery-bump-no-edits.netlify.app/

• TOC hides on load

  toc-hide-on-load.mov

Discussion in comment: #1397 (comment)

Description

This PR changes calls of .toggle(fn, fn) with .click(fn) so that the users who want to upgrade jQuery can replace jquery.js vendored by YARD.

Testing

I verified that replacing toggle with click does not break existing functionality in jQuery 1.7.1

API reference preview: https://lsegal-yard-replace-toggle-with-click.netlify.app/

• The jQuery version is "1.7.1", verified by running $.fn.jquery in Console.
• Screen recordings:
  • Table of Contents on landing page

    TOC.mov
  • Inheritance tree on Array page.

    inheritanceTree.mov
  • Source above footer on Array page.

    Source.mov
  • Defines on Module: YARD::Handlers::Ruby page.

    toggleDefines.mov

Completed Tasks

• I have read the Contributing Guide.
• The pull request is complete (implemented / written).
• Git commits have been cleaned up (squash WIP / revert commits).
• I wrote tests and ran bundle exec rake locally (if code is attached to PR).

[trivikr]

@lsegal Any update on this PR?

We had a detailed discussion in other PR on why replacing calls to .toggle(fn, fn) are required to be forward compatible, if downstream users want to override jQuery version used by YARD #1397 (comment)

I've also provided details in the Background and Description section of this PR.

[lsegal]

As pointed out in #1397, we're still very much not on the same page about expectations for this PR.

As you've mentioned, this code allows users to manually patch YARD via template customization downstream in order to workaround the issue described in the related ticket (as well as earlier jQuery CVE issues). This doesn't make sense to me, since it doesn't solve the root issue of upgrading jQuery, rather it puts the burden on downstream users to fix this problem. Your codebase may have solved the rest of the issue, but other users of YARD have not necessarily done so, and so the burden for those users remains, meaning pulling this PR and releasing a hotfix would do nothing to directly help those users who would still need to go through the trouble of creating custom templates. Frankly, I can't imagine how I would word such a changelog entry that wouldn't be interpreted as YARD passing the buck onto the end-user to fix the root issue.

I've also already outlined the downsides of pulling this fix into YARD in the related ticket, but I will reiterate here: Primarily, the issue with this code for me is that this hides the fact that YARD has a backward compatible requirement on the jQuery 1.3 API regardless of what APIs the YARD core templates use. Pulling this code in would create the false sense that we can simply vendor the latest jQuery without dealing with the backward compatibility question. Keeping functions like toggle() force us to continue to consider the required support for those APIs.

FWIW I had already provided a code review for this exact code in #1397: asked why this change was needed and the answer seemed to imply that it was simply needed for downstream use. That's not a sufficient reason for a PR to YARD's repository.

Downstream code is not YARD's maintenance burden. If downstream users are making customizations, it's the responsibility of those downstream codebases to provide those patches. Again, the reason downstream users need changes is not because of toggle(), but because of the jQuery version. That should really be the focus of all of this. If the goal is to improve YARD as a whole, we should be doing the thing that provides the best experience users. Replacing toggle doesn't do that.

Quite simply: the fix to an old jQuery is to upgrade jQuery. Unfortunately this has to be done in a backward compatible way for YARD to not break downstream users.

I would say that if you require this code in your repository, you can apply it via a patch, or, better yet, create your own template customization that replaces toggle. This is actually fairly easy to do (by replacing app.js), something you're probably already aware of, since presumably you are already vendoring an upgraded jQuery. You could simply import this app.js file into your repository to resolve the issue fully on your end.

I'm going to mark this as closed since the plan is to continue supporting the toggle() APIs provided by jQuery, including within YARD's own codebase, until a major version bump is made that allows us to break compatibility.

Finally as a sidenote, although this probably belongs more clearly inside the discussion in #1397, I want to point out that though I can't claim that it's "zero", the severity impact of the jQuery CVEs in the context of YARD's own usage is marginal at best. The reality is that there are roughly 9 CVE items that could theoretically affect YARD, the vast majority of which are XSS / remote execution and require some form of user input or remote requests. YARD itself, however, does not perform any external requests, nor does it store or manipulate sensitive data or user input in a way that could be exploited maliciously. In other words, these are fundamentally pre-generated and completely insulated read-only pages specifically designed to work "offline". Now, downstream usage certainly can have a wider surface area of attack, and there is a lot YARD can do to make those downstream users better off, which is why upgrading jQuery in YARD itself is really the only valuable course of action here. Everything else can and should be handled by downstream usage.

Hope this all makes sense.