Fix search results display

[GuillaumeGomez]

Fixes #85544.

cc @dns2utf8

r? @jsha

[rust-highfive]

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

[jsha]

I don't think flex is the solution to our problems here. Consider the mobile screenshots below. With flex, we unnecessarily wrap the type really tightly, in a way that's very confusing.

One cool side effect of switching from a <table> to a list of <a> for search results is that we have a lot more flexibility with how we do layout. Thinking about what we want for the very narrow mobile case:

As the screen gets narrower, first the descriptions on the right should either get hidden or get bumped down to the next line. If they get bumped down to the next line, they should have a bit of left padding to distinguish them from the item names.

Then, as the screen gets too narrow even for the item names, we should start wrapping the item names themselves.

Note that we have a similar problem for the table layout at the bottom of a module page, where the module links to items inside it (see last screenshot).

This PR:

nightly:

https://doc.rust-lang.org/nightly/std/#macros

[jsha]

When we have a series of direct-parent selectors like that, we're creating a brittle dependency on a specific structure of the DOM. It becomes hard to make a change to the code that generates search results and figure out which CSS selectors need to change.

Instead, we should add classes, or use the classes that are already here - result-name and desc (though note desc is on a span rather than a div and might not be exactly what you're looking for).

[GuillaumeGomez]

Good point.

[GuillaumeGomez]

I don't think flex is the solution to our problems here. Consider the mobile screenshots below. With flex, we unnecessarily wrap the type really tightly, in a way that's very confusing.

The problem is that some paths are really long. I much prefer having them on multiple lines rather than displaying less things. The most important thing for me is to have the items width be the same for all of them (which is why we used a table before). The current display on nightly is really bad in my opinion.

I don't mind the current module display but it's up-to-debate.

[dns2utf8]

Could we improve visibility with something like this?

.search-results > a:nth-child(even) { background: rgba(128,128,128, 0.42); }

@jsha do I understand you correctly that you suggest an alternating layout.
How would you solve the issue of entries that have no description?

[jsha]

The problem is that some paths are really long. I much prefer having them on multiple lines rather than displaying less things.

I agree the problem is that some paths are really long. That's why I think it's better to use more of the screen width for them on small screens, so there's less wrapping.

@jsha do I understand you correctly that you suggest an alternating layout.

Yep.

How would you solve the issue of entries that have no description?

A mockup:

[GuillaumeGomez]

I strongly approve this example!

[dns2utf8]

I like the mockup too 👍

Would extremely long paths (like use protobuf::descriptor::EnumDescriptorProto_EnumReservedRange::has_end) wrap and enlarge the result box?

[jsha]

I mocked this up in DevTools so I don't have an easy way to turn it into a patch, but it was very few CSS changes:

• Remove float: left on .result-name
• Add to .search-results a: padding-top and padding-bottom: 5px, plus border-bottom: 1px solid lightgrey.
• Add to .search-results .desc: padding-left: 1em.
• Add to one of the wrapping divs (I forget which) overflow-wrap: break-word;. I'm less sure about this one. It didn't do exactly what I wanted but it fixed overflowing names when they get longer than the screen. Ideally I'd like overflowing names to get broken on :: boundaries, with the :: showing up first on the next line to make it clear it's a continuation. But that can be a later improvement.

[dns2utf8]

We can insert <wbr>or zero width unicode spaces after the :: (I have a demo on my blog) to break there.

[GuillaumeGomez]

Making the <span> using display: inline-block also makes them go back to line in case they're too long.

Add to one of the wrapping divs (I forget which) overflow-wrap: break-word;.

@jsha This doesn't work here unfortunately.

[GuillaumeGomez]

I'll let @dns2utf8 implement the mobile version you suggested then, so he can try things out on his own without conflict with mine. :)

As for the current PR, I'll update the CSS. But does it look ok to you @jsha? Or do you have another solution in mind maybe?

[dns2utf8]

I implemented it (GuillaumeGomez#8) as a pure CSS change because there are some substantial changes in #85540

[jsha]

As for the current PR, I'll update the CSS. But does it look ok to you @jsha? Or do you have another solution in mind maybe?

Yeah, I think it looks okay. I am reluctant to add more flexboxes to our layout because I think it is usually more of a fit for application-style layouts and we are emphatically a document-style layout. But it does seem the right tool for the job.

The other thought I had was that it would be nice if we could seamlessly switch over to the alternating layout in my mockup, for instance if we detected that any name would wrap. But that would be awkward in the case where (a) you're on a wide desktop screen and (b) only one result has a long name but the others are short.

So, let's proceed with this. But can you change the a > div > div selector?

[dns2utf8]

I need a new class to remove the deeply nested a > div > div selector, I picked .result-description for now.
However, I noticed that we don't need the first div because we can use the a for the flexbox too.

[GuillaumeGomez]

So, let's proceed with this. But can you change the a > div > div selector?

Sure!

I need a new class to remove the deeply nested a > div > div selector, I picked .result-description for now.
However, I noticed that we don't need the first div because we can use the a for the flexbox too.

Then I'll merge your PR into mine, that seems to be the simplest way. ;)

[dns2utf8]

Yes, I like merging all the things 👍
We are reaching kernel level fork complexity 😅

[GuillaumeGomez]

@jsha: I moved the desc class to the <span>'s parent, allowing simpler CSS rules. That means I'll need to be careful when updating #85540 too (I'll add a note there).

[jsha]

@bors r+

[bors]

📌 Commit 8968c0e has been approved by jsha