JDK-8308659: Use CSS scroll-margin instead of flexbox layout in API documentation #15969
Conversation
|
👋 Welcome back hannesw! A progress list of the required criteria for merging this PR into |
| buildConstructorComments(constructorContent); | ||
| buildTagInfo(constructorContent); | ||
|
|
||
| Content scrollBox = HtmlTree.DIV(HtmlStyle.horizontalScroll); |
There was a problem hiding this comment.
Stylistically, it's a shame to change the name of the variable in this and similar situations when all you are changing is its initial value. In the lines that follow, its only important that it is some kind of container, not specifically a scrollBox. This is the variable name equivalent of whether to write
Map m = new HashMap();
or
HashMap m = new HashMap();
| // Workaround for scroll position not being included in browser history (8249133) | ||
| document.addEventListener("DOMContentLoaded", function(e) { | ||
| var contentDiv = document.querySelector("div.flex-content"); | ||
| window.addEventListener("popstate", function(e) { | ||
| if (e.state !== null) { | ||
| contentDiv.scrollTop = e.state; | ||
| } | ||
| }); | ||
| window.addEventListener("hashchange", function(e) { | ||
| history.replaceState(contentDiv.scrollTop, document.title); | ||
| }); | ||
| var timeoutId; | ||
| contentDiv.addEventListener("scroll", function(e) { | ||
| if (timeoutId) { | ||
| clearTimeout(timeoutId); | ||
| } | ||
| timeoutId = setTimeout(function() { | ||
| history.replaceState(contentDiv.scrollTop, document.title); | ||
| }, 100); | ||
| }); | ||
| if (!location.hash) { | ||
| history.replaceState(contentDiv.scrollTop, document.title); | ||
| } | ||
| }); |
There was a problem hiding this comment.
always nice to see workarounds removed!
Webrevs
|
| /** | ||
| * The class of a {@code div} element that allows its horizontal overflow to be scrolled. | ||
| */ | ||
| horizontalScroll, |
There was a problem hiding this comment.
quibble: scroll? or scrollable?
| @@ -105,13 +105,13 @@ protected void buildMethodDoc(Content detailsList) { | |||
| for (Element method : methods) { | |||
| currentMethod = (ExecutableElement)method; | |||
There was a problem hiding this comment.
As a separate RFE, it would be nice if getVisibleMembers took an optional Class<T> argument that is used to cast the result of the method to List<T extends Element> instead of just Element. This would enable to eliminate downstream casts, like here on line 106, to get the subtype.
See a similar technique in Utils for accessing sublists of a list of DocTree nodes,
| Content div = HtmlTree.DIV(HtmlStyle.horizontalScroll); | ||
| buildSignature(div); | ||
| buildDeprecationInfo(div); | ||
| buildPreviewInfo(div); | ||
| buildMethodComments(div); | ||
| buildTagInfo(div); | ||
| methodContent.add(div); |
There was a problem hiding this comment.
Purely as a matter of style, I like the style of building "bottom up", so first build all those items into a ContentBuilder, then make a div with HtmlTree.DIV(HtmlStyle.horizontalScroll, contentBuilder) then add the div into the methodContent, as on line 114.
|
@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 186 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 |
- Move scroll-margin update from search.js to script.js - Use DOMContentLoaded event to update scroll-margin - Avoid use of :has() CSS pseudo-class which is unsupported on Firefox
|
I have updated this PR with a few additional fixes:
Generated documentation can be viewed here: https://cr.openjdk.org/~hannesw/8308659/api.03/ |
|
/integrate |
|
Going to push as commit eb7d972.
Your commit was automatically rebased without conflicts. |
A few years ago we switched to Flexbox Layout to implement the sticky header in the API docs generated by
javadoc. This solved the problem of anchor link targets not being positioned correctly, but it also introduced a few new ones:The reason for most of these problems is that the layout paradigm used by Flexbox is very different from traditional layout of HTML pages. The
scroll-margin-*CSS properties introduced by the CSS Scroll Snap Module provide a simpler and less intrusive way to implement link target offsets in combination with sticky elements implemented usingposition: sticky. However, although it is implemented in all supported browsers, it comes with its own challanges and quirks, which are explained below.The first problem to overcome was that
position: stickyis more fragile on mobile browsers than Flexbox. If some part of the page content overflows the allotted horizontal space, the whole page can be zoomed out to view the whole content. This causes the header to become unsticky and the link target offsets to become erroneous. It was therefore necessary to make sure page content never overflows its allotted horizontal space, either by adding line break opportunities, or by making all or part of the page horizontally scrollable when its content overflows. Line break opportunities are difficult to add especially in preformatted code, so I opted for making the parts of the page most likely containing long lines of code scrollable.The next problem was that enabling horizontal scrolling on an element or one of its containing elements breaks the
scroll-margin-topproperty in Chrome due to a browser quirk (both desktop and mobile versions). This means that the scrolling must occur in a child element rather than the sections or other elements serving as link targets.When enabling horizontal scrolling on the contents of sections containing user-provided content, another problem is that it disables Margin Collapse which regulates margins between adjacent and contained boxes. Since these sections can contain a mix of HTML containers, it is almost impossible to preserve the margins between elements while making them scrollable.
The solution to these problems was to add a new
<div class="horizontal-scroll">to the sections containing user-provided content. Adding a single scrollable element per section allowed the margins to be controlled in a relatively safe way, although some CSS is introduced to limit the margin of certain trailing block elements in certain sections.The
scroll-margin-topproperty is set to the normal height of the sticky header instylesheet.css, with a line added tosearch.jsto set the property dynamically to the actual header height to accommodate for the pre-release notification banner. In a previous version of this branch, the pre-release banner was excluded from the sticky header, but it was made sticky again in the final version to make the output more comparable.The generated documentation should render the same on supported browsers pixel by pixel. Before/after snapshots of the JDK docs are available here:
Before: https://cr.openjdk.org/~hannesw/8308659/api.00/
After: https://cr.openjdk.org/~hannesw/8308659/api.01/
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15969/head:pull/15969$ git checkout pull/15969Update a local copy of the PR:
$ git checkout pull/15969$ git pull https://git.openjdk.org/jdk.git pull/15969/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 15969View PR using the GUI difftool:
$ git pr show -t 15969Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15969.diff
Webrev
Link to Webrev Comment