JDK-8273034: Make javadoc navigation collapsible on small displays #5360

hns · 2021-09-03T12:15:45Z

Please review a change to add a collapsible "hamburger" navigation bar to javadoc pages for small screens. The collapsible navbar is activated when the browser width is below 920px, which is the point where the normal navigation bar starts to wrap for some javadoc pages.

The feature is implemented using CSS and JavaScript techniques employed by popular frameworks and has been successfully tested on several browsers and platforms, including various Android and iOS devices as well as IE and Edge browsers on Windows 10.

Sample documentation generated with this change is provided at the URL below. For testing, please review it on a mobile phone or tablet, or using the device emulation features provided with the developer tools of many desktop browsers.

http://cr.openjdk.java.net/~hannesw/8273034/api.00/index.html

One peculiarity of this change is that the sub-navigation links are rendered separately for the normal and the mobile navbar. The reason for this is that the links are structured differently (flat list vs nested lists) and reside in different containers.

On the CSS side, apart from the collapsible navbar I overhauled the styles for the search input, especially the small reset "X" button. The positioning of the reset button is now simpler and more robust by using absolute instead of relative position, and doesn't require moving the search input and label to the right anymore.

The collapsible menu uses the aria-controls, aria-expanded attributes as described in the following wiki page:

https://www.w3.org/WAI/GL/wiki/Using_the_WAI-ARIA_aria-expanded_state_to_mark_expandable_and_collapsible_regions

Furthermore, the aria-label attribute is used to provide a human-readable lable for the button.

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_aria-label_attribute

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8273034: Make javadoc navigation collapsible on small displays

Reviewers

Jonathan Gibbons (@jonathan-gibbons - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5360/head:pull/5360
$ git checkout pull/5360

Update a local copy of the PR:
$ git checkout pull/5360
$ git pull https://git.openjdk.java.net/jdk pull/5360/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5360

View PR using the GUI difftool:
$ git pr show -t 5360

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5360.diff

bridgekeeper · 2021-09-03T12:16:19Z

👋 Welcome back hannesw! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-09-03T12:18:29Z

@hns The following label will be automatically applied to this pull request:

javadoc

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2021-09-03T12:22:39Z

Webrevs

01: Full - Incremental (da47f090)
00: Full (0c027e79)

pavelrappo · 2021-09-03T13:08:06Z

I haven't looked at the code yet. I've only seen the sample documentation on my laptop and my initial reaction was: Oh boy, does it look good! Both primary and secondary navigation is at your fingertips (no pun intended).

A few remarks.

I don't expect this to be an issue on small devices and it's not a big deal, but might look better if fixed. When you try to resize the window in an expanded hamburger view, the view collapses. Compare this behavior with that of, for example, https://getbootstrap.com/.
A problem which was highlighted by this change rather than brought by it, is that the primary navigation items (i.e. OVERVIEW, MODULE, PACKAGE, CLASS, USE, etc.) that are disabled (i.e. cannot be clicked) are indistinguishable from those that are enabled. Sure, if you have a pointing device such as a trackpad or mouse, you can hover over those items to see if they change their appearance. However, this does not seem achievable with your finger touching a phone screen.

hns · 2021-09-06T09:13:35Z

Thanks for the feedback, Pavel! Both of the issues you spotted are very valid. I spent some time on both of them, but gave up for the time being since I didn't find a quick solution.

Regarding the menu collapsing when resizing the browser (only when changing browser width btw), avoiding this would have required either duplication of hard-coded values between the CSS and script files, or adding more complexity to the script than seemed reasonable to me. I haven't looked at how Bootstrap solves this particular problem, but I guess they are willing to go the extra mile since this is part of their core purpose. In the pursuit of simplicity and maintainability, I thought the closing behaviour was acceptable.

Regarding the same color being used for active and inactive links in the navigation bar, I tried to find a faded-out color for the inactive links, but it turned out that with the particular blue background any color but white is considered to break accessibility rules. We might want to file an separate issue for this.

jonathan-gibbons

Only minor comments/questions

jonathan-gibbons · 2021-09-22T17:06:24Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Navigation.java

-                    addListToNav(listContents, tree);
+                    };
+                    if (nested) {
+                        tree.add(HtmlTree.LI(HtmlTree.P(label))


Is the HtmlTree.P necessary/useful?

I think it is necessary, at least I haven't found an alternative solution.

The <li> element we are creating here contains the main label as well as a nested list of links. The <li> elements of the nested list are sized and padded via CSS. In order to apply similar size and padding to the main label it needs to be contained in an element with block display.

I realize the use of <p> instead of <div> may seem a bit strange as the label does not really represent a paragraph. I think I chose it because it had more useful default sizing rules. However, I could easily switch to using a <div> instead.

jonathan-gibbons · 2021-09-22T17:06:58Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Navigation.java

-                tree.add(li);
-                addListToNav(listContents, tree);
+                if (nested) {
+                    Content li = HtmlTree.LI(HtmlTree.P(contents.detailLabel));


Ditto: Is the HtmlTree.P necessary/useful?

jonathan-gibbons · 2021-09-22T17:09:29Z

...javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/search.js.template

@@ -291,6 +291,35 @@ function doSearch(request, response) {
    response(result);
 }
 $(function() {
+    var expanded = false;


#include standard/recurring discussion on testing JavaScript

jonathan-gibbons · 2021-09-22T17:12:36Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/resources/stylesheet.css

+    ul.sub-nav-list-small li p {
+        margin: 5px 0;
+    }
+    div#navbar-sub-list {


Note to us:

The use of standard id's in the stylesheet should be documented in the upcoming new spec document

jonathan-gibbons · 2021-09-22T17:15:11Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/resources/stylesheet.css

@@ -807,34 +807,113 @@ table.striped > tbody > tr > th {
    font-weight: normal;
 }
 /**
- * Tweak font sizes and paddings for small screens.
+ * Tweak style for small screens.


General comment, for discussion ...
There's a growing set of absolute values in the stylesheet. At what point should we generate the .css file from a template and a separate list of definitions?

jonathan-gibbons · 2021-09-22T17:17:36Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Navigation.java

+        HtmlTree ulNavSummaryRight = new HtmlTree(TagName.UL).setStyle(HtmlStyle.subNavListSmall);
+        addSummaryLinks(ulNavSummaryRight, true);
+        addDetailLinks(ulNavSummaryRight, true);
+        navDiv.add(ulNavSummaryRight);
        tree.add(navDiv);

        HtmlTree subDiv = new HtmlTree(TagName.DIV).setStyle(HtmlStyle.subNav);


Maybe we should have some factory methods for HtmlTree.DIV

openjdk · 2021-09-22T17:19:41Z

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

8273034: Make javadoc navigation collapsible on small displays

Reviewed-by: jjg

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 263 new commits pushed to the master branch:

57fe11c: 8274120: [JVMCI] CompileBroker should resolve parameter types for JVMCI compiles
81d4164: 8272759: (fc) java/nio/channels/FileChannel/Transfer2GPlus.java failed in timeout
da38ced: 8271602: Add Math.ceilDiv() family parallel to Math.floorDiv() family
d39aad9: 8273924: ArrayIndexOutOfBoundsException thrown in java.util.JapaneseImperialCalendar.add()
c9de806: 8274039: codestrings gtest fails when hsdis is present
33df388: 8274003: ProcessHandleImpl.Info toString has an if check which is always true
0a36163: 8272600: (test) Use native "sleep" in Basic.java
c6df3c9: 8274071: Clean up java.lang.ref comments and documentation
71788c6: 8271287: jdk/jshell/CommandCompletionTest.java fails with "lists don't have the same size expected"
ba7d550: 8270139: jshell InternalError crash for import of @repeatable followed by unresolved ref
... and 253 more: https://git.openjdk.java.net/jdk/compare/02822e1398d6015f0ed26edd440db8e0d50bf152...master

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 master branch, type /integrate in a new comment.

hns · 2021-09-24T08:49:30Z

/integrate

openjdk · 2021-09-24T08:52:06Z

Going to push as commit 1d44014.
Since your change was applied there have been 285 commits pushed to the master branch:

bb74ae8: 8274171: java/nio/file/Files/probeContentType/Basic.java failed on "Content type" mismatches
56b8b35: 8273261: Replace 'while' cycles with iterator with enhanced-for in java.base
0aa63fe: 8274216: ProblemList 2 serviceability/dcmd/gc tests with ZGC on linux-all and windows-all
5ffbe75: 8274195: Doc cleanup in java.nio.file
1fdc656: 8274175: (fc) java/nio/channels/FileChannel/Transfer2GPlus.java still failed in timeout
3b1b8fc: 8269850: Most JDK releases report macOS version 12 as 10.16 instead of 12.0
1b7f4b7: 8274169: HotSpot Style Guide has stale link to chromium style guide
653a612: 8270156: Add "randomness" and "stress" keys to JTreg tests which use StressGCM, StressLCM and/or StressIGVN
66ce09f: 8272703: StressSeed should be set via FLAG_SET_ERGO
c434528: 8273546: DecimalFormat documentation contains literal HTML character references
... and 275 more: https://git.openjdk.java.net/jdk/compare/02822e1398d6015f0ed26edd440db8e0d50bf152...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-09-24T08:52:28Z

@hns Pushed as commit 1d44014.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

hns added 6 commits September 1, 2021 16:37

JDK-8273034: Make javadoc navigation collapsible on small displays

4c68589

JDK-8273034: Clean up CSS for sub-navigation bar

7759c91

JDK-8273034: Better collapse

c6e052f

JDK-8273034: fix media queries

cf3a421

JDK-8273034: update tests

ec05fbc

JDK-8273034: make header scrollable if necessary

0c027e7

openjdk bot added the rfr Pull request is ready for review label Sep 3, 2021

openjdk bot added the javadoc javadoc-dev@openjdk.org label Sep 3, 2021

JDK-8273034: tweak button height

da47f09

jonathan-gibbons approved these changes Sep 22, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 22, 2021

openjdk bot closed this Sep 24, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 24, 2021

JDK-8273034: Make javadoc navigation collapsible on small displays #5360

JDK-8273034: Make javadoc navigation collapsible on small displays #5360

Uh oh!

Conversation

hns commented Sep 3, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Sep 3, 2021

Uh oh!

openjdk bot commented Sep 3, 2021

Uh oh!

mlbridge bot commented Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

pavelrappo commented Sep 3, 2021

Uh oh!

hns commented Sep 6, 2021

Uh oh!

jonathan-gibbons left a comment

Choose a reason for hiding this comment

Uh oh!

jonathan-gibbons Sep 22, 2021

Choose a reason for hiding this comment

Uh oh!

hns Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

jonathan-gibbons Sep 22, 2021

Choose a reason for hiding this comment

Uh oh!

jonathan-gibbons Sep 22, 2021

Choose a reason for hiding this comment

Uh oh!

jonathan-gibbons Sep 22, 2021

Choose a reason for hiding this comment

Uh oh!

jonathan-gibbons Sep 22, 2021

Choose a reason for hiding this comment

Uh oh!

jonathan-gibbons Sep 22, 2021

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hns commented Sep 24, 2021

Uh oh!

openjdk bot commented Sep 24, 2021

Uh oh!

openjdk bot commented Sep 24, 2021

Uh oh!

Uh oh!

hns commented Sep 3, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 3, 2021 •

edited

Loading

openjdk bot commented Sep 22, 2021 •

edited

Loading