-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
JDK-8320458: Improve structural navigation in API documentation #17062
Conversation
👋 Welcome back hannesw! A progress list of the required criteria for merging this PR into |
import jdk.javadoc.internal.doclets.formats.html.Navigation.PageMode; | ||
import jdk.javadoc.internal.doclets.formats.html.markup.Text; |
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.
Quibble: order of imports?
.setNavLinkClass(classLinkContent); | ||
List<Content> subnavLinks = new ArrayList<>(); | ||
if (configuration.showModules) { | ||
ModuleElement mdle = configuration.docEnv.getElementUtils().getModuleOf(typeElement); |
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's more common to use utils.elementUtils
than the expression configuration.docEnv.getElementUtils()
@@ -122,14 +122,4 @@ static class SerializedForm { | |||
static class TypeUse { | |||
static final TagName SUMMARY_HEADING = TagName.H2; | |||
} | |||
|
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.
Yay!
/** | ||
* A utility class for building nested HTML lists. This class is implemented as a | ||
* stack of nested list elements where list items are added to the inner-most | ||
* list. Lists can be added to and removed from the stack using the {@code pushNested} | ||
* and {@code popNested} methods. | ||
*/ |
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.
Nice!
Webrevs
|
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.
Build changes look good given that the stylesheet workaround is no longer needed.
/reviewers 2
@hns This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 621 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java
Outdated
Show resolved
Hide resolved
…mats/html/HtmlDocletWriter.java Co-authored-by: Andrey Turbanov <turbanoff@gmail.com>
@@ -350,7 +350,7 @@ protected void addLinksForIndexes(List<Character> allFirstCharacters, Content co | |||
} | |||
|
|||
content.add(new HtmlTree(TagName.BR)); | |||
List<Content> pageLinks = Stream.of(IndexItem.Category.values()) | |||
List<? extends Content> pageLinks = Stream.of(IndexItem.Category.values()) |
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.
List<? extends Content>
is not wrong and is better than List<Content>
but would var
be simpler and even better?
writer.addToTableOfContents(HtmlIds.METHOD_DETAIL, contents.methodDetailLabel); | ||
writer.tocBuilder.pushNested(HtmlTree.OL(HtmlStyle.tocList)); |
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.
Coding-style question:
Rather than have multiple members (methods and fields) on HtmlDocletWriter
would it be better to treat the TOC as a first class item for each page and put all the functionality there? Or maybe that's a separate cleanup for later.
@@ -169,35 +134,28 @@ private void addMainNavLinks(Content target) { | |||
switch (documentedPage) { |
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.
Maybe not now, but I have long felt this big switch statement could be better served as a virtual abstract method, with suitable implementations in each kind of page.
if (element != null && !(element instanceof ModuleElement)) { | ||
addPageElementLink(target); | ||
if (options.classUse()) { | ||
if (element instanceof PackageElement || element instanceof TypeElement) { | ||
addItemToList(target, links.createLink(DocPaths.PACKAGE_USE, contents.useLabel)); | ||
} | ||
} |
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.
As a matter of policy, it is generally incorrect to use instanceof
for subtypes of Element
. The correct coding style is to get and check the ElementKind
.
That being said, the code here will work on the JDK implementation of the Language Model API, but the coding style may not work on other implementations.
if (element != null) { | ||
addPageElementLink(target); | ||
if (options.classUse()) { | ||
if (element instanceof PackageElement pkg) { |
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.
Yeah ... this is where it is nice to use instanceof
patterns, even though the recommended form is to check the ElementKind
and do a separate cast. As I said above, this will work on JDK, but the coding style may not work in other contexts.
.setNavLinkModule(linkContent); | ||
List<Content> subnavLinks = new ArrayList<>(); | ||
if (configuration.showModules) { | ||
ModuleElement mdle = configuration.docEnv.getElementUtils().getModuleOf(packageElement); |
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.
Minor suggestion: use utils.elementUtils
instead of configuration.docEnv.getElementUtils()
""" | ||
<a href="../../../overview-tree.html">Tree</a>""", | ||
<a href="../package-tree.html">Tree</a>""", |
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.
What is the significance of this filename change?
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.
For context-specific links in the top navigation bar we always link to the target closest to the current page; for example, The "USE" link in class documentation links to the Class Use page of the current class, and the "TREE" link in files belonging to a package links to the Package Tree page. This was previously not handled correctly for package-level doc files. It is now fixed, so the test has to be updated.
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.
Thanks for the explanation.
<ol class="toc-list"> | ||
<li><a href="#" tabindex="0">Description</a></li> | ||
<li><a href="#packages-summary" tabindex="0">Packages</a></li> | ||
</ol>"""); |
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.
I had missed this subtlety earlier. The vertical bars have gone from the generated code. Nice!
@@ -576,7 +577,7 @@ void checkHtml5NoDescription(boolean found) { | |||
} | |||
|
|||
void checkModuleLink() { | |||
checkOutput("index.html", true, | |||
checkOutput("index.html", false, |
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.
Interesting change ;-). And yes, I agree.
public void testOverview(Path ignore) { | ||
javadoc("-d", "out-overview", |
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.
Generally, I would recommend renaming Path ignore
to Path base
and then generating a filename for use in the javadoc
call, such as base.resolve("api").toString()
. This helps guarantee the files for each test case are isolated.
@@ -201,57 +172,82 @@ public static class Y extends X { | |||
package pkg1; public interface InterfaceWithNoMembers { | |||
}"""); | |||
|
|||
javadoc("-d", "out-3", | |||
javadoc("-d", "out-navlinks", |
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.
See preceding comment. I would recommend using the equivalent of base.resolve("api:).toString()
here.
<li><a href="#nested-class-summary" tabindex="0">Nested Class Summary</a></li> | ||
<li><a href="#field-summary" tabindex="0">Field Summary</a></li> | ||
<li><a href="#constructor-summary" tabindex="0">Constructor Summary</a></li> | ||
<li><a href="#method-summary" tabindex="0">Method Summary</a></li> | ||
<li><a href="#constructor-detail" tabindex="0">Constructor Details</a> | ||
<ol class="toc-list"> |
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.
General comment, not specific to this instance:
I like that we cleaned up HTML id names in recent releases.
if ("tag-list".equals(attrs.get("class"))) { | ||
inSeeList = true; | ||
String classAttr = attrs.get("class"); | ||
if ("tag-list".equals(classAttr) || "toc-list".equals(classAttr) || "sub-nav-list".equals(classAttr)) { |
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.
This is not wrong, but it might be a good place to use Set.contains
in some manner.
// special handling for strings in .js.template files | ||
for (String fileName : List.of("resources/search.js.template", "resources/script.js.template")) { | ||
FileObject fo = fm.getFileForInput(javadocLoc, | ||
"jdk.javadoc.internal.doclets.formats.html", | ||
fileName); | ||
CharSequence search_js = fo.getCharContent(true); | ||
Pattern p = Pattern.compile("##REPLACE:(?<key>[A-Za-z0-9._]+)##"); | ||
Matcher m = p.matcher(search_js); | ||
while (m.find()) { | ||
results.add(m.group("key")); | ||
} |
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.
See earlier suggestion about possibly merging template-y values into a single file.
|
The last commit (70d06e5) moves the TOC-related methods from This includes some not totally obvious changes:
|
@@ -97,7 +96,7 @@ public ClassWriter(HtmlConfiguration configuration, TypeElement typeElement, | |||
this.classTree = classTree; | |||
|
|||
pHelper = new PropertyUtils.PropertyHelper(configuration, typeElement); | |||
tocBuilder = new ListBuilder(HtmlTree.OL(HtmlStyle.tocList)); | |||
tableOfContents = new TableOfContents(this); |
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.
Is there a reason to do this here (and also in ModuleWriter
and PackageWriter
etc) rather than in the (shared) super-constructor for HtmlDocletWriter
?
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 reason for doing it in the concrete writer classes is that only some HtmlDocletWriter
subclasses have a table of contents, and I don't think there's an easy way to figure out in the super-constructor which instances need a table of contents and which don't. We could have the subclasses pass it as an argument to the super-constructor I guess.
protected final Map<String, String> headings = new LinkedHashMap<>(); | ||
|
||
protected ListBuilder tocBuilder; | ||
protected TableOfContents tableOfContents; |
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.
If you init it in the constructor for HtmlDocletWriter
, the field can be final
*/ | ||
public class ListBuilder extends Content { | ||
|
||
private final HtmlTree root; | ||
private final Deque<HtmlTree> stack = new ArrayDeque<>(); | ||
private final Deque<HtmlTree[]> stack = new ArrayDeque<>(); |
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.
Instead of an array of fixed-size 2, consider using a locally defined record, which means you get nice helpful names for the two components, instead of just [0]
and [1]
For example, you could insert the following immediately before this line:
private record Pair(HtmlTree list, HtmlTree item) { }
and then update the use sites to match.
* @param hasFilterInput whether to add a filter text input | ||
* @return a content object | ||
*/ | ||
protected Content getSideBar(boolean hasFilterInput) { |
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.
(Just asking)
In other builders for "complex" content, like Table
and Navigation
, we have methods like toContent
or getContent
. Should we follow that naming convention? While the name getSidebar
is certainly more descriptive, is the returned object inherently a sidebar -- or is the fact that it is a sidebar just an artifact of how we currently choose to present the content?
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 is actually up to the CSS how the table of contents is rendered. I'm renaming the method to toContent()
.
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.
Overall, very nice: the change to introduce and use a new TableOfContents
class is definitely a worthwhile improvement.
-
Medium-size suggestion/question:
consider init-ingHtmlDocletWriter.tableOfContents
directly in the constructor forHtmlDocletWriter
and not in the constructors for the subtypes. -
Small suggestion:
consider using a locally defined record instead of a two-element array in the newTableOfContents
class.
I agree it would be nice, but not sure how to do it without proliferation of |
- Use record instead of array in ListBuilder - Rename TableOfContents.getSideBar to toContent
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.
Nice.
/integrate |
Going to push as commit 81df265.
Your commit was automatically rebased without conflicts. |
This is a rather big change to update the structural navigation in API documentation generated by JavaDoc. It adds a table of contents for the current page to module, package, and class documentation, and replaces the old sub-navigation bar with a breadcrumb-style links in those pages. The table of contents is displayed as sidebar on wide/desktop displays and integrated into the collapsible menu for narrow/mobile displays. The generated docs can be browsed here:
https://cr.openjdk.org/~hannesw/8320458/api.00/
This change includes improvements in
stylesheet.css
andscript.js
that are not strictly related to the navigation changes, but happened as a consequence of the necessary restructuring. All these are described together with the more on-topic changes in the list below.HtmlDocletWriter
has a newtocBuilder
field of new typeListBuilder
. If the field is notnull
it is used to build the table of contents as the page is built using either one of the newHtmlDocletWriter.addToTableOfContents
methods or theListBuilder
directly.HtmlDocletWriter.getSideBar
is used to generate the markup for the sidebar and it is added to the page via theBodyContents.setSideContent
method.<div>
container with CSS classnav-content
that uses a flex layout for its content. This also handles vertical positioning, so the old workaround for vertical of the language version label inDocs.gmk
is not necessary anymore.<aside>
element for the sidebar, but I learned that this was the wrong element as it is meant for content that is not strictly related to the main content of the page. The prevailing notion seems to be that a table of contents is a navigation element and therefore should use the<nav>
element, so I used that for the TOC sidebar. The same applies for the breadcrumbs sub-navigation, so I left the header<nav>
element wrapped around both top and sub-navigation.<ol>
instead of unordered<ul>
we use everywhere else, as that is what should be used when list order is important.SEARCH
link that also served as label for the search input field has moved to the top navigation bar. The search input field uses "Search" as placeholder and also got a "Search in documentation" aria-label attribute.--top-nav-height
,--sub-nav-height
and--nav-height
have been introduced which are also updated dynamically to avoid any flickering seen in the previous version.<p>
tag was present or not.Navigation
has been simplified a bit and now uses a singleaddOverviewLink
to generate a link to the overview page, the first module page, or the first package page, depending on configuration. a newaddPageElementLink
method creates a link to the current page element.TestNavigation
andTestModuleNavigation
to check the navigation links in previously untested configurations.search.js.template
such as implementation of the mobile menu and creating the heading anchor links was not search related and therefore belonged intoscript.js
. This requiredscript.js
localized properties to be injected intoscript.js
which is therefore renamed toscript.js.template
Unfortunately, git did not recognize this as renaming but rather as removal ofscript.js
and addition ofscript.js.template
, but the file is pretty much unchanged up to line 232 with the new code starting in line 233.x
button in the search input field and the new TOC filter field behave a bit differently than previously in that they are only visible if there is some text in the input field.""
in a fragment was so far interpreted as no fragment. However, the empty fragment is now used to navigate to the top of the page (this is standard behaviour and implemented in all browsers and different from clicking on a link without#
which results in reloading the page rather than scrolling to the top). This required the changes inDocLink
andHtmlLinkInfo
as well as theLinkChecker
test.TestSingletonLists
test.TestSubTitle
test was removed as module and package subtitles have been replaced by the breadcrumb links.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17062/head:pull/17062
$ git checkout pull/17062
Update a local copy of the PR:
$ git checkout pull/17062
$ git pull https://git.openjdk.org/jdk.git pull/17062/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 17062
View PR using the GUI difftool:
$ git pr show -t 17062
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17062.diff
Webrev
Link to Webrev Comment