Support type-dependent search result highlighting via CSS

[timhoffm]

Edit: Summary (Update: 25.07.2024)

This adds result type-dependent HTML classes to the <li> elements of the search result list. Implemented types / classes are:

• context-index: If the search term is found in an index such as the glossary
• context-object: If the search term a code object such as a python function
• context-text: If the search term is found in plain documentation text
• context-title: If the search term is found in a heading

The basic style intentionally does not apply styling to these classes to not interfere with derived themes. We encourage theme authors to apply suitable styling to the results list via CSS as they see fit. Check sphinx13.css in this PR as an example.

---

It's helpful to visually distinguish different content types.

This PR adds context to the javascript result entries (one of "title", "index", "object", "text" - please check that these names make sense, I've inferred them from the JS context) and adds them as a class to the <li> item in the result list. This allows styling via CSS.

For simplicity, I've styled with unicode symbols, which should give a decent look without the need to ship our own symbols. Derived themes have the ability to adapt this via CSS and use their own icons if they want to.

Before / after:

        

[jayaddison]

I wonder whether it might make sense to be slightly more explicit and define these using the CSS list-style-type property?

Also, to check: is the white-square default here intended to highlight cases where sites have upgraded their Sphinx CSS but not rebuilt their searchindex.js?

[timhoffm]

I'm not a CSS expert. I was anticipating that list-style is easier to override for downstream themes, e.g. list-style: url(image.svg), but just a guess. If you say list-style-type is better, I'm happy to change.

I've added the empty square as a catch-all fallback; e.g. could be that somebody rewrites search and includes a new type, but forgets the corresponding CSS. Likely other things can happen and the empty square as as placeholder is better than nothing.

[jayaddison]

I'm not a CSS expert. I was anticipating that list-style is easier to override for downstream themes, e.g. list-style: url(image.svg), but just a guess. If you say list-style-type is better, I'm happy to change.

Me neither, and your intuition may be correct. I based my suggestion on a sense that list-style-type is slightly less ambiguous (list-style does accept the same values, but can also be set to an image URL), and also a small note on the Mozilla MDN page:

The list-style property is specified as one, two, or three values in any order. If list-style-type and list-style-image are both set, the list-style-type is used as a fallback if the image is unavailable.

I've added the empty square as a catch-all fallback; e.g. could be that somebody rewrites search and includes a new type, but forgets the corresponding CSS. Likely other things can happen and the empty square as as placeholder is better than nothing.

That makes sense. A concern I have with it is that the same (or a similar) symbol is used on some systems when a unicode codepage/symbol is not found in the relevant font, and that could make problems slightly trickier to track down. Perhaps we could set the value to initial (initial browser default)?

[AA-Turner]

A concern I have with it is that the same (or a similar) symbol is used on some systems when a unicode codepage/symbol is not found in the relevant font

Our assumptions in the HTML search is that the user has a modern browser, and hence we should be confident in using Unicode -- every modern browser is able to display emoji, etc.

[jayaddison]

This is the only annotation symbol that I find slightly confusing / out of context, personally.

It does sorta logically fit that it's for use to represent that titles are 'containers' in some sense for text content.

I'm browsing unicode character references to try to find something else that could represent text/documents, headings/titles, and/or the potential for containership/nesting.

Edit: remove repetitive phrasing

[jayaddison]

Maybe it's a controversial misuse, but perhaps the paragraph symbol (called Pilcrow, as I learned today)?

[jayaddison]

(or we could try to bring the capitulum back into style)

[jayaddison]

Another alternative could simply be to style title and text matches identically to begin with.

I'll try to spend a bit more time thinking about this before adding any further thoughts/comments.

[timhoffm]

The idea behind folder was "structuring element" but I'm not too convinced of it either.

[jayaddison]

On reflection: I'd recommend keeping this simple, and using the same styling for both title and text search results - that is, 1F4C4 (unicode: page facing).

We could still emit the CSS classes title and text in the result list, allowing downstream sites to customize their display to suit their projects if they choose.

Some reasoning for this suggestion: with a default Sphinx configuration and HTML build, it is possible to infer from each listed search result whether the input search query term(s) matched in the corresponding document's title and/or content body. In my personal opinion it is a nice addition to be able to distinguish between glossary items and programming objects, but seems lower priority to distinguish page title matches from other page content matches.

[jayaddison]

Maybe nitpicky: the horizontal margin/padding between the icons and the search result titles has increased; is there a way to reduce that back to near the original spacing?

[wlach]

Might be interesting to see how this works with some popular themes e.g. furo, readthedocs, and sphinx-book-theme. I suspect it'll look fine but can't hurt to double check.

[timhoffm]

Maybe nitpicky: the horizontal margin/padding between the icons and the search result titles has increased; is there a way to reduce that back to near the original spacing?

Done:

[timhoffm]

Might be interesting to see how this works with some popular themes e.g. furo, readthedocs, and sphinx-book-theme. I suspect it'll look fine but can't hurt to double check.

Good point. They all set ul.search {list-style: none}, which this ul.search li.* {list-style: ...} will superseed; e.g. Furo:

That may be ok, but still we're pushing unexpected styling, and e.g. the spacing is too narrow for furo.
If we do that, we should give them a heads up. They can either set ul.search li {list-style: none} themselves. or maybe the also want to style depending on the type.

The alternative would be only to implement the class now, and change the CSS later, which gives downstream more time to react.

[timhoffm]

Alternative idea:

• Remove any styling from the basic theme, so that it's a simple bulleted list (the current constant background image does not add much and it feels reasonable to keep basic theme styling minimal).
• Still add the html classes in basic via search
• Actual CSS styling is up to the downstream themes; in particular I would add the styling proposed to sphinx13 and alabaster. Other themes should decide for themselves.

What do you think?

[jayaddison]

If we roll the theming out 'centrally' (in the basic) theme, I'd expect that there could be some display/layout issues for some downstream themes; hopefully those problems would be reported back to us and we could resolve them in a cross-theme-compatible manner (but those are assumptions!).

If we roll out only the HTML class names, my guess is that many themes will not notice or react to the possibility to style these elements -- that's fine, but I do think that there is a genuine user-facing improvement possible here. I also think that there's some risk -- but also some opportunity -- if fragmentation of result icon theming occurs.

In either scenario I'd be hopeful for some future requests for additional class names / tweaks to improve the categories further.

I don't have a strong preference either way yet; just trying to consider the likely outcomes at the moment.

One other thought: the basic.css_t file is a Jinja template, and can contain some basic logic; we could theoretically add a Sphinx HTML search config option to control the theming. I don't know whether that's worth it (another config setting to support, potentially over a long timescale), but again mentioning it so that we can consider it.

[timhoffm]

From the themes listed at https://sphinx-themes.org/, I pulled the GitHub stars as a proxy measure of usage. ~10 themes cover most of the user space.

So the amount of impact is managable either way: If we supply icons by default, we can check them. If we only provide classes, we can notify them that they could do styling.

From the top 7 themes only Alabaster and Bootstrap keep the original search result styling. The others have chosen to explicitly style the result themselves (often just using list-style: none).

Conceptually, I find it attractive to include as little styling as possible in the basic theme. It should give the structure, appearance should be handled by the derived themes. Us putting icons into basic makes it potentially harder for them. I believe my proposal above minimizes negative impact.

[jayaddison]

@timhoffm Thanks for providing that contextual data. I'm mostly in agreement with your suggestion about leaving the basic theme as-is - providing a gradual opt-in approach seems sensible.

An aspect I'm puzzling over is whether we should be even more cautious about affecting downstream, and not remove the file.png icon alongside each result (redundant though it does seem).

Yet one more thought: the choice of tag names does seem important, as you highlighted originally. In particular object currently strikes me as an area where we might be able to do better -- perhaps passing the Sphinx domain and object-type (e.g. py:module, c:function) could allow additional useful theming.

[jayaddison]

An aspect I'm puzzling over is whether we should be even more cautious about affecting downstream, and not remove the file.png icon alongside each result (redundant though it does seem).

I think that was too cautious and hesistant/change-resistant of me. I'm not an accessibility expert but I would expect that the file.png icon appearances in the result HTML are noise that people may want to filter out; we should remove the icon since it doesn't add any value visually either, IMO.

[timhoffm]

I've changed the basic theme to not have any styling. The effect can be seen as follows at the example of alabaster, which uses the default settings for search:

old / new:

The sphinx13 theme still got the icon styling. I plan to provide a PR for that on alabaster as well, once this PR is merged.

Yet one more thought: the choice of tag names does seem important, as you highlighted originally. In particular object currently strikes me as an area where we might be able to do better -- perhaps passing the Sphinx domain and object-type (e.g. py:module, c:function) could allow additional useful theming.

IMHO we anyway need object to allow easy styling for the whole category. We don't want downstream themes to force adding a long exhaustive list in CSS .py:module .py:class .c:function ... { }. For simplicity, I've now started only with object. One can later expand this to have more specific py:module etc. classes alongside object.

[jayaddison]

The sphinx13 theme still got the icon styling. I plan to provide a PR for that on alabaster as well, once this PR is merged.

+1 to this approach (applying the change to the project's own documentation theme first / milling our own grain)

[jayaddison]

Looks good to me - thanks @timhoffm!

[jayaddison]

Ah, one more thing @timhoffm - I always forget this - please could you add an entry to the CHANGES.rst file to describe this feature?

[jayaddison]

I'm taking a pause here @timhoffm - I'll try to continue this review (and to actually test the suggestions I'm making before adding them, when doing that) later today or tomorrow.

[timhoffm]

By the way: if your local workflow isn't too disrupted by it, it's generally preferable to accumulate commits (including merge commits if necessary to stay up-to-date with the master branch) rather than force-push; pull requests are generally (always?) squashed into a single commit at merge-time. It's not mentioned in the contributing guide, but perhaps should be.

I personally find PRs with "merge master into feature" more difficult to review, because you mix irrelevant changes from main into the feature code. Therefore, I typically rebase. But if you prefer merges, I can do that as well.

Would be good to mention this in the Contributing docs if you have preferences for a specific workflow.

[timhoffm]

I'm taking a pause here @timhoffm - I'll try to continue this review (and to actually test the suggestions I'm making before adding them, when doing that) later today or tomorrow.

@jayaddison Thanks for the review! Feel free to directly push any changes. I feel, I've done all that I can for this PR, and in particular if it's JavaScript, I won't be any help.

[AA-Turner]

@jayaddison at some point, let me know when you're happy with this PR and/or what your outstanding concerns are. This will go into 8.1.

A

[jayaddison]

@jayaddison Thanks for the review! Feel free to directly push any changes. I feel, I've done all that I can for this PR, and in particular if it's JavaScript, I won't be any help.

You're welcome @timhoffm - please note that I don't have committer rights here, so I can't push to the branch, but I may add a few more suggestions when ready, or open a pull request on your fork of the repo.

@jayaddison at some point, let me know when you're happy with this PR and/or what your outstanding concerns are. This will go into 8.1.

Ok, thank you @AA-Turner. The three remaining items I have in mind are:

1. Some phrasing adjustments for the documentation.
2. If possible, making the stylesheets theme config option in the documentation a reference/hyperlink (maybe non-trivial / could require separate supporting work).
3. The JavaScript enum handling questions, as found in previous subthreads here.

[AA-Turner]

The three remaining items I have in mind are:

Thanks. I'm minded to merge this as-is and potentially we can tackle those points in follow-ups, given none seem blockers to the feature itself? If you'd prefer that this PR stay open though, no worries.

A

[jayaddison]

The three remaining items I have in mind are:

Thanks. I'm minded to merge this as-is and potentially we can tackle those points in follow-ups, given none seem blockers to the feature itself? If you'd prefer that this PR stay open though, no worries.

I don't think they're worth considering as blockers either, merging is fine with me 👍

[jayaddison]

🎉 thanks @timhoffm @AA-Turner!

[AA-Turner]

Something seems to be off: https://www.sphinx-doc.org/en/master/search.html?q=directive

Note every result is "White Square"...

A

[timhoffm]

Has there been a change to the search results meanwhile? They seem to be grouped now: All glossary entries (directive, master document, object) are together, all entries on a page are together, ...

I will check in more detail, but that will take a week.

[AA-Turner]

Perhaps @wlach or @jayaddison might know?

A

[jayaddison]

@AA-Turner @timhoffm it's worth noting that the www.sphinx-doc.org website uses ReadTheDocs' cloud-hosted search implementation -- it may have begun grouping results, but that isn't a feature that is (yet!) implemented in the standalone/native search within Sphinx.

That probably explains why the results don't have any context-... CSS class attributes on them at all, and hence why we're seeing the default white-squares. Does that seem to make sense? (I haven't confirmed this in precise detail, but it's my theory so far)

[jayaddison]

One more note: inspecting the HTML, I noticed that we already use the term context in the CSS classes to refer to the text snippet / summary text shown alongside each search result; see the code here:

sphinx/sphinx/themes/basic/static/searchtools.js

Line 632 in 1852d0f

summary.classList.add("context");

My apologies for not noticing that during the discussion about what to name the type/kind/context classname. I now feel that we should disambiguate the two (and we do still have time to do that, since this change is not-yet-released). I think kind-... is probably the way to go after all (and I will have to learn to be less lazy about the way that I ask about non-optimal search results during bugreports).