sphinx
sphinx copied to clipboard
sphinx-doc
ENH: Support type-dependent search result highlighting via CSS
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 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 itemType 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 CSSand use their own icons if they want to.
Before / after:
An accessibility suggestion, because although I generally like these changes a lot, they do make the result list appear less like a standard HTML list visually: perhaps we should follow an MDN recommendation to add role="list" to the parent list element (searchList in this case)?
https://github.com/sphinx-doc/sphinx/blob/b70578f80c78b00c6571203f3204126742367bce/sphinx/themes/basic/static/searchtools.js#L250-L251
Ref: https://developer.mozilla.org/en-US/docs/Web/CSS/list-style-type#accessibility_concerns
Safari will not recognize an ordered or unordered list as a list in the accessibility tree if it has a
list-style-typevalue ofnone. This can be resolved by addingrole="list"to the list's opening tag. To learn more about this and potential workarounds, seelist-style.
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.
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:
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.
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?
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.
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.
@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.
An aspect I'm puzzling over is whether we should be even more cautious about affecting downstream, and not remove the
file.pngicon 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.
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
objectcurrently 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.
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)
Ah, one more thing @timhoffm - I always forget this - please could you add an entry to the CHANGES.rst file to describe this feature?
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.
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.
Both yes, but only makes sense, when the exact semantics and behavior is agreed upon (c.f. https://github.com/sphinx-doc/sphinx/pull/12474#issuecomment-2200462297)
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).
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. Traditional, EPUB, Agogo, and Nonav will also need to be updated.
Once done, we can alert the maintainers of downstream themes.
Other than that, looks great currently! Thanks @timhoffm, @wlach, @jayaddison, et al.
A
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.
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 list-style: none). So no styling seems to be not a bad default.
+1 on adding styling back in for the builtin themes. Should this be in this PR or can that be a follow-up?
Given that it's just styling and the potential downstream problems, IMHO basic should not have any styling.
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 list-style: none or otherwise), surely that would override basic?
+1 on adding styling back in for the builtin themes. Should this be in this PR or can that be a follow-up?
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
basic should provide a sane and useful set of defaults.
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:
- Only add the HTML class but do not change styling.
- Remove the current static document icon from basic.
- Add new more fancy styling to basic.
(1.) Is 100% backward compatible. I vouche that (2.) does not have a relevant negative impact on downstream themes. Either they have removed the document icon themselves, in which case nothing has changed, or they have kept the original style, in which case they just don't get the document icon in the future. The difference is shown in https://github.com/sphinx-doc/sphinx/pull/12474#issuecomment-2198604882. I propose that this should be acceptable. It's a minor visual change, and I assume it's unlikely that theme authors have a strong opinion on the icon being there - they likely just have taken what is provided. (3.) Was my initial propsal, but I withdraw from that, because it is far more likely to interfer with downstream themes that customize the search results. See for example https://github.com/sphinx-doc/sphinx/pull/12474#issuecomment-2190133277. It's not just a simple on/off switch but alsco involves placement / spacing aspects that need to be handled depending on the customized theme.
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.
Whichever you prefer. The changes to Alabaster will need to be in a PR as it lives at https://github.com/sphinx-doc/alabaster.
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
Adding a note to mention that I'm still supportive of this feature.
I was thinking about it again a few moments ago when considering compatibility with result grouping, if we introduce that in future (re: a discussion comment I've added here).
In particular: in the top-level result list, I think the simplest approach would be to display the title, theme/style and search summary of the first result displayed (which should also be the highest-scored), and not to change that despite any other items subsequently included in the grouping.
Updates:
- changed to
context-*naming. - Added documentation in
html_themes/index.rst - Rebased onto master to remove conflicts
IMHO this is now good-to-go.
Ping @AA-Turner, in case you want to bring this into 8.0.
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'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.
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
masterbranch) 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.
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.
@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 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:
- Some phrasing adjustments for the documentation.
- If possible, making the
stylesheetstheme config option in the documentation a reference/hyperlink (maybe non-trivial / could require separate supporting work). - The JavaScript enum handling questions, as found in previous subthreads here.
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