-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
manual: add links to subsection anchors #9101
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good and I think this is fine to merge. (I made a super-nitpicky code remark.)
This sounds like a feature that any Hevea user could be interested in. Have you considered sending a PR to the Hevea upstream to use your modified book
class?
With your current style proposal, the title is displayed just as before, but it is in fact clickable, as can be noticed on hover. I tend prefer a slightly different user interaction/interface where the link itself is not clickable, but there is a small anchor symbol shown on hover. For example, the Coq manual does this -- but I've seen it in many places. We could merge the present PR and tune the style afterwards.
The anchors as generated are section numbers. In particular, they cannot be expected to be stable from one version of the manual to the next. Would it be conceivable to use either a mangling of the title, or the LaTeX label of the section, to name the anchor? |
In fact, I hesitated with the style that you propose. I am not sure which option is the best. But we certainly could go for consistency with Coq manual. |
Yes, I make sure to always manually change my URL to the version-specific manual before giving a link, but having links somewhat stable across versions would be nice, and the resulting URLs would be more readable. I agree it's out of the scope of the present PR. |
This markups titles as |
\newcommand{\@titlesecanchor}{\@open{a}{href="\#\@sec@id@attr"}} | ||
\@makesection | ||
{\part}{-2}{part} | ||
{\@opencell{class="center"}{}{}\@open{h1}{\@book@attr{part}}}% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what opencell
generates but center
is not a very good class name. part
would be more approriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would probably be a good idea to do some class name cleaning in a later PR indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About that opencell
I actually had a look at that it seems the toc is still using table layout. Nowadays we have much better layout tool in CSS so it might be worth at a certain point to turn that toc
page into properly nested <ol>
possibly simply seperated by heading for the the different parts.
Sorry the pattern for anchoring is: |
@dbuenzli , do you have a reference for this pattern? The variation <element id=...>... <a href="..."></a></element> seems also quite common (the Coq's manual and the python documentation for instance) |
No. I bet one or the other depends if you want to make it easy to have the anchoring widget on the left or on the right. Here I'd suggest on the left for consistency with |
(Also a good argument to have the anchoring widget on the left is that its position becomes predictable). |
I experimented with @dbuenzli proposition in https://www.polychoron.fr/ocaml-nonmanual/anchors-2/manual032.html . |
I like the result of the experiment slightly better than the current proposal. (I agree that having the anchor on the left is nicer, and I also agree that being consistent with |
After discussing with @shindere , it is not clear if the small-link-on-the-left-version is accessible. |
Florian Angeletti (2019/11/08 07:09 -0800):
After discussing with @shindere , it is not clear if the
small-link-on-the-left-version is accessible.
Well it is likely not, here is why.
To be conform to the WCAG, one requirements is that links make sense not
only in context but also out of context. This is because some assistive
technologies provide a feature that lets users build a list of all the
links a page contains. The links should make sense also when they are
presented in such a list, which would not be the case with an empty or a
one-character link.
|
I suppose we could repeat the section title within the link's |
Gabriel Scherer (2019/11/08 07:15 -0800):
I suppose we could repeat the section title within the link's `<a>`
tag, with CSS styling to hide them?
Well yes, but then one shold make sure the text won't be rendered twice
to the assistive technology.
|
Let's use @Octachron's initial approach for now, that is a reasonable first step and we know for sure is accessible. (It may be possible to give an interactive behavior that is similar to the non-accessible form by using CSS tricks.) |
I'm all for respecting usability guidelines, but in my opinion the link you are creating is equally odd since it basically links to itself. In particular this means you can no longer anchor an element that is a link itself, like all the package names on this page. So I would be curious what is the correct way to do this. |
Btw: there is in some frameworks an "sronly" CSS class that canbe used
to provide text only to the screen reader (SR) but without printing it
on screen. I believe this is implemented by giving the text in this
class an out-of-screen position.
|
@shindere isn't a simpler way to simply properly classify these anchor links with say
|
Indeed, it seems that setting |
@gasche indeed. Note that it seems github's anchors do sport that attribute (not via css though in the generated markup). |
I don't know how well these media rules are honored, to be honest.
|
Gabriel Scherer (2019/11/08 08:01 -0800):
Indeed, it seems that setting `aria-hidden: true;` has the effect of hiding some comment from screen readers and other assistive technologies:
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_aria-hidden_attribute
Yes, sorry, that's the one. sronly is the dual one when you wnat to show
something only to the screen reader.
|
So I think it should be fine (a good compromise) if we use the newer/nicer rendering, and include a @shindere, would you be willing to perform a test on a trial rendering? |
Gabriel Scherer (2019/11/08 08:31 -0800):
So I think it should be fine (a good compromise) if we use the
newer/nicer rendering, and include a `aria-hidden:true;` attribute in
the `section-anchor` class.
No no, you're wrong here @gasche. That's not a CSS thing, as far as I
understand it. It's an HTML attribute that has to go on the a element, I
think.
@shindere, would you be willing to perform a test on a trial
rendering?
Sure.
|
After discussion with @gasche, another option would be to require every section of the manual to have a meaningful label (by altering the section/chapter command to take a compulsory label argument). With this change, we will get both meaningful and stable anchor names |
Florian Angeletti (2019/11/26 07:04 -0800):
After discussion with @gasche, another option would be to require
every section of the manual to have a meaningful label (by altering
the section/chapter command to take a compulsory label argument). With
this change, we will get both meaningful and stable anchor names
Sounds good!
|
I don't think we should wait for all sections to be labelled, or that we should anchor only human-labelled sections (this may not be enough sections); ideally we would have the automatic-counter anchor for non-labelled sections, and the human-chosen label name as anchor of labelled sections. One difficulty with the "labelled" approach is that currently the What I propose to do is therefore the following in the present PR:
If both of those work well, we can merge the present PR, which already presents a real improvement for users. We can finalize the details of the labelled-sections macros and transition over time. |
Will it be so much work to label the sections manually that we need a
transition period? Couldn't that be done in one shoot?
|
63854ee
to
b4abe25
Compare
I took the time to add a label names to every sections, subsections, and subsubsections — while making hevea happy. The result is here. For now, I am using an |
Looks very nice to me, thanks for the tedious work! What needs to be done before merging this? |
Nitpick: one thing I don't find so convincing is the automatic label prefixing of your commands. I think it is clearer what the argument does and how to use it if users have to use |
I agree that the automatic prefixing made the source harder to read. I updated the section/subsection/subsubsection macros accordingly. I also added some documentation for the new behavior of \section, \subsection and \subsubsection in the manual's README. I am planning to merge once CI is green. |
manual: add links to subsection anchors (cherry picked from commit 3f63f9a)
Cherry-picked on 4.10 as bcc64a1 . |
This PR proposes to make every section and subsection titles of manual links to its own anchor.
The major advantage of this change is that it becomes possible to find the anchor name without going down to the html source level.
For instance, consider the current GADT page .
If I need to point someone to the error message parapgrah, currently I have to look to the html source code, find that the corresponding anchor name is
sec261
, and then I can link to the paragraph with https://caml.inria.fr/pub/docs/manual-ocaml/manual033.html#sec261 .With this PR, on the GADT page, I can click on the paragraph title and directly get the link to the paragraph anchor https://www.polychoron.fr/ocaml-nonmanual/anchors/manual032.html#sec261.
It also makes it visible that there are anchors available on the section, subsections and paragraphs.