Clean up toggle logic in rustdoc

[Manishearth]

I was working on #82114 and discovered that the toggle logic is written scattered all throughout main.js. This seems suboptimal; I'd rather not be generating HTML in main.js. A lot of the toggle logic does things like check the classes of nearby items, etc, which seems super brittle.

I propose we move the toggle logic to Rust code instead; writing a wrapToggle() method that wraps some HTML in a toggle.

Thoughts? @rust-lang/rustdoc

Might be worth creating a meta issue for reducing the size of main.js in general. We seem to do a fair amount of additional codegen there, not just for toggles.

[Manishearth]

Also the way toggles are handled is a global click event that bubbles down. There's a very complicated collapseDocs function that has a whole bunch of special logic, and furthermore the global click event handler basically reimplements a broken version of bubbling.

It feels to me that adding a toggle should just be a matter of wrapping the relevant block in a toggle div + toggle, and at runtime we simply need to hook up event listeners.

[Manishearth]

Also there are a whole bunch of hardcoded pixel values in the css that guess the sizes of toggle buttons. We ... shouldn't be doing that.

[Manishearth]

Overall the toggle code needs a major refactor, I think. I can make my changes without such a refactor but I think we need to redo the JS/CSS side of this entirely.

[camelid]

I propose we move the toggle logic to Rust code instead; writing a wrapToggle() method that wraps some HTML in a toggle.

I am definitely 👍 on moving JS code to Rust.

Might be worth creating a meta issue for reducing the size of main.js in general. We seem to do a fair amount of additional codegen there, not just for toggles.

Yeah, main.js is pretty complicated. It would be great to simplify it and move more to Rust.

Also there are a whole bunch of hardcoded pixel values in the css that guess the sizes of toggle buttons.

Hmm, what does that mean?

[Manishearth]

Also there are a whole bunch of hardcoded pixel values in the css that guess the sizes of toggle buttons.

Hmm, what does that mean?

This is used to keep the toggle block the same size. This seems to be related to attribute rendering but I can't be sure since I can't get rustdoc to render any attributes on enum variants or struct fields.

Those pixel values are roughly the size of the little +. We should never be doing this.

[camelid]

but I can't be sure since I can't get rustdoc to render any attributes on enum variants or struct fields.

That CSS that you linked to uses .enum > .toggle-wrapper and .struct > .toggle-wrapper which IIUC means it will only apply to toggle wrappers that are direct children of .enums or .structs. Is that why you can't get it rendering or is it unrelated?

[Manishearth]

It was just an example, there are lots of manual margins throughout to make abspos on the toggle wrappers work right, and I don't think it should be necessary?

[camelid]

Those pixel values are roughly the size of the little +. We should never be doing this.

Ah, I see what you mean. When I turn off the height: 25px in dev tools I get:

You're right, this code really does need redesigning!

[camelid]

So what do you think we should start with to clean this up?

[Manishearth]

I think we should first come up with a plan as to how it should work, get that reviewed by the team, and then work on it.

A very rough plan is:

• Tear out all JS toggle code. Perhaps all CSS as well.
• Anything that needs to be toggled should be wrapped in <div class=toggle-wrap data-toggle-text="Show foobar"> during Rust code generation. I kinda think this should always be a separate div so that we can have toggle_start and toggle_end in Rust, but I guess mixing it with other divs may be okay too. I'd prefer not to.
• We use a couple direct-descendant CSS selectors (>) where we shouldn't. These will break, fix them
  • Make sure this div renders cleanly
• Add very simple JS code that iterates through all of these and adds a toggle button sibling. There should be no special cases in this code. The codegen can later be moved into toggle_start in Rust if we want.
• Get this sibling to render correctly. All special cases should be handled by adding additional classes to the toggle-wrap element and using CSS, so we could have things like toggle-wrap toggle-section or whatever.
• Make the above JS hook up an event listener that can find the element to be toggled, and toggle it
• Make the above JS set defaults based on settings. We should use another css class for indicating which setting to pull from.
• The show/hide all button just does a getElementsByClassName to do its thing.

[GuillaumeGomez]

As extra information: we have four kind of toggles iirc:

• For documentation blocks
• For items' inner content (fields and variants for example)
• For attributes
• For default elements on implemented traits

I opened an issue to improve the toggles for the documentation blocks already (here: #56442). Currently, the documentation follows the item it documents. In my idea, it could be simplified by making it a child of the item instead (in the DOM). Then we could eventually try to handle the toggle through CSS only, which would allow us to remove a lot of JS.

I didn't look for the other toggles yet.

[Manishearth]

Yeah restructuring toggles to be inside a wrapping element would be very good. We should unify these types of toggles so that their mechanism is exactly the same (but their css might be slightly different)

[GuillaumeGomez]

Well, it's been in my TODO list for a while. 😆

[Manishearth]

Sounds good, I think if we're going to refactor this we should have a concrete plan in the issue before moving ahead though.

[GuillaumeGomez]

The "only" thing missing is how to do it with CSS only. Someone suggested it but I didn't investigate it yet. Once we have a solution for this, we can go to the next step.

Here is what I have in mind: we keep the current DOM but move the "docblock" elements into the element they document, as simple as that. From what I tested, it seems to be working as is but to be confirmed once again, I tested a while ago after all and some DOM changes happened in the meantime, but I think it'll still work. The only changes required would then be in the CSS once again and to check that there is no difference in the rendering.

What we can do is maybe split the work between each kind of toggle and then merge the CSS at the end? I don't think we can merge all of them though. For example, I think that the types' fields/variants' toggles will still remain on their own. Only the logic will be the same.

I expect a huge reduction of JS after this change. It would bring a huge improvement in the rendering speed.

[GuillaumeGomez]

Well, it was easy enough to find: https://stackoverflow.com/a/17731854

I can send a first PR for the "normal" doc blocks then so we can have a base to discuss on.

EDIT: Well, got super motivated and actually started the implementation. More to come soon.

[Manishearth]

What we can do is maybe split the work between each kind of toggle and then merge the CSS at the end? I don't think we can merge all of them though. For example, I think that the types' fields/variants' toggles will still remain on their own. Only the logic will be the same.

Honestly given the state of the code I'd prefer if we did it from scratch.

For example, I think that the types' fields/variants' toggles will still remain on their own. Only the logic will be the same.

What do you mean? Yes, the code generating them will be in different spots, and their CSS will be different, but any relevant JS should be the same I think.

[Manishearth]

EDIT: Well, got super motivated and actually started the implementation. More to come soon.

Can you write a concrete plan first? I'm worried we'll do something similarly hard to work with. Though if it makes all the JS go away I guess it's fine either way.

[jsha]

Update on the plan: @GuillaumeGomez started work on the HTML-ification in #83355. I did some additional work setting up CSS to use <details> in place of the current JS toggles, and landed that as part of @Manishearth's #83337. I think we have general consensus that turning the JS toggles into <details> tags is the way to go.

I've got one PR open (#84320) converting one more set of toggles to <details>, and @Swatinem has one for sub-variants (#84321). I think the best path forward is to do these one chunk at a time, which allows more attention and testing on each one. I plan to do some more of the chunks as I have time, and will plan to post here when starting on the next chunk to avoid duplicated effort.

One open question: I've noticed there are some toggle types (undocumented items; sub-variants) that don't have corresponding settings entries. Should we add settings for these or (my preference) group them under some other setting?

[Manishearth]

Thanks for taking point on this, @jsha!

Overall I don't think every toggle must have a config option, rather config options are useful when asked for. Otherwise having lots of niche configs can be pretty confusing, indeed a lot of folks used to be confused by our hyperspecific toggle settings before.

So basically: don't add a config unless someone asks, and if someone asks figure out if it their needs can be met without a config (but if not, nbd, just add it)

[Nemo157]

Has there been a check on how the <details> looks on iOS Safari? IIRC it doesn't allow hiding the default ➤ toggle button.

EDIT: Ah, the current implementation is working in iOS Safari because it happens to inherit .content .item-list { list-style-type: none; }, it might be worth explicitly putting that on the <summary> element too. (In other browsers the fact the <summary> has display: block; implicitly suppresses the default toggle button, but that doesn't apply to iOS Safari).

[jsha]

I checked safari on iPad OS 1.13 and the toggle is displayed correctly.

[GuillaumeGomez]

Currently working on methods and trait implementations.

[GuillaumeGomez]

I updated #83332 (comment). So now only two things remain:

• the documentation for non-exhaustiveness (e.g. on ErrorKind - see discussion on rustdoc: should we stop showing #[lang = ...] and some other attributes? #84309)
• the global doc comment

[GuillaumeGomez]

Working on the two last points.

[jsha]

The main refactoring (switching to <details>) is close to finished. I wanted to follow up on a couple of other points @Manishearth made early in this issue. Click handlers and bubbling are fixed in #85117.

there are a whole bunch of hardcoded pixel values in the css that guess the sizes of toggle buttons. We ... shouldn't be doing that.

There are some hardcoded negative offsets in the CSS, like left: -23px (as seen in #84602). These are not ideal, but they don't correspond to the size of the toggle buttons, they correspond to the indent at which the content occurs. That is, a doc block might be indented at 23px, but we want to "outdent" the toggles so we can use the padding space effectively. I haven't been able to think of a nice clean way to get rid of this need for negative offsetting, but I'd welcome ideas.

[jsha]

Reopening because I think these JS functions (collapseNonInherent and collapseDocs) still deal with toggling open / closed sections and need cleaning up:

rust/src/librustdoc/html/static/main.js

Lines 888 to 1018 in 455c5e0

	function collapseDocs(toggle, mode) {
	if (!toggle || !toggle.parentNode) {
	return;
	}
	function adjustToggle(arg) {
	return function(e) {
	if (hasClass(e, "toggle-label")) {
	if (arg) {
	e.style.display = "inline-block";
	} else {
	e.style.display = "none";
	}
	}
	if (hasClass(e, "inner")) {
	e.innerHTML = labelForToggleButton(arg);
	}
	};
	}
	function implHider(addOrRemove, fullHide) {
	return function(n) {
	var shouldHide =
	fullHide === true ||
	hasClass(n, "method") === true ||
	hasClass(n, "associatedconstant") === true;
	if (shouldHide === true || hasClass(n, "type") === true) {
	if (shouldHide === true) {
	if (addOrRemove) {
	addClass(n, "hidden-by-impl-hider");
	} else {
	removeClass(n, "hidden-by-impl-hider");
	}
	}
	var ns = n.nextElementSibling;
	while (ns && (hasClass(ns, "docblock") || hasClass(ns, "item-info"))) {
	if (addOrRemove) {
	addClass(ns, "hidden-by-impl-hider");
	} else {
	removeClass(ns, "hidden-by-impl-hider");
	}
	ns = ns.nextElementSibling;
	}
	}
	};
	}
	var relatedDoc;
	var action = mode;
	if (hasClass(toggle.parentNode, "impl") === false) {
	relatedDoc = toggle.parentNode.nextElementSibling;
	if (hasClass(relatedDoc, "item-info")) {
	relatedDoc = relatedDoc.nextElementSibling;
	}
	if (hasClass(relatedDoc, "docblock")) {
	if (mode === "toggle") {
	if (hasClass(relatedDoc, "hidden-by-usual-hider")) {
	action = "show";
	} else {
	action = "hide";
	}
	}
	if (action === "hide") {
	addClass(relatedDoc, "hidden-by-usual-hider");
	onEachLazy(toggle.childNodes, adjustToggle(true));
	addClass(toggle.parentNode, "collapsed");
	} else if (action === "show") {
	removeClass(relatedDoc, "hidden-by-usual-hider");
	removeClass(toggle.parentNode, "collapsed");
	onEachLazy(toggle.childNodes, adjustToggle(false));
	}
	}
	} else {
	// we are collapsing the impl block(s).
	var parentElem = toggle.parentNode;
	relatedDoc = parentElem;
	var docblock = relatedDoc.nextElementSibling;
	while (hasClass(relatedDoc, "impl-items") === false) {
	relatedDoc = relatedDoc.nextElementSibling;
	}
	if (!relatedDoc && hasClass(docblock, "docblock") === false) {
	return;
	}
	// Hide all functions, but not associated types/consts.
	if (mode === "toggle") {
	if (hasClass(relatedDoc, "fns-now-collapsed") ||
	hasClass(docblock, "hidden-by-impl-hider")) {
	action = "show";
	} else {
	action = "hide";
	}
	}
	var dontApplyBlockRule = toggle.parentNode.parentNode.id !== "main";
	if (action === "show") {
	removeClass(relatedDoc, "fns-now-collapsed");
	// Stability/deprecation/portability information is never hidden.
	if (hasClass(docblock, "item-info") === false) {
	removeClass(docblock, "hidden-by-usual-hider");
	}
	onEachLazy(toggle.childNodes, adjustToggle(false, dontApplyBlockRule));
	onEachLazy(relatedDoc.childNodes, implHider(false, dontApplyBlockRule));
	} else if (action === "hide") {
	addClass(relatedDoc, "fns-now-collapsed");
	// Stability/deprecation/portability information should be shown even when detailed
	// info is hidden.
	if (hasClass(docblock, "item-info") === false) {
	addClass(docblock, "hidden-by-usual-hider");
	}
	onEachLazy(toggle.childNodes, adjustToggle(true, dontApplyBlockRule));
	onEachLazy(relatedDoc.childNodes, implHider(true, dontApplyBlockRule));
	}
	}
	}
	function collapseNonInherent(e) {
	// inherent impl ids are like "impl" or impl-<number>'.
	// they will never be hidden by default.
	var n = e.parentElement;
	if (n.id.match(/^impl(?:-\d+)?$/) === null) {
	// Automatically minimize all non-inherent impls
	if (hasClass(n, "impl")) {
	collapseDocs(e, "hide");
	}
	}
	}

. I don't fully understand what they do yet.

[jsha]

Also we still need to remove the <span class="loading-content">Loading content...</span> from the HTML, and

        onEachLazy(main.getElementsByClassName("loading-content"), function(e) {
            e.remove();
        });

[GuillaumeGomez]

I'll start by removing the "loading-content" and then check what I forgot to remove. But normally, there is no more toggle to deal with.

[camelid]

This may be related: it looks like the CSS for h2 + div has display: none as the default and then the JS goes in and adds inline display: block styles. It seems like that behavior should be removed? An example of this occurring is the "Layout" section; e.g., scroll to the bottom of https://doc.rust-lang.org/nightly/nightly-rustc/rustc_span/symbol/struct.Symbol.html, then "Inspect Element", and then notice that there's an inline display: block style. Removing that style causes the contents of the layout section to disappear due to the display: none CSS.

[jsha]

@camelid: I think that was fixed in #85175, which was just merged so presumably doesn't show up in that nightly doc link.

[camelid]

@camelid: I think that was fixed in #85175, which was just merged so presumably doesn't show up in that nightly doc link.

Indeed, it is now fixed!

[jsha]

Great! We have one last cosmetic cleanup in #85289, then I think this is done.

[GuillaumeGomez]

🎉

[jsha]

Alright, now I think this is well and truly done. Congrats all!