Put negative implementors first and apply same ordering logic to foreign implementors

[GuillaumeGomez]

Fixes #51129.

This PR changeda surprisingly small amount of things to put negative trait impls before the others: basically just adding a new information in the generated JS file for foreign implementors and a "negative marker" DOM element to know where to insert negative impls.

I also used this occasion to make the foreign implementors sort the same as the local ones by using compare_names.

You can test it here.

r? @notriddle

[rustbot]

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @jsha, @lolbinarycat

[GuillaumeGomez]

Took me a while to figure out where the typescript definition was stored to fix the error. ^^'

[lolbinarycat]

This is definitely an improvement, as it eliminates an allocation, but we're still allocating an intermediate string for each element.

OrderedJson could be much more efficient if it had a method that appended using serde_json::to_writer.

I can open an issue for improving the OrderedJson interface, if you want.

[GuillaumeGomez]

An issue would be welcome indeed.

[bors]

☔ The latest upstream changes (presumably #142667) made this pull request unmergeable. Please resolve the merge conflicts.

[GuillaumeGomez]

Fixed merge conflicts.

[notriddle]

Either JavaScript-loaded content needs to go on the bottom, or it needs to be loaded in a blocking manner. Otherwise, you get layout shift and potential misclicks.

output.mp4

[notriddle]

Need a solution to the layout shift problem.

[GuillaumeGomez]

Fair enough. I can make the implementors content to be hidden by default (when JS is enabled) and display it when the updated is done.

[GuillaumeGomez]

So now, the implementors lists are only displayed once they have been filled by the JS. I still display the headings though. I also opened the hosted example.

Does that sound good to you @notriddle or do you have something else in mind?

[notriddle]

How does this work on a page like

rust/tests/rustdoc-gui/trait-with-bounds.goml

Lines 3 to 4 in e76a5eb

	// It tries to load a JS for each trait but there are none since they're not implemented.
	fail-on-request-error: false

that has no foreign impls at all?

[notriddle]

Does that sound good to you @notriddle or do you have something else in mind?

I wanted to put downstream foreign trait impls on the bottom, so we end up equivalent to impls.sort_by_key(|i| (!i.is_foreign, i.is_negative)). That way main.js and trait.Sync.js don't block us from showing content that's already in the HTML anyway.

But, in order of preference, if I have to pick between longer load time and more layout shift, I'll usually pick longer load time. But I'd rather go with an option that avoids both.

[GuillaumeGomez]

The order needs to be as follows (and is part of the issue): negative impls (including foreign ones) then other impls. It was suggested that we could split between local and foreign impls but it was mentioned that splitting them was irrelevant for doc readers and therefore should be avoided. Which leaves us with the current situation: I don't see how we can have the negative impls then the others without having the layout shift or the longer load time.

If we don't have ideas now, can always be improved later. It's a very old issue and I don't think implementors are the first thing people are looking for, so this extra load time (which is only noticeable on std/core as far as I could see) seems acceptable, even if not perfect. What do you think?

[jsha]

I think the layout shift problem exists even before this PR. Foreign implementors (regardless of whether they are negative implementations) get loaded async and added to the doc.

This is probably less noticeable in general because (IIRC) they get added to the end of the Implementors section, where there is no hyperlink target. By comparison in this change it's easy to see the layout shift because we can link to the top of the Implementors section and the foreign negative implementations (which are common for Send and Sync) get plonked at the top.

In theory the simplest solution should be to make trait.Sync.js (or equivalent JS data for other traits) load in a blocking way: move it to <head> and remove the async. Unfortunately, main.js is loaded with defer, and trait.Sync.js needs to call register_implementors from main.js to display its contents (or set pending_implementors, but that just results in layout shift too).

It was suggested that we could split between local and foreign impls but it was mentioned that splitting them was irrelevant for doc readers and therefore should be avoided.

Where was this suggested? I don't see it in the issue (#51129).

This seems like an good solution to me. It still results in layout shift, but it would mean the top of each section ("Implementors" and a new "Negative Implementors") would be stable, while new entries would be added below. It also makes the distinction clear. If we use sort order, readers have to guess whether it's coincidence that all the negative ones are on top. If we use a separate section, we're explicitly telling the reader that all negative impls will wind up in that section.

In other words, we would have:

## Negative Implementors

<local implementors in arbitrary order>
<foreign implementors in arbitrary order>

## Implementors

<local implementors in arbitrary order>
<foreign implementors in arbitrary order>

[lolbinarycat]

it would mean the top of each section ("Implementors" and a new "Negative Implementors") would be stable

Does that matter? are browsers smart enough to keep screen position relative to section starts?

If you're looking at the bottom section, and something is added to the top section, will the page scroll position appear to jump around?

Another hypothetical solution would be disabling all the clickable elements in these sections (also make them visually not look like a link) until they're fully loaded. This ofc doesn't actually solve the problem if the user tries to click the link before it is clickable.

[GuillaumeGomez]

@jsha It was discussed in your previous PR, starting from this comment (should have provided link to it before, my bad). I assumed it was what the team agreed upon but I realize it's not really the case (although I do prefer not having to keep foreign and local impls separated).

Splitting negative implementors out wouldn't resolve the layout shift issue either since we would still need to add foreign ones with JS. It gets a bit tricky in case there are no local negative impls, only foreign ones. Currently, we always generate "implementors" heading, does it mean we would do the same for negative implementors?

[notriddle]

Does [keeping the top of each section stable] matter? are browsers smart enough to keep screen position relative to section starts?

Yes?

Some of the behavior involved in scroll anchoring is not specced, so it's hard to be sure, but I've tried a test in Firefox and Chromium where I click the "Auto Implementors" sidebar entry before trait.Sync.js finishes loading on the nightly docs.

Both browsers are definitely treating the # target specially, but not exactly the same. You can see this very clearly in Chromium:

output.mp4

If I click it while it's loading, my scroll point stays glued onto it. If I manually scroll above it while I'm still loading, I get jumped back to the header after the load finishes.

Firefox, instead, unglues me if I scroll above the header, so you get this behavior instead (I have to use my scroll wheel to do it quickly enough; the smooth scrolling is me, not the app):

output.mp4

But this is specifically talking about using the sidebar to get to the "Auto Implementors" section, because scrolling that far before it finishes loading is difficult.

[jsha]

It was discussed in your previous PR, starting from #85398 (comment) (should have provided link to it before, my bad)

In this linked comment thread we were discussing sections for "Foreign Implementors" vs "Local Implementors". Here I'm proposing a different split: "Negative Implementors" vs "Implementors".

Splitting negative implementors out wouldn't resolve the layout shift issue either since we would still need to add foreign ones with JS. It gets a bit tricky in case there are no local negative impls, only foreign ones. Currently, we always generate "implementors" heading, does it mean we would do the same for negative implementors?

We should only provide a "Negative Implementors" section for auto traits. For most other traits, negative implementors are irrelevant. There's the unstable feature for negative impls, but I think the most important situation to solve here is the built in auto traits.