Support type-dependent search result highlighting via CSS #12474
Conversation
jayaddison
added
type:enhancement
| background-image: url(file.png); | ||
| background-repeat: no-repeat; | ||
| background-position: 0 7px; | ||
| list-style: "\25A1"; /* Unicode: White Square */ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm not a CSS expert. I was anticipating that
list-styleis easier to override for downstream themes, e.g.list-style: url(image.svg), but just a guess. If you saylist-style-typeis 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-styleproperty is specified as one, two, or three values in any order. Iflist-style-typeandlist-style-imageare both set, thelist-style-typeis 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)?
There was a problem hiding this comment.
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.
| ul.search li.title { | ||
| list-style: "\1F4C1"; /* Unicode: File Folder */ | ||
| } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Maybe it's a controversial misuse, but perhaps the paragraph symbol (called Pilcrow, as I learned today)?
There was a problem hiding this comment.
(or we could try to bring the capitulum back into style)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The idea behind folder was "structuring element" but I'm not too convinced of it either.
There was a problem hiding this comment.
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.
This comment was marked as resolved.
This comment was marked as resolved.
|
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? |
|
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. |
Done:
|
Good point. They all set That may be ok, but still we're pushing unexpected styling, and e.g. the spacing is too narrow for furo. The alternative would be only to implement the class now, and change the CSS later, which gives downstream more time to react. |
|
Alternative idea:
What do you think? |
|
If we roll the theming out 'centrally' (in the 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 |
|
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 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. |
|
@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 Yet one more thought: the choice of tag names does seem important, as you highlighted originally. In particular |
I think that was too cautious and hesistant/change-resistant of me. I'm not an accessibility expert but I would expect that the |
timhoffm
force-pushed
the
search-icons
branch
3 times, most recently
from
b4affb1
to
0ac6880
Compare
|
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.
IMHO we anyway need |
+1 to this approach (applying the change to the project's own documentation theme first / milling our own grain) |
|
Ah, one more thing @timhoffm - I always forget this - please could you add an entry to the |
timhoffm
changed the title
|
It seems the changelog entries only refer to the PR number and reflect it's title. I've put a short summary at the top to make this more useful. Possibly worth adding a section on CSS classes in HTML theme development at some point. But that's for later. |
AA-Turner
changed the title
This allows styling different types of results individually. It's helpful to visually distinguish different content types. This PR adds `itemType` to the javascript result entries (one of "title", "index", "object", "text") and adds them as a class to the <li> item in the result list. This allows styling via CSS. The basic theme only contains the mechanism to add the HTML classes. It does intentionally not do any styling via CSS. We reserve that freedom to derived themes. For the internal sphinx13 theme, I've styled with unicode symbols, which should give a decent look without the need to ship our own symbols.
There was a problem hiding this comment.
Sorry a bit late looking at this again.
Approving because I wouldn't be sad if this went in as-is. We'll need a committer (maybe @picnixz or @chrisjsewell?) to actually accept + land it though.
One thing I wonder is whether we should add some documentation about this feature, perhaps somewhere like this:
https://www.sphinx-doc.org/en/master/development/html_themes/index.html
We could also link maintainers of other themes to this as a resource, to get them to adopt/use this new data.
|
|
||
| let listItem = document.createElement("li"); | ||
| listItem.classList.add(resultType) |
There was a problem hiding this comment.
Could you add a quick comment explaining what this does?
There was a problem hiding this comment.
Do you mean a comment here for clarification, or a code comment for documentation?
Starting here: The search results are written to HTML <li> elements - in JS here the listItem variable. resultType is one of the strings "title", "object", "index", "text". This code line adds the result type as class attribute. e.g. resulting in <li class="title">, which can then be in a CSS selector for styling.
There was a problem hiding this comment.
Yeah, something like that but I would probably be a little more succinct: // Add a class representing the item's type: can be used by a theme's CSS selector for styling or so.
Both yes, but only makes sense, when the exact semantics and behavior is agreed upon (c.f. #12474 (comment)) |
I'd prefer to add this to the built-in themes if possible, including basic. Though Sphinx13/Alabaster can be fancier with their styling if need be. Once done, we can alert the maintainers of downstream themes. Other than that, looks great currently! Thanks @timhoffm, @wlach, @jayaddison, et al. A |
Any styling we do in basic may have to be undone by downstream theme authors, which is cumbersome and potentially fragile if we change something in the basic styling later. The current basic styling with the static document image does not convey information. It's just a stylistic thing (whether looking nicer or just adding clutter is in the eye of the beholder). From a usability perspective, a plain bullet list is as good as the bullet list with the static document icon. Given that it's just styling and the potential downstream problems, IMHO basic should not have any styling. Note - as mentionend in the above post - that a majority of themes have just removed our styling (using +1 on adding styling back in for the builtin themes. Should this be in this PR or can that be a follow-up? |
basic should provide a sane and useful set of defaults. I spoke in favour of adding the styles to basic as it would mean that every downstream theme gets those defaults automatically. For the themes that customise their list styles (with
Whichever you prefer. The changes to Alabaster will need to be in a PR as it lives at https://github.com/sphinx-doc/alabaster. A |
I claim that the sane and useful set of defaults is no styling when taking into account compatibility with existing downstream themes 😃. You could consider the changes discussed here as a multi-step process:
(1.) Is 100% backward compatible. Personally, I think going up to including (2.) is an ok migration. I don't dare to (3.) because of potentially breaking other themes. - If you want to do that, I recommend to check the effect on the most important ones, but that's cumbersome and I'm not going to do that. If you otherwise think (2.) is not a good middle ground, I'd rather only go to (1.) and not touch default styling at all. This is my personal recommendation. You should go as far as you think is feasible. But if it's 3., please take over from here. I intentionally stepped back from this. |
I'd rather go to separate PRs so that we frist have a definitive version of this PR fixed. |
|
You make a fair argument -- lets go with (2) for now, and I will see if I have time to evaluate (3) later -- but that won't block this or any follow-up PRs to the built-in themes. Thank you for humouring the discussion! A |
| // Add a class representing the item's type: | ||
| // can be used by a theme's CSS selector for styling | ||
| // See SearchResultKind for the class names. | ||
| listItem.classList.add(`result-kind-${resultKind}`); |
There was a problem hiding this comment.
@AA-Turner in #12542 we don't seem to add any prefixes to domain/object-type when those are added as CSS classes. What's the reason for adding a prefix here?
It feels to me like the CSS class names we choose here could be long-lived; this is the only part of the branch as-is that I feel cautious about.
There was a problem hiding this comment.
I had originally not included any prefix, because the typical selector will be ul.search li.[type] and ul.search li.object seemed clear enough. But I don’t have a strong opinion.
There was a problem hiding this comment.
Gradually iterating through some thoughts:
- From the perspective of a theme author, the ability to reference consistent CSS names when styling search results and ToC entries seems like it'd be beneficial.
- Sphinx-style object qualifiers (
py:module,cpp:class) would conflict with CSS syntax - soclass="py module"orclass="cpp class"might be the preferred output here. - Building upon those two thoughts, the
kindvalue that we're talking about here is useful context -- but it is a different dimension to the Sphinx object qualifier. The values could conflict for some domains though -- so in fact, a prefix does seem like a good idea.
There was a problem hiding this comment.
One more thought: for some reason I don't love the word kind in this context. It seems like an unnecessarily-different synonym for type -- itself a frequently-overused name in software.
I'd like to express something about what the currently-named-kind here relates to. And I think it's something like a match category -- it's how and/or why the search algorithm decided to include an entry (yes, that's generally derived 1-1 from how the search index is structured, but it doesn't necessarily have to be).
There was a problem hiding this comment.
Prototyping / pseudogenerating some results:
<result class="match-object cpp class">utils::Format</result>
<result class="match-text">Formatting Guidelines</result>
<result class="match-object py function">foo.bar.format</result>
Does that seem semantic and themeable?
There was a problem hiding this comment.
(I'm not suggesting that we include Sphinx domain and object type yet -- but trying to anticipate that they could be included in future)
There was a problem hiding this comment.
class="py module" seems a good extension possibility. I’ve first thought about class="py-module" because you likely want to be domain-specific and not style all modules the same way across contexts. However, you can easily combine selectors .py.module, which is not worse than .py-module and leaves more flexibility.
I’m not having a good prefix either. Personal thoughts: type is very generic. Same with kind, but that’s additionally feels somewhat unconventional. match connects too much to the search process and less to the result. Other possibilities context, category
Not necessarily related but possibly a good way to come up with suitable names(prefix and type/kind): If we’d add a GitHub-like filter mechanism to the search field, what would be intuitive names for our end users? e.g. some_func category:api feels better thansome_func kind:object.
There was a problem hiding this comment.
Not necessarily related but possibly a good way to come up with suitable names(prefix and type/kind): If we’d add a GitHub-like filter mechanism to the search field, what would be intuitive names for our end users? e.g.
some_func category:apifeels better thansome_func kind:object.
Mm, good question - query language syntax. The label:value syntax does seem well established for various web-based search engines. For that specific example, I think that name:some_func might be simplest - the function is an object entity that is indexed, and those entities tend to have names. Other entities could be document, section, word... but I'd also be wary of reinventing the docutils node hierarchy.
There was a problem hiding this comment.
It feels to me like the CSS class names we choose here could be long-lived; this is the only part of the branch as-is that I feel cautious about.
I had originally not included any prefix, because the typical selector will be
ul.search li.[type]andul.search li.objectseemed clear enough.
I added a prefix out of caution -- CSS backwards-compatibility is very annoying, as you've no idea what selectors people can use. Whilst ul.search li.[type] is best, li.[type] or even .[type] is possible, and hence reusing the class names somewhere else carries a small risk. Namespacing with some prefix reduces that. I haven't merged #12542 as I'm unsure if the benefit is worth the long-term backward compatibilty commitment. On the other hand, I might be overthinking this whole endevaour entirely.
One more thought: for some reason I don't love the word kind in this context. It seems like an unnecessarily-different synonym for
type-- itself a frequently-overused name in software.
type felt wrong here, though on a 'feelings' level rather than a great deal of rationality. No strong feelings on going back to type really.
A
Edit: Summary
This adds result type-dependent HTML classes to the
<li>elements of the search result list. Implemented types / classes are:index: If the search term is found in an index such as the glossaryobject: If the search term a code object such as a python functiontext: If the search term is found in plain documentation texttitle: If the search term is found in a headingThe 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.cssin this PR as an example.It's helpful to visually distinguish different content types.
This PR adds
itemTypeto 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 CSSand use their own icons if they want to.
Before / after: