621078 - API docs should have tables of contents

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: Documentation, defect, P2)

Description

[Will Bamberg [:wbamberg]]

Some of them are quite long, and if you're just looking for a specific API element there can be a lot of scrolling to do.

Comment 2

[Will Bamberg [:wbamberg]]

Attachment: Auto-generated tables of contents

Here's a patch to auto-generate ToCs. The code is adapted from http://www.jankoatwarpspeed.com/post/2009/08/20/Table-of-contents-using-jQuery.aspx . I was going to do it using the Python Markdown processor, but it turns out to be much easier using JQuery. I'm not sure which we should prefer.

I'd be interested to hear if you think it's helpful, and if you have any views on the styling and position of it. I think maybe it could do with a title like "API Reference" or something.

One problem I haven't resolved yet is that it doesn't behave nicely when the main content column has a background: see "request" or, worse, "tabs" (although "tabs" renders nicely in Chrome :-(). I'll work on that but if you know the answer off-hand that would be really helpful.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 521646 [details] [diff] [review]
Auto-generated tables of contents

(In reply to comment #2)
> Here's a patch to auto-generate ToCs. The code is adapted from
> http://www.jankoatwarpspeed.com/post/2009/08/20/Table-of-contents-using-jQuery.aspx
> . I was going to do it using the Python Markdown processor, but it turns out to
> be much easier using JQuery. I'm not sure which we should prefer.

As long as AMO folks are cool with jQuery in the docs, this is fine.


> I'd be interested to hear if you think it's helpful, and if you have any views
> on the styling and position of it. I think maybe it could do with a title like
> "API Reference" or something.

I think it's helpful!  However, I wouldn't limit it to the API reference; I'd make it a table for all the contents on the page, i.e. include all the stuff before the API reference, like Events and Tab Enumeration for the tabs module.

The position seems right.

The style is ok.  I like the use of bold for section headings.  But I would position it below the page title and make it narrower to make it a bit less obtrusive.

I don't otherwise have specific feedback, but I recommend looking at the way MDC does Tables of Contents for inspiration, for example on the Error page <https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Error>.

It definitely needs a heading.  I recommend "Table of Contents"! :-)


> One problem I haven't resolved yet is that it doesn't behave nicely when the
> main content column has a background: see "request" or, worse, "tabs" (although
> "tabs" renders nicely in Chrome :-(). I'll work on that but if you know the
> answer off-hand that would be really helpful.

Erm, indeed.  Unfortunately, I'm not sure what's going on there.

Comment 4

[Will Bamberg [:wbamberg]]

Thanks for the feedback. There were a couple of things I wanted to argue about, but I'm happy to make the changes if I can't change your mind.
 
> > I'd be interested to hear if you think it's helpful, and if you have any views
> > on the styling and position of it. I think maybe it could do with a title like
> > "API Reference" or something.
> 
> I think it's helpful!  However, I wouldn't limit it to the API reference; I'd
> make it a table for all the contents on the page, i.e. include all the stuff
> before the API reference, like Events and Tab Enumeration for the tabs module.
 
Hm. It was intentional to limit it to the API reference, to be a quick overview of the scope of the API, and a way of getting to individual function documentation quickly. So I guess it's not really a Table of Contents exactly. I think it's really nice to be able to see the whole scope of a given module's API at a glance. But if it's terribly unintuitive then I'm OK to make it like a traditional ToC.

> The position seems right.
> 
> The style is ok.  I like the use of bold for section headings.  But I would
> position it below the page title and make it narrower to make it a bit less
> obtrusive.

I'm not sure how the position can be right, but it should be positioned under the page title. I think it would look a bit weird under the page title. In the latest version I've put it in a separate column over on the right, which also fixes this problem...

I also made it narrower and reduced the font size a bit. Some elements (in particular function signatures) can get quite long, but as long as most of them don't overflow then it's OK for a few of them to I think.

> > One problem I haven't resolved yet is that it doesn't behave nicely when the
> > main content column has a background: see "request" or, worse, "tabs" (although
> > "tabs" renders nicely in Chrome :-(). I'll work on that but if you know the
> > answer off-hand that would be really helpful.
> 
> Erm, indeed.  Unfortunately, I'm not sure what's going on there.

(The MDC docs also have this problem: see https://developer.mozilla.org/en/Using_nsILoginManager for example.) But as above: it's fixed here by just putting it in a different column.

I've pushed the changes to https://github.com/wbamberg/jetpack-sdk/tree/621078.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #4)
> I'm not sure how the position can be right, but it should be positioned under
> the page title. I think it would look a bit weird under the page title. In the
> latest version I've put it in a separate column over on the right, which also
> fixes this problem...

Erm, indeed, I was too vague.  What I was trying to say is that the horizontal position (on the right-hand side of the main content column, with the main content wrapping around it) seems right, but the vertical positioning (top edge at the same height as the top edge of the page title) seems too high.

Basically, I was trying to recommend the kind of placement that ToCs have on MDC.  Now that you have the ToC in a separate column, the horizontal position still seems fine (better in some ways, worse in others, but overall good), but the vertical position still seems too high.  I would move the box down so that its top edge is at the same height as the top edge of the Docs ToC column on the left-hand side.

(As a separate issue that is perhaps outside the scope of this bug, I might style the page title with a border and background color that stretches across the width of the page, including both the width of the main content column and the width of the API Reference ToC right-hand column, to distinguish it better from the content it entitles and make it clear that it entitles both the content of the page and the API Reference ToC but not the Docs ToC column, the latter of which is meta-information outside the scope of the particular page.)


> I also made it narrower and reduced the font size a bit. Some elements (in
> particular function signatures) can get quite long, but as long as most of them
> don't overflow then it's OK for a few of them to I think.

It looks great!

Comment 6

[Will Bamberg [:wbamberg]]

Attachment: Patch for review

I've adjusted the vertical position of the ToC so it aligns with the sidebar.

I think it's a good suggestion about the title as well, but it won't happen today: so if you'd like what's here without that, we can potentially land this patch, otherwise I can add that when I get back - or we can just wait till the real graphic designer has a go.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 523730 [details] [diff] [review]
Patch for review

>diff --git a/static-files/base.html b/static-files/base.html

>-<div id="container">
>-  <div id="columns">
>+  <div id="container">
> 
>-  <div id="left-column">
>+  <div id="main-content" class="column"></div>
>+
>+  <div id="sidebar" class="column">

Nit: it'd be nice to preserve indentation of the container relative to its children (although presumably this would pollute the patch with whitespace-only changes).


>diff --git a/static-files/css/sdk-docs.css b/static-files/css/sdk-docs.css

>+  min-width: 660px;         /* 2 x sidebar fullwidth + toc fullwidth + main-content padding*/
...
>+  padding-right: 205px;     /* toc width + main-content padding*/

Nit padding*/ -> padding */


>-  margin-right: -10%;
>+  margin-right: 0%;

Nit: zeroes are commonly specified in Mozilla CSS without a unit marker, i.e.:

  margin-right: 0;


>+#toc a[title = H6] {
>+  margin-left: 3em;
>+  font-family: "andale mono",monospace;

Nit: space after comma.


>diff --git a/static-files/js/main.js b/static-files/js/main.js

>+  function generateToC() {
>+    var select = '.api_reference  h2,.api_reference h3, .api_reference h4, ' +
>+                 '.api_reference h5, .api_reference h6';

Nit: consistent space after commas, and remove extra space before h2.


>+    if ($(select).length == 0) {
>+      $("#toc").hide();
>+      return;
>+    }
>+    $(select).each(function(i) {
>+      var link = document.location + "#title" + i;
>+      var current = $(this);
>+      var dataTypeStart = current.html().indexOf(" : ");
>+      var tocName = current.html();
>+      if (dataTypeStart != -1)
>+        tocName = tocName.slice(0, dataTypeStart);
>+      current.attr("id", "title" + i);
>+      $("#toc").append("<a id='link" + i + "' href='" +
>+        link + "' title='" + current.attr("tagName") + "'>" +
>+        tocName + "</a>");
>+      });
>+  }

This creates anchors whose names depend on the order in which the sections appear in the reference, which is brittle against changes in that order and makes the URLs opaque.

On MDN, anchors are named after the section titles, with numbers being used only to distinguish between sections that have identical titles (like the Properties and Methods sections in the docs on Error <https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Error>).

That seems more resilient against changes, although perhaps the most resilient approach (modulo changes to our information architecture) would be to reflect the header hierarchy into the anchors, so the show() method in the Panel docs has the anchor #Classes_Panel_Methods_show.

Naming anchors after section titles seems relatively straightforward to implement (the code would just need to keep a hash of titles that have been used and how many times they have been used in order to assign the correct name to each title), while reflecting the hierarchy seems complicated to implement, so I'm ok with doing the former, but it does seem like we should do one of those two things.

I'm open to differing opinions however, so let me know if you think I'm off-base.

Comment 8

[Will Bamberg [:wbamberg]]

> 
> >+    if ($(select).length == 0) {
> >+      $("#toc").hide();
> >+      return;
> >+    }
> >+    $(select).each(function(i) {
> >+      var link = document.location + "#title" + i;
> >+      var current = $(this);
> >+      var dataTypeStart = current.html().indexOf(" : ");
> >+      var tocName = current.html();
> >+      if (dataTypeStart != -1)
> >+        tocName = tocName.slice(0, dataTypeStart);
> >+      current.attr("id", "title" + i);
> >+      $("#toc").append("<a id='link" + i + "' href='" +
> >+        link + "' title='" + current.attr("tagName") + "'>" +
> >+        tocName + "</a>");
> >+      });
> >+  }
> 
> This creates anchors whose names depend on the order in which the sections
> appear in the reference, which is brittle against changes in that order and
> makes the URLs opaque.
> 
> On MDN, anchors are named after the section titles, with numbers being used
> only to distinguish between sections that have identical titles (like the
> Properties and Methods sections in the docs on Error
> <https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Error>).
> 
> That seems more resilient against changes, although perhaps the most resilient
> approach (modulo changes to our information architecture) would be to reflect
> the header hierarchy into the anchors, so the show() method in the Panel docs
> has the anchor #Classes_Panel_Methods_show.
> 
> Naming anchors after section titles seems relatively straightforward to
> implement (the code would just need to keep a hash of titles that have been
> used and how many times they have been used in order to assign the correct name
> to each title), while reflecting the hierarchy seems complicated to implement,
> so I'm ok with doing the former, but it does seem like we should do one of
> those two things.
> 
> I'm open to differing opinions however, so let me know if you think I'm
> off-base.

Well, I thought about this at the time. I felt that this code isn't intended
to produce general-purpose anchors, it's only intended to produce ToCs, that's
why I didn't do it.

So I think this is mostly a question about the scope of this bug.

But it would be nice to be able to link to specific API elements from other
pages, so maybe it is worth considering here, while we are doing this.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 523730 [details] [diff] [review]
Patch for review

(In reply to comment #8)
> Well, I thought about this at the time. I felt that this code isn't intended
> to produce general-purpose anchors, it's only intended to produce ToCs, that's
> why I didn't do it.
> 
> So I think this is mostly a question about the scope of this bug.
> 
> But it would be nice to be able to link to specific API elements from other
> pages, so maybe it is worth considering here, while we are doing this.

I see your point.  I do think it's worth doing, but it's something we can tackle in a separate bug.  We should just make sure not to try to link to any of these anchors until then (including on the public web).

In which case r=myk, as the only other issues were nits.

Comment 10

[Will Bamberg [:wbamberg]]

Attachment: ToC placed inline, sensible anchor names

Here is yet another patch for tables of contents.

This one places the ToCs inside the flow of text, like the ones on MDC. I think this is more readable than having them in a separate column. I managed to get syntaxhighlighter to behave (although had to change syntaxhighlighter to do so).

It also generates names for anchors based on the heading name, although only using the first, simple, scheme you suggested.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 528227 [details] [diff] [review]
ToC placed inline, sensible anchor names

Inline ToCs and non-arbitrary anchor names for the win!

Just a few issues to resolve...

>diff --git a/static-files/js/main.js b/static-files/js/main.js
>index 8a7a12e..c80a53a 100644
>--- a/static-files/js/main.js
>+++ b/static-files/js/main.js
>@@ -41,6 +41,52 @@ function run(jQuery) {
>     $(currentSideBarSection).show();
>   }
> 
>+  function generateToC() {
>+    var headings = '.api_reference  h2,.api_reference h3, .api_reference h4, ' +
>+                   '.api_reference h5, .api_reference h6';

Nit:

    var headings = '.api_reference h2, .api_reference h3, .api_reference h4, ' +
                   '.api_reference h5, .api_reference h6';


>+      // slugify the name of the heading
>+      slugifiedBaseName = slugify(baseName);
>+      // uniqueify the name of the heading
>+      suffixedName = slugifiedBaseName;
>+      headingIdExists = headingIds.indexOf(suffixedName) != -1;
>+      while(headingIdExists) {
>+        suffix++;
>+        suffixedName = baseName + suffix;
>+        headingIdExists = headingIds.indexOf(suffixedName) != -1;
>+        }

Nit: capitalize all-caps acronyms like ID, even in camelCasedIdentifiers (unless they are the initial word in the identifier, in which case lower-case them), i.e.: headingIdExists -> headingIDExists.

Nit: space between while and opening parenthesis: while (

Nit: line up a closing parenthesis with its opening statement, both for this `while` loop and for the enclosing |$(headings).each(function(i) {| block, i.e.:

    $(headings).each(function(i) {
      ...
      while (headingIdExists) {
        ...
      }
      ...
    });

Nit: this adds "1" to the second duplicate anchor.  On MDC, the second one has "2" appended to it, which seems more intuitive, and it separates the anchor text and the number with an underscore (i.e. "Properties_2"), which makes it easier to read.

Non-nit: declare slugifiedBaseName, suffixedName, and headingIdExists with "var" so they aren't unintentionally global variables.

Non-nit: if I read this code correctly, it uses the slugified name only for the first duplicate anchor on a page, since `suffixedName` is composed from `baseName` rather than `slugifiedBaseName` within the `while` loop.  Presumably this needs to be (change delimited by ***):

      while(headingIdExists) {
        suffix++;
        suffixedName = ***slugifiedBaseName*** + suffix;
        headingIdExists = headingIds.indexOf(suffixedName) != -1;
        }

Nevertheless, I don't think you need to create slugified names at all, since all characters are valid in URLs as long as they are properly encoded, which you should be able to do by calling `encodeURIComponent` on `suffixedName` before you append it to the URL.


>+      $("#toc").append("<a id='link" + i +
>+                       "' href='" + document.location + "#" +
>+        suffixedName + "' title='" + $(this).attr("tagName") + "'>" +
>+        baseName + "</a>");

What's the value of adding |id='link[n]'| to these entries?  It makes it possible to link directly to a ToC entry itself via http://example.com/foo#link[n], but it's not clear why we would want to do that.

Also, generating absolute URLs with |document.location + "#" + suffixedName| while on a page whose URL already has an anchor results in double anchors.  For example, if I load http://example.com/foo.html#foo, and then I click the entry for the "bar" section, I get http://example.com/foo.html#foo#bar.

Instead, just make the value of `href` be the anchor itself (f.e. "#foo"), as that'll resolve as a relative URL to the anchor in the current page.

Also, what's the value of making `title` be the tag name?  That results in a tooltip containing the tag name when I hover over an entry, which doesn't seem helpful.

Also, nit: it's better to line up such multi-line statements consistently, because easier to read, i.e.:

      $("#toc").append("<a id='link" + i +
                       "' href='" + document.location + "#" +
                       suffixedName + "' title='" + $(this).attr("tagName") +
                       "'>" + baseName + "</a>");

Or alternately:

      $("#toc").append(
        "<a id='link" + i + "' href='" + document.location + "#" +
        suffixedName + "' title='" + $(this).attr("tagName") + "'>" +
        baseName + "</a>"
      );

Finally, I don't see how anyone can exploit this, but it's probably worth escaping HTML in the text that you insert as the child of the <a> tag.  The simplest way to do this is to create the tag first, then use its .text() method to insert the text, i.e.:

      var link = $("<a id='link" + i + "' href='" + document.location + "#" +
                   suffixedName + "' title='" + $(this).attr("tagName") + "'>" +
                   "</a>");
      link.text(baseName);
      $("#toc").append(link);


>diff --git a/static-files/syntaxhighlighter/styles/shCore.css b/static-files/syntaxhighlighter/styles/shCore.css

> .syntaxhighlighter {
>-  width: 100% !important;
>+  width: 100%;

Might we be able to upstream this?

Comment 12

[Will Bamberg [:wbamberg]]

Attachment: Fixed some code review problems

Comment 13

[Will Bamberg [:wbamberg]]

Thanks for the review. I've attached a new patch addressing most of these.

But...

> Also, generating absolute URLs with |document.location + "#" + suffixedName|
> while on a page whose URL already has an anchor results in double anchors.  For
> example, if I load http://example.com/foo.html#foo, and then I click the entry
> for the "bar" section, I get http://example.com/foo.html#foo#bar.
> 
> Instead, just make the value of `href` be the anchor itself (f.e. "#foo"), as
> that'll resolve as a relative URL to the anchor in the current page.

This does not work for me. That is, a ToC entry like this:

      var tocEntry = $("<a " +
                       "href='#" + encodedName + "' " +
                       "class='" + $(this).attr("tagName") + "' " +
                       "title='" + baseName + "'></a>");

...will give me links like:

http://127.0.0.1:8888/#search%28options%29

...rather than the desired:

http://127.0.0.1:8888/packages/addon-kit/docs/passwords.html#search%28options%29

I do not know what stupid thing I am doing, that causes this. I think it might be a side effect of the <base> tag:

http://stackoverflow.com/questions/1889076/is-it-recommended-to-use-the-base-html-tag

> Also, what's the value of making `title` be the tag name?  That results in a
> tooltip containing the tag name when I hover over an entry, which doesn't seem
> helpful.

This was blindly copied from the example. It's to enable the CSS to apply different styles for different headings, but a class attribute seems better for that so I changed it in this patch.

> >diff --git a/static-files/syntaxhighlighter/styles/shCore.css b/static-files/syntaxhighlighter/styles/shCore.css
> 
> > .syntaxhighlighter {
> >-  width: 100% !important;
> >+  width: 100%;
> 
> Might we be able to upstream this?

Yeah, I wasn't happy about having to do this. However, !important means it's impossible to manipulate it with jQuery, as far as I could tell. I hope it's not, you know, important. Worth asking, anyway, especially as MDC suffers from the same problem.

Comment 14

[Will Bamberg [:wbamberg]]

Attachment: Fixed a capitalization problem; removed an unused variable

Comment 15

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #13)
> This does not work for me. That is, a ToC entry like this:
> 
>       var tocEntry = $("<a " +
>                        "href='#" + encodedName + "' " +
>                        "class='" + $(this).attr("tagName") + "' " +
>                        "title='" + baseName + "'></a>");
> 
> ...will give me links like:
> 
> http://127.0.0.1:8888/#search%28options%29
> 
> ...rather than the desired:
> 
> http://127.0.0.1:8888/packages/addon-kit/docs/passwords.html#search%28options%29
> 
> I do not know what stupid thing I am doing, that causes this. I think it might
> be a side effect of the <base> tag:
> 
> http://stackoverflow.com/questions/1889076/is-it-recommended-to-use-the-base-html-tag

Hmm, indeed.  In that case, the short-term fix is to construct an absolute URL from pieces of the current URL:

      var url = document.location.protocol + "//" +
                document.location.host +
                document.location.pathname +
                document.location.search +
                "#" + encodedName;
      // now add the ID attribute and ToC entry
      $(this).attr("id", suffixedName);
      var tocEntry = $("<a href='" + url + "' " +
                       "class='" + $(this).attr("tagName") + "' " +
                       "title='" + baseName + "'></a>");

The long-term fix is probably to stop using <base> tags.


One more thing: encodeURIComponent actually doesn't encode single quotation marks, although it does encode double quotation marks, so the encapsulation of quotation marks should be reversed here, otherwise a URL with a single quotation mark will break this string concatenation.

If you want to be absolutely certain such bugs won't creep into this code, set tocEntry's attributes with the attr() method:

      // now add the ID attribute and ToC entry
      $(this).attr("id", suffixedName);
      var tocEntry = $("<a></a>").attr({
        href: url,
        "class": $(this).attr("tagName"),
        title: baseName
      });
      ...


> > > .syntaxhighlighter {
> > >-  width: 100% !important;
> > >+  width: 100%;
> > 
> > Might we be able to upstream this?
> 
> Yeah, I wasn't happy about having to do this. However, !important means it's
> impossible to manipulate it with jQuery, as far as I could tell. I hope it's
> not, you know, important. Worth asking, anyway, especially as MDC suffers from
> the same problem.

Hmm, ok.  Interesting.  I would think jQuery would be able to manipulate !important rules.


One more "one more thing"!

Currently, if you load a URL with an anchor in it, Firefox doesn't scroll to the anchor.  I think that's because the anchors aren't added until after Firefox has parsed the DOM, and Firefox decides whether or not to scroll to an anchor while parsing the DOM.

I can't think of an elegant solution to this problem, but a hack is to do the following after generating the table of contents:

  // Make Firefox jump to the anchor even though we created it after it
  // parsed the page.
  if (document.location.hash) {
    var hash = document.location.hash;
    var url = document.location.protocol + "//" +
              document.location.host +
              document.location.pathname +
              document.location.search;
    document.location.replace(url + "#");
    document.location.replace(url + hash);
  }


And one last nit:

    var headings = '.api_reference  h2, .api_reference h3, .api_reference h4, ' +
                   '.api_reference h5, .api_reference h6';

There's still a double space on that first line, which happens to push the line width to 81 columns, one more than our convention for line length.

(Yes, I know I'm being crazy pedantic.)

Comment 16

[Will Bamberg [:wbamberg]]

Attachment: Fixed more code review problems

Thanks for all your help with this. Crazy pedantic is fine.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 528609 [details] [diff] [review]
Fixed more code review problems

Review of attachment 528609 [details] [diff] [review]:

Looks and works great, r=myk!

::: static-files/js/main.js
@@ +91,5 @@
+  if (document.location.hash) {
+    var hash = document.location.hash;
+    document.location.replace(pageURL + "#");
+    document.location.replace(pageURL + hash);
+    }

Nit: indent closing brace like opening brace's line:

  if (document.location.hash) {
    var hash = document.location.hash;
    document.location.replace(pageURL + "#");
    document.location.replace(pageURL + hash);
  }

Comment 18

[Brian Warner [:warner :bwarner]]

Attachment: fix test_apirenderer.py by updating the expected output file

The test_apirenderer.py test is failing on trunk after the main patch was landed: a lot of output lines now say "<h5>" or "<h6>" where before they were just <div>s. This patch updates the expected output to match the actual output (and adds some line numbers to the failure message to make it easier to see what's going on).

This makes the test pass, but I don't know if it's correct: perhaps the previous patch has accidentally changed the output to use h5/h6, and the correct fix is to change the renderer instead of the expected output.

Comment 19

[Will Bamberg [:wbamberg]]

Comment on attachment 528669 [details] [diff] [review]
fix test_apirenderer.py by updating the expected output file

The change to use h5 and h6 headings is intentional, and this patch looks right. Sorry, I should have caught that.

Comment 20

[Brian Warner [:warner :bwarner]]

Cool, ok, just need a+ from myk or dcm, then I'll land it. Thanks!

Comment 21

[Brian Warner [:warner :bwarner]]

Test fix landed in https://github.com/mozilla/addon-sdk/commit/50e40c3c081b2d1f3a64efe84b1f48d5a744d0bb

Comment 22

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 528669 [details] [diff] [review]
fix test_apirenderer.py by updating the expected output file

Note: I gave a=myk on IRC.