RFR: 8253117: Replace HTML tables in javadoc summaries with CSS grid elements

Fri Sep 25 18:17:00 UTC 2020

On Fri, 18 Sep 2020 14:51:29 GMT, Hannes Wallnöfer <hannesw at openjdk.org> wrote:

> This changes the output of the `html.markup.Table` class to plain `div` elements, using CSS Grid Layout to display them
> in a tabular format.
> I decided against renaming the Table class and related identifiers even though it does no longer emit an HTML <table>
> element. I admit this results in a somewhat odd mismatch in a few places, but on the other hand the generated HTML
> still represents tabular data. Also, the changes are much easier to understand and review this way. I'm open to
> renaming things if we can find a better terminology.  I simplified the existing code in quite a few places:
>  - Removed the setters for table tab ids and the browser tab script. The ids are now derived from the main table id which
>    makes them unique even with multiple tables per page (provided the tables have different ids), and the browser script
>    will always work for the used ids.
>  
>  - Removed the complex tab selection scheme based on bitwise operations and replaced it with one CSS class per tab. The
>    elements making up a table row will have a CSS class for each tab they belong to. The CSS class names are derived from
>    the table id as well.
> 
>  - Reduced per usage style classes for summary tables, thereby simplifying the style sheet. Instead of having a CSS class
>    for each useage of a table (e.g. `member-summary`, `type-summary` etc) there is only one common CSS class for summary
>    tables as well as one specifying the number of columns to use, e.g. `two-column-summary`, `three-column-summary` etc.
> 
> The rendering and spacing of the tables should be the same as previously. There are a few exceptions:
> 
>  - The style sheet has additional media queries to switch the layout of tables when the width of the browser window
>    becomes very narrow. This happens at different thresholds for tables with two, three, or four columns. Note that these
>    theresholds are based on heuristics, it is what I have found to work well under most circumstances.
> 
>  - The new grid never grow larger than the width available in the browser. When a table cell becomes too narrow to contain
>    its content, the cell becomes scrollable. This happens very rarely and is not too disturbing IMO.
> 
>  - Spacing of columns is usually a bit different than previously. Grids offers very complex layout options, and the
>    setting I came to use partitions space depending on the width of cell contents.
> 
> Here are the API docs for java.base rendered with these changes:
> http://cr.openjdk.java.net/~hannesw/8253117/api.00/
> 
> Here are the API docs with these changes and additionally the patch for JDK-8248566 (mobile browser optimizations)
> applied: http://cr.openjdk.java.net/~hannesw/8253117/api.00.mobile/

I've read all the `src/` files and spot-checked some of the `test/` files.

Generally excellent;  various comments inline.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractMemberWriter.java line 333:

> 331:             Table useTable = new Table(HtmlStyle.summaryTable)
> 332:                     .setCaption(heading)
> 333:                     .setColumnStyles(HtmlStyle.colFirst, HtmlStyle.colSecond, HtmlStyle.colLast);

�� Yay, nice to see these go!

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AllClassesIndexWriter.java line 135:

> 133:                 .addTab(resources.annotationTypeSummary, utils::isAnnotationType);
> 134:         for (Character unicode : indexBuilder.keys()) {
> 135:             for (IndexItem indexItem : indexBuilder.getMemberList(unicode)) {

�� Also nice to see go

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ClassUseWriter.java line 147:

> 145:         methodSubWriter = new MethodWriterImpl(this);
> 146:         constrSubWriter = new ConstructorWriterImpl(this);
> 147:         constrSubWriter.setFoundNonPubConstructor(true);

is this related?  or was it a previous omission/oversight?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/HtmlStyle.java line 411:

> 409:
> 410:     /**
> 411:      * The class of a {@code div} element whose content should be rendered as a table

This comment applies to the file as a whole, and not to this specific position in the file. (I couldn't find a way to
attach a comment to the file or an unedited line.)

I'm surprised that I didn't see lots of deletions in this file.  You are removing references from the `Table`
constructors, so I was expecting those table-specific names to be deleted here as well: or am I missing something?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/Table.java line 233:

> 231:         return this;
> 232:     }
> 233:

(This comment applies to this entire method. The UI will not let me extend the selection any further)

This method is now effectively empty. Should it be deleted?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/Table.java line 433:

> 431:                 .put(HtmlAttr.ONKEYDOWN, "switchTab(event)")
> 432:                 .put(HtmlAttr.ID, tabId)
> 433:                 .put(HtmlAttr.ONCLICK, "show('" + id + "', '" + (defaultTab ? id : tabId)

minor style suggestion: place the ONCLICK code nearer the ONKEYDOWN  code, possibly by moving the ID code up in the
list.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/resources/script.js line 60:

> 58:             var isEvenRow = index % (columns * 2) < columns;
> 59:             elem.classList.remove(isEvenRow ? rowColor : altColor);
> 60:             elem.classList.add(isEvenRow ? altColor : rowColor);

I wasn't going to suggest this until I saw the use of these names. Can we fix these weird/bad/non-obvious names for
`rowColor` and `altColor`.  Suggest either names like `oddRowColor` `evenRowColor` or `whiteRowColor` `greyRowColor` etc

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/resources/script.js line 60:

> 58:             var isEvenRow = index % (columns * 2) < columns;
> 59:             elem.classList.remove(isEvenRow ? rowColor : altColor);
> 60:             elem.classList.add(isEvenRow ? altColor : rowColor);

I assume that `elem.classList` is a set, not a list.  If it was previously an odd/even row and is still the same, we
don't want to add an extra duplicate entry to the list.

In the DOM spec here
https://dom.spec.whatwg.org/#dom-element-classlist
I could only find non-normative narrative description that it is a set.

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

> 345:  */
> 346: .summary-table {
> 347:     width:100%;

See related comment in HtmlStyle about removing these names from that class.

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

> 414:     display: grid;
> 415:     grid-template-columns: minmax(10%, max-content) minmax(10%, max-content) minmax(10%, max-content) minmax(10%,
> auto); 416: }

I like the minmax values and mostly like the effect on the generated docs ... although it is still slightly weird (to
me) that the column widths can change when switching tabs in the same table.

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

> 438: .summary-table > div {
> 439:     text-align:left;
> 440:     padding: 8px 3px 3px 7px;

General comment: should we be using some font-related unit such as `em` `ex` instead of absolute units like `px` ?

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

> 457: }
> 458: .provides-summary .col-first, .provides-summary .col-last, .provides-summary .col-first,
> 459: .provides-summary .col-last {

Hmm, I thought the `*-summary` names were going away?

-------------

PR: https://git.openjdk.java.net/jdk/pull/253