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

manual: add links to subsection anchors #9101

Merged
merged 7 commits into from Dec 6, 2019
Merged

Conversation

@Octachron
Copy link
Member

Octachron commented Nov 7, 2019

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.

@gasche
gasche approved these changes Nov 7, 2019
Copy link
Member

gasche left a comment

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.

manual/manual/macros.hva Outdated Show resolved Hide resolved
@gasche

This comment has been minimized.

Copy link
Member

gasche commented Nov 7, 2019

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?

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Nov 7, 2019

@trefis, @Gbury: it's nice to use the full nuance of modern user interfaces by picking your favorite emoticon to comment on the PR, but it doesn't actually let us make much forward progress; a review does. Consider doing it next time!

@Octachron

This comment has been minimized.

Copy link
Member Author

Octachron commented Nov 7, 2019

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.
Submitting the modified book class is certainly on the table (at first I was looking to patch hevea to add the feature).
Before generating stable anchor names, we would need stable manual pages name too. I am not sure how time consuming both features would be. Moreover, since we are versioning the manual, it should be possible to have stable links to a precise version of the manual.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Nov 7, 2019

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.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

dbuenzli commented Nov 7, 2019

This markups titles as <h1><a id="bla">title</a></h1> it would be better to have <h1><a id="bla"></a>title</h1>. The latter enables to devise a css style such that anchors appear on hover e.g. like github does for markdown headers or odoc does for structure items and section anchors (see e.g. here).

\newcommand{\@titlesecanchor}{\@open{a}{href="\#\@sec@id@attr"}}
\@makesection
{\part}{-2}{part}
{\@opencell{class="center"}{}{}\@open{h1}{\@book@attr{part}}}%

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Nov 7, 2019

Contributor

Not sure what opencell generates but center is not a very good class name. part would be more approriate.

This comment has been minimized.

Copy link
@Octachron

Octachron Nov 8, 2019

Author Member

It would probably be a good idea to do some class name cleaning in a later PR indeed.

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Nov 8, 2019

Contributor

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.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

dbuenzli commented Nov 7, 2019

<h1><a id="bla"></a>title</h1>.

Sorry the pattern for anchoring is: <element id="myid"><a href="#myid"></a> ...</element>

@Octachron

This comment has been minimized.

Copy link
Member Author

Octachron commented Nov 8, 2019

@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)

@dbuenzli

This comment has been minimized.

Copy link
Contributor

dbuenzli commented Nov 8, 2019

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 odoc.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

dbuenzli commented Nov 8, 2019

(Also a good argument to have the anchoring widget on the left is that its position becomes predictable).

@Octachron

This comment has been minimized.

Copy link
Member Author

Octachron commented Nov 8, 2019

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Nov 8, 2019

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 odoc is a nice plus.)

@Octachron

This comment has been minimized.

Copy link
Member Author

Octachron commented Nov 8, 2019

After discussing with @shindere , it is not clear if the small-link-on-the-left-version is accessible.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Nov 8, 2019

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Nov 8, 2019

I suppose we could repeat the section title within the link's <a> tag, with CSS styling to hide them?

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Nov 8, 2019

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Nov 8, 2019

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.)

@dbuenzli

This comment has been minimized.

Copy link
Contributor

dbuenzli commented Nov 8, 2019

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.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Nov 8, 2019

@dbuenzli

This comment has been minimized.

Copy link
Contributor

dbuenzli commented Nov 8, 2019

@shindere isn't a simpler way to simply properly classify these anchor links with say class='anchor' and have the following media rule:

@media only speech {
.anchor { display: none }
}
@gasche

This comment has been minimized.

Copy link
Member

gasche commented Nov 8, 2019

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

@dbuenzli

This comment has been minimized.

Copy link
Contributor

dbuenzli commented Nov 8, 2019

@gasche indeed. Note that it seems github's anchors do sport that attribute (not via css though in the generated markup).

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Nov 8, 2019

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Nov 8, 2019

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Nov 8, 2019

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.

@shindere, would you be willing to perform a test on a trial rendering?

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Nov 8, 2019

@Octachron

This comment has been minimized.

Copy link
Member Author

Octachron commented Nov 26, 2019

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

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Nov 26, 2019

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Nov 26, 2019

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 \label{..} command comes later in the LaTeX document, its information is not available at the place where the section HTML is generated. (As a result, the anchor generated later points "right after" the section heading, which is annoying.) To make human-chosen-label-anchoring work well, we have to build a macro that bundles the section name with the label (\labsection{<label>}{<title>}, etc.). It's not completely obvious to convince ourselves that this strategy, once we start applying it (gradually) to manual sections, will play nicely with the automatic-counter-anchor scheme in the current implementation.

What I propose to do is therefore the following in the present PR:

  • Implement the accessibility stuff.
  • Do a prototype of \labsection command, use it for exactly one section of the manual, and check that the hybrid anchoring works as expected.

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.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Nov 26, 2019

@Octachron Octachron force-pushed the Octachron:manual_anchor branch from 63854ee to b4abe25 Dec 5, 2019
@Octachron

This comment has been minimized.

Copy link
Member Author

Octachron commented Dec 5, 2019

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 aria-hidden attribute. I have also added cutnames to the extension chapter to have explicit names everywhere.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 5, 2019

Looks very nice to me, thanks for the tedious work! What needs to be done before merging this?

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 5, 2019

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 \subsection{ss:foo}{Foo} rather than \subsection{foo}{Foo}. The latter make the whole thing looks more magical, and harder to discover how to reference things. It does also improve consistency, but I'm not convinced that this is a big win.

@Octachron

This comment has been minimized.

Copy link
Member Author

Octachron commented Dec 6, 2019

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.

@Octachron Octachron merged commit 3f63f9a into ocaml:trunk Dec 6, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Octachron added a commit that referenced this pull request Dec 6, 2019
manual: add links to subsection anchors
(cherry picked from commit 3f63f9a)
@Octachron

This comment has been minimized.

Copy link
Member Author

Octachron commented Dec 6, 2019

Cherry-picked on 4.10 as bcc64a1 .

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

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.