RFR: JDK-8239804 : Cleanup/simplify HTML/CSS for general block tags

Thu Feb 27 14:29:09 UTC 2020

I haven't seen webrev.01, so I'll describe what I see in webrev.02.

Some of the affected tests have been made to check for <dl> elements, where they
previously didn't:

    "<dl class=\"notes\">"

For the record, it's not that those <dl> elements were not in the HTML in the
first place, it's just the tests are now checking that those <dl>s have
a particular form.

<span class="..."> with the classes like "paramLabel", "returnLabel", "seeLabel",
"simpleTagLabel", and "throwsLabel" have gone from the associated <dt>s.

I can see that not all of the affected tests have been updated with this issue
number in their @bug. I skimmed through all those tests and I can see that it
makes sense. The tests have been tidied up a bit too. Thanks!

I agree with Hannes, this whole change makes HTML output more economical and
cleaner. This new structure makes much more sense.

As for that CSS class name, "notes". I don't have a strong opinion. Hannes and
yourself know that area much better than I do. So I trust your judgment,
especially that you've mentioned you had something (related) in the works that
gives you a broader context for this particular choice of name.

Nits
====

1. Since we are in this area, could you please fix the preexisting bad grammar?
That is, all the occurrences of "overriden" in "TestExternalOverridenMethod"
and everywhere under the test/langtools/jdk/javadoc/doclet/testPrivateClasses
directory.

2. Please escape "@param" in the top-level comment of the ParamTaglet type
with {@code ...}.

3. There's a line in "TestOverrideMethods" whose indentation is a bit off.

Otherwise looks good.

-Pavel

> On 26 Feb 2020, at 21:41, Jonathan Gibbons <jonathan.gibbons at oracle.com> wrote:
> 
> Updated webrev, with one additional change.  The new CSS class name is changed from "blockTags" to "notes".  This was done automagically in a single command, with one manual fixup to move the position of the renamed member in the list, which is still alphabetically sorted (for now).
> 
> I'm suggesting this rename now, because upcoming work suggests there are more definition lists involved that need to be subject to cleanup, for which "blockTags" is not a good CSS class name. The proposed name "notes" is intended to cover the lists generated for block tags, the lists for supertype and subtype information for a class, and the supertype information for an overriding method.
> 
> New webrev:
> 
> Webrev: http://cr.openjdk.java.net/~jjg/8239804/webrev.02/
> 
> -- Jon
> 
> 
> On 02/25/2020 02:32 PM, Jonathan Gibbons wrote:
>> Hannes,
>> 
>> Thanks for all the feedback; comments inline.
>> 
>> New webrev:
>> 
>> Webrev: http://cr.openjdk.java.net/~jjg/8239804/webrev.01/
>> 
>> -- Jon
>> 
>> 
>> On 02/25/2020 07:05 AM, Hannes Wallnöfer wrote:
>>> Hi Jon,
>>> 
>>> I think this is a good change overall. Less HTML tags, less CSS classes while still providing the same result.
>>> 
>>> A few minor issues:
>>> 
>>> - The new imports in ParamTaglet.java and MethodWriterImpl.java are not used.
>> Fixed.
>>> 
>>> - In line 297 of MethodWriterImpl.java there’s a use of writer.contents that should use the local contents variable (or get rid of local var and just use writer.contents)
>> I'll keep the local variable, since it's used in a few places, and fix line 297.
>>> 
>>> - In TagletWriter#getParamHeader the javadoc @param tag needs to be renamed/updated as well.
>> Oops, yes. Fixed.
>>> 
>>> - Given its slightly broader usage I’m not fully convinced about the „blockTags“ name, but can’t think of anything better. I’m fine with keeping the name for now, maybe we'll find something better in the upcoming CSS cleanup.
>> I'm not wildly enthusiastic about the name either, and like you, I can't think of anything better right now.  But, the only anomaly is the use for overriding methods, and it's hard to come up with a meaningful term for both block tags and overriding info.   There's words like "info", "more info" which are very generic; "details" is too specific and already has a different meaning on the most of the same pages.
>> 
>> There's also the CSS name "block" being used, that means something else, but that is a better candidate to be renamed!
>> 
>>> 
>>> - I got very confused when I saw one of the CSS styles you removed in the generated docs in one of the module-overview.html pages. I even thought the generated docs were built with some other JDK, until I realised some of the taglets reside outside the src directory. Below is a patch that updates these taglet classes to conform to the new output. I think it should be included with this patch for consistency.
>> 
>> Yeah, I know about the other taglets, and was going to handle them separately, but now they've come up here, I'll fix them here as well.
>> 
>>> 
>>> diff -r 4f902f017def make/jdk/src/classes/build/tools/taglet/ModuleGraph.java
>>> --- a/make/jdk/src/classes/build/tools/taglet/ModuleGraph.java Tue Feb 25 09:46:12 2020 +0100
>>> +++ b/make/jdk/src/classes/build/tools/taglet/ModuleGraph.java Tue Feb 25 15:32:53 2020 +0100
>>> @@ -74,7 +74,7 @@
>>>                   + "</span>";
>>>           }
>>>           return "<dt>"
>>> -            + "<span class=\"simpleTagLabel\">Module Graph:</span>\n"
>>> +            + "Module Graph:\n"
>>>               + "</dt>"
>>>               + "<dd>"
>>>               + "<a class=moduleGraph href=\"" + imageFile + "\">"
>>> diff -r 4f902f017def make/jdk/src/classes/build/tools/taglet/ToolGuide.java
>>> --- a/make/jdk/src/classes/build/tools/taglet/ToolGuide.java Tue Feb 25 09:46:12 2020 +0100
>>> +++ b/make/jdk/src/classes/build/tools/taglet/ToolGuide.java Tue Feb 25 15:32:53 2020 +0100
>>> @@ -95,7 +95,7 @@
>>>               return "";
>>>             StringBuilder sb = new StringBuilder();
>>> -        sb.append("<dt class=\"simpleTagLabel\">Tool Guides:</dt>\n")
>>> +        sb.append("<dt>Tool Guides:</dt>\n")
>>>                   .append("<dd>");
>>>             boolean needComma = false;
>>> 
>>> 
>>> —
>>> Hannes
>>> 
>>> 
>>>> Am 22.02.2020 um 01:57 schrieb Jonathan Gibbons <jonathan.gibbons at oracle.com>:
>>>> 
>>>> Please review a change to cleanup/simplify the HTML and CSS used to present block tags, like @see, @since, etc.
>>>> 
>>>> Currently, the information from these tags is presented in a definition list (<dl>). The labels, in the <dt> nodes, are wrapped in <span> using an inconsistent set of CSS class names. Sometimes the names are shared, sometimes they are unique.
>>>> 
>>>> With this change, a new CSS class "blockTags" is put on the enclosing <dl> node, and the <span> nodes wrapping the individual labels removed. This does mean that it is no longer possible to style some of the labels individually, but the use of CSS class names that were sometimes shared, sometime unique, makes it unlikely that anyone tried to do this. If (and only if) there is a demand to style tags individually, that should be handled as a new Enhancement request, that might involve putting tag-specific CSS class names on the <dt> and <dd> nodes for each tag (i.e. without an additional <span>).
>>>> 
>>>> There is one anomaly. The definition list for the block tags for a method is also used to present information when the method overrides or implements a method from a supertype. That makes the new CSS class name of "blockTags" slightly less than ideal. Suggestions for a better name are welcome.
>>>> 
>>>> The code to generate the overall list of tags for any element is not as centralized as might be desired. This changeset does not attempt to fix that.
>>>> 
>>>> There should be no visible change to docs generated using the default stylesheet when the docs are viewed in a browser.
>>>> 
>>>> The src/ code changes are relatively simple, and just consist of adjusting the HTML and CSS that is generated. In some cases, there is some additional code cleanup. About 30 tests are affected, although the changes are generally simple and localized. The bug number has been added to the @bug list for a subset of the tests; there are no new additional tests.
>>>> 
>>>> ---
>>>> 
>>>> A separate exercise would be to identify other <dl> nodes generated by the doclet, and to add a CSS class to those nodes to identify the kind of list.
>>>> 
>>>> -- Jon
>>>> 
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8239804
>>>> Webrev: http://cr.openjdk.java.net/~jjg/8239804/webrev.00/
>>>> 
>> 
> 