Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[rst] Add collapsible option to admonition directives #12507

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

chrisjsewell
Copy link
Member

@chrisjsewell chrisjsewell commented Jul 3, 2024

edit: see https://sphinx--12507.org.readthedocs.build/en/12507/usage/restructuredtext/directives.html#directive-note, for an example usage and rendering


This PR adds collapsible and open flag options to the core admonition type directives, which are propagated to the nodes, e.g.

.. admonition:: title
   :collapsible:
   :open:

   content

.. note:: content
   :collapsible:
   :open:

For the HTML5 writer, this replaces the outer div with a details tag, and the title p with a summary tag (with an open attribute if set), e.g.

<div class="admonition note">
<p class="admonition-title">Note</p>
<p>hallo</p>
</div>
<details class="admonition note" open="open">
<summary class="admonition-title">Note</summary>
<p>hallo</p>
</details>

For all other writers, these attributes are ignored.


I feel this a better alternative to #10532, since:

  1. it is more user-friendly, since users do not have to learn any new extensions/directives, and can easily adapt current documentation
  2. it still fits into the conceptual model of an “abstract document”, since no new (HTML-only) nodes are added

This would inherently require additions to theme extensions, to correctly style such admonitions.

But I feel this would be worth the effort (and already necessary anyway for #10532),
(in fact, it has already been added in https://jbms.github.io/sphinx-immaterial/admonitions.html#directive-option-admonition-collapsible,
which in-turn mirrors https://squidfunk.github.io/mkdocs-material/reference/admonitions/#collapsible-blocks)

cc @pradyunsg for comment


TODO: tests, documentation, addition of CSS styling to core/docs theme

@chrisjsewell chrisjsewell changed the title Admonition collapsible [rst] Add collabsible option to admonition directives Jul 3, 2024
@pradyunsg
Copy link
Contributor

cc @pradyunsg for comment

I'd be happy to accomodate for this downstream.

@Carreau
Copy link
Contributor

Carreau commented Jul 5, 2024

For what it is worth, in Pydata Sphinx Theme we have equivalent "dropdown" we use from sphinx-design:

https://pydata-sphinx-theme.readthedocs.io/en/stable/user_guide/web-components.html#dropdowns

Not pronouncing for everybody of the core team, but I think we would be happy with having this in core sphinx.

I don't know if it's of use but there was an accessibility audit of PST, and I believe one of the findings was that the >/v chevron as an indication you can click to expand was not a good enough indication. In PST there is a hover effect which was judged not sufficient. Quansight-Labs/czi-scientific-python-mgmt#80

I guess that's more of a theming question.

Would it make sens ":collapsible:" to take a boolean instead of a flag in case you want to make all collapsible by default and override with false ?

@2bndy5
Copy link

2bndy5 commented Jul 14, 2024

Would it make sens ":collapsible:" to take a boolean instead of a flag in case you want to make all collapsible by default and override with false ?

In sphinx-immaterial theme, we conditionally override the admonitions to include this feature (among others). I implemented the :collapsible: option as a string that only changes collapsible behavior if the value is open; otherwise any value is considered as "closed".

.. admonition:: An optional title in sphinx-immaterial
    :collapsible:

    Closed by default.

.. admonition:: An optional title in sphinx-immaterial
    :collapsible: open

    Opened by default.

Where the optional title becomes required if the :collapsible: option is specified at all.

It makes sense to have a separate :open: option in sphinx-design because the .. dropdown:: directive already implies a specific behavior. In this patch, the specific behavior is described by the admonition directive's :collapsible: option. Whether or not the admonition is open or closed by default is a parameter specific to the :collapsible: behavior.

Copy link

@2bndy5 2bndy5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't effect the sphinx.ext.todo admonitions. I imagine people would make an assumption that this feature is extended to .. todo:: directives as well.

Comment on lines +349 to +354
if node.get('open'):
self.body.append(
self.starttag(node, 'details', CLASS=('admonition ' + name), open='open')
)
else:
self.body.append(self.starttag(node, 'details', CLASS=('admonition ' + name)))
Copy link

@2bndy5 2bndy5 Jul 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The repetition here could be reduced with a dict expanded for kwargs to starttag().

Suggested change
if node.get('open'):
self.body.append(
self.starttag(node, 'details', CLASS=('admonition ' + name), open='open')
)
else:
self.body.append(self.starttag(node, 'details', CLASS=('admonition ' + name)))
kwargs = {"CLASS": ('admonition ' + name)}
if node.get('open'):
kwargs["open"] = "open"
self.body.append(self.starttag(node, 'details', **kwargs))

@2bndy5
Copy link

2bndy5 commented Jul 14, 2024

We, at sphinx-immaterial, also satisfied a feature request to style the version directives as admonitions. Ironically, the request came from the author of the sphinx-material theme (sphinx-immaterial's ancestor) in which he assumed the version directives were types of admonitions. He isn't wrong, but the fact that the sphinx docs grouped all admonitions in with the version directives (as "Paragraph-level markup") led to the confusion (and he cited the pydata theme's styling of version directives).

In resolving the request, I extended the collapsible behavior to the version directives as well. But I had to mandate the version directives had at least 2 directive arguments specified as well as directive content if the :collapsible: option was specified. See our docs for demo. And the default title is not customizable to preserve expected behavior of the native sphinx implementation.

@trallard
Copy link

trallard commented Jul 18, 2024

This makes sense and I am sure we can accommodate this in PST.

A couple of thoughts/suggestions (for usability and accessibility)

  1. Can there be an accompanying text vs only the Chevron? Something like "Click to collapse/expand"
  2. Can this be keyboard accessible? We recently made some remediation in PST to improve the keyboard access and focus. So perhaps we could reuse some of the code/implementation we already have in PST.

Ref in PST: https://pydata-sphinx-theme.readthedocs.io/en/stable/user_guide/web-components.html#dropdowns

Note that per the issue linked above by Matthias we are making some interaction design improvements (designed not implemented though).

@chrisjsewell
Copy link
Member Author

A couple of thoughts/suggestions (for usability and accessibility) ... text vs only the Chevron, keyboard accessible
I don't know if it's of use but there was an accessibility audit of PST, and I believe one of the findings was that the >/v chevron as an indication you can click to expand was not a good enough indication.

Thanks for the feedback @trallard, @Carreau, but would these not be theme specific concerns, i.e. these considerations would not change the actual HTML that is written, only the CSS?

TBH I kind of feel the chevron is sufficient;
it's part of the "common web visual language" for progressive disclosures (see e.g. https://primer.style/ui-patterns/progressive-disclosure#chevron-icon),
and also having text would be problematic for longer admonition titles (there would need to be sufficient gap between the admonition title and the dropdown text).
But yeh, it should be possible to leave this decision up to the theme developer I think

@pradyunsg
Copy link
Contributor

Can this be keyboard accessible?

Yes. This is using the <details> tag directly, so it gets regular HTML interactivity from the browsers directly -- you can focus on this with [tab] and it can be collapsed/opened with [space] when in focus. From a quick manual check, it looks functionally the same as PST example linked (the markup is also similar in the relevant ways).

The one difference is that the example here has got the browser-provided outline that PST's example does not have due to sphinx-design providing an outline: none.

@trallard
Copy link

Yeah that outline none was just changed in the latest Sphinx Design release which messed up with our visible focus indicator. And we yet have to fix.

@timhoffm
Copy link
Contributor

My 2 cents: While this is a nice option, I believe it's not as useful as it may seem from a usability perspective:

  • I would typically not bother to collapse originally expanded admonitions but instead just scroll past them.
  • Initially closed admontions only expose the type, but don't reveal anything about their content:
    image
    I've then two choices: 1. Skipping with a vague feeling that I'm missing someting relevant, or 2. investing extra effort to unfold the admonition.
    Conversely, as an author I would only use this very sparringly, because I'd have the feeling that most users will just skip over.
  • The story would be different if the admonitions had a caption, but that concept does not exist in ReST/docutils.
  • OTOH, what I'm really missing are collapsible code blocks. Unfortuately, AFAICT the <details> mechanism cannot be applied to code blocks, because we do not have a persistent part that can serve as <summary>. One would have to come up with something else here, likely involving javascript.

@chrisjsewell
Copy link
Member Author

The story would be different if the admonitions had a caption, but that concept does not exist in ReST/docutils

The admonition directive and admonition nodes in general allow for any title

@2bndy5
Copy link

2bndy5 commented Jul 19, 2024

Initially closed admontions only expose the type, but don't reveal anything about their content

This is true about specific admonitions (note, warning, seealso, etc) provided by docutils/Sphinx directives. But with a generic admonition directive (provided by docutils), the title can be more easily controlled and CSS classes can be used to inherit from specific admonition styles or override a theme's default style (for an individual admonition):

.. admonition:: Important Implementation Details
    :collapsible:
    :class: important

    End-users probably won't know this unless they dig through our source code...

@chrisjsewell You kinda beat me to this answer 🤣 But I added the bit about CSS classes

@timhoffm
Copy link
Contributor

timhoffm commented Jul 19, 2024

Ok, you are right. It works for .. admonition:: but not for anything else, because the specific admonitions claim the title for their type.

.. note:: This is a title

   Something to do.

.. admonition:: This is a title

   Something to do.

grafik

It's worth documenting / illustrating the admonition+:class: approach. That's something really helpful, but not easily discoverable.

@2bndy5
Copy link

2bndy5 commented Jul 19, 2024

Yeah we overhauled the admonitions in sphinx-immaterial to allow for a custom title in our added :title: option.

I also think that docutils' generic admonition shouldn't mandate a title at all (a requirement sphinx-immaterial theme has removed), but that's getting off topic.

It's worth documenting / illustrating the admonition+:class: approach

This tactic might be theme-specific. It depends on how the CSS for a theme is structured.

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Jul 19, 2024

Yeh... obviously a bit out of scope for this PR, but basically, these are in essence equivalent:

.. note:: Content

.. admonition:: Note
   :class: note
   
   Content

It's good to have the "shorthand", but indeed I feel there should be a middle ground:

  • note allows for a title option, and/or
  • admonition allows for a style option, to make it more explicit that we want e.g. note styling

I also don't really get in docutils, why they need to have separate, specific nodes for each admonition type, rather than a single admonition node with a style attribute,
you see e.g. in the HTML writer that you basically just have to have extra/unnecessary boilerplate, to redirect all the admonition type nodes to admonition:

def visit_note(self, node: Element) -> None:
self.visit_admonition(node, 'note')

@chrisjsewell chrisjsewell changed the title [rst] Add collabsible option to admonition directives [rst] Add collapsible option to admonition directives Jul 19, 2024
@chrisjsewell
Copy link
Member Author

It's worth documenting / illustrating the admonition+:class: approach. That's something really helpful, but not easily discoverable.

yep, apready do this in myst-parser 😉 https://myst-parser.readthedocs.io/en/latest/syntax/admonitions.html#admonition-titles

This tactic might be theme-specific. It depends on how the CSS for a theme is structured.

it would only be theme specific, if they overrode the HTML writer, which I don't really think they should do

@2bndy5

This comment was marked as off-topic.

@2bndy5
Copy link

2bndy5 commented Jul 23, 2024

Getting back on topic...

It makes sense to have a separate :open: option in sphinx-design because the .. dropdown:: directive already implies a specific behavior. In this patch, the specific behavior is described by the admonition directive's :collapsible: option. Whether or not the admonition is open or closed by default is a parameter specific to the :collapsible: behavior.

Is this my assessment here accurate? Or is my sphinx-immaterial bias just colliding with @chrisjsewell sphinx-design bias?

I'm also interested with what to do about the todo admonitions (in sphinx.ext.todo). 🤓

@Carreau
Copy link
Contributor

Carreau commented Jul 25, 2024

I would tend to gravitte toward :collapsible: <bool>, :open: <bool> than having the presence/abscense of :collapsible:/:open: set the value. Being able to set a value to false is much more discoverable for users, that is to say we can have a example in theme that say:

.. admonition::
   :collapsible: false 
   :open: true

And let people play with true|false

Then say "you can add a :collapsible:" option.

I think that :open: <bool> also make sens, as you may want to have some things collapsed by default (solutions to an exercise). Or expanded by default.

OTOH, I can see what you don't want an :open: as non-collapsible but open don't make sens.

One thing that is not discussed is wether we can set the default state globally.

maybe both collapsible and open should be a 3 state true|false|default, and globally someone can say "I want all to be collapsible and open", and then on a per directive basis :collapsible: and :open: would be overwrites. In which case it make sens to allow boolean for each as otherwise you can't overwrite.

@2bndy5
Copy link

2bndy5 commented Jul 25, 2024

I'm afraid true/false may not be deterministic enough. It could be mistakenly perceived as "disable or enable the collapsible feature." Whereas open/<not open> should unmistakably describe the collapsible state.

I'm not that adamant about what the option's value is. Rather, I'm more adamant about having a value instead of 2 new options to control the same feature.

OTOH, I can see what you don't want an :open: as non-collapsible but open don't make sens.

exactly

@2bndy5
Copy link

2bndy5 commented Jul 25, 2024

One thing that is not discussed is wether we can set the default state globally.

Since this feature directly relates to HTML <details> elements, I think having the default set to closed aligns with the <details> element's default when no open attribute is specified. I think adding a new sphinx config (or allowing reserved env var) might be over-complicating this entirely.

I did come across an implied suggestion that the sphinx-immaterial theme's custom admonitions feature's configuration should allow certain admonition directives to be collapsible by default. I wasn't interested in maintaining the idea because that theme's implementation for collapsible behavior adds requirements about other admonition properties (like requiring a title and requiring 2 directive arguments + directive content for the version directives).

@trallard
Copy link

trallard commented Jul 25, 2024

Since this feature directly relates to HTML <details> elements, I think having the default set to closed aligns with the <details> element's default when no open attribute is specified

I agree with having the default behaviour as collapsed and allowing the users to change the state of a certain accordion (collapsible admonition) if needed.

As for the state wording/indicator, I'd like to suggest collapsed/expanded instead of open/not open
these are unambiguous terms (and I am generally biased towards descriptions based on what something is vs what is not).

@2bndy5
Copy link

2bndy5 commented Jul 25, 2024

I'd like to suggest collapsed/expanded instead of open/not open

I have no problems with that idea. I think the open term came from the HTML specifications for the <details> element. I don't think it matters what the value is for a "collapsed" state. According to the HTML specs, a <details> element is only expanded by default if it has an open attribute; it does not matter what the HTML attribute's value is, only that the open attribute is present or not.

<details><summary>Title</summary>

collapsed by default.

</details>

<details open="false"><summary>Title</summary>

expanded by default.

</details>

renders as:

Title

collapsed by default.

Title

expanded by default.

This is why the term open has been used in previous RST implementations of the <details> HTML element.

@trallard
Copy link

I understand that is the HTML specification and that should be followed.
Though my suggestion came from

I'm afraid true/false may not be deterministic enough. It could be mistakenly perceived as "disable or enable the collapsible feature." Whereas open/ should unmistakably describe the collapsible state.

I'm not that adamant about what the option's value is. Rather, I'm more adamant about having a value instead of 2 new options to control the same feature.

As this is an option to be surfaced to the user to control whether a collapsible admonition would be presented as collapsed/expanded by default in a documentation page (not globally).

@electric-coder
Copy link

electric-coder commented Aug 17, 2024

I'm afraid true/false may not be deterministic enough.

Sphinx's extensions often go for True/False e.g. napoleon; autodoc (here specifying an option without value is common); and also for HTML options so the argument should be made that adding special wording would be an inconsistency thus making the option harder to use because users will have to check the docs for the name instead of going with the prevalent logic.

@2bndy5
Copy link

2bndy5 commented Aug 17, 2024

Keeping the vocabulary simple would be more cohesive with non-English speakers as well. Behaviorally speaking, I still think it doesn't matter what the vocabulary is for a "collapsed" state...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants