RFR: JDK-8276964: Better indicate a snippet that could not be processed [v2]

[Hannes Wallnöfer]

On Mon, 29 Nov 2021 13:55:39 GMT, Hannes Wallnöfer <hannesw at openjdk.org> wrote:

>> The primary purpose of this change is to make it easier to spot invalid snippets in generated documentation. 
>> 
>> This adds a new `Content invalidTagOutput(String summary, Optional<String> detail)` method to the `TagletWriter` class that returns HTML to display the summary and optionally a detail message. The method is only used for snippet tags for now, but is generic and could be used for other invalid tags in the future. 
>> 
>> If the `detail` argument is empty or contains a blank string, a `<span>` element is returned containing the `summary` argument. If a detail argument is provided, a HTML5 `<details>` element is returned containing a `<summary>` element with the `summary` argument and a `<pre>` element containing the `details` argument. In both cases the returned element is styled with a thin border and a light red background.
>> 
>> In its current use the `detail` argument is always provided by the message of the `ParseException` or `BadSnippetException` that was thrown and caught. 
>> 
>> Example output is available here: http://cr.openjdk.java.net/~hannesw/8276964/api.01/snippet_errormessages/A.html
>> 
>> I added output checks to some but not all of the negative tests. In addition I slightly reformatted `TestSnippetTag.java` to add indentation to some previously unindeted text blocks.
>
> Hannes Wallnöfer has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Normalize newlines on detail message

Thanks for the review.

> 
>     1. Consider allowing the `detail` to be `Optional<Content>` so there is more flexibility in how the detail is presented.  You can provide and use convenience methods to wrap plain text or preformatted text into `Content`.

The reason I went with `String` instead of `Content` is that this is called from the `*.toolkit` package hierarchy and all the `Content` implementations are within the `*.html` package hierarchy. I know the separation between the two is somewhat obsolete as we only have one output format, but I still prefer not to infringe the rule. Besides, I would not like a Taglet or other toolkit class to pass preformatted HTML to this method, so I don't see the actual benefit of a `Content` argument.
 
>     2. While I generally prefer simplifying and removing stuff from `HtmlDocletWriter`, I wonder if this is a case where the method would be better there than `TagletWriter[Impl]`. The specific use case I have in mind is the presentation of `ErroneousTree`.  This is currently handled in `HtmlDocletWriter.commentTagsToContent`, line 1621, and the erroneous text is simply presented as regular `Text`. (line 1631).  At a push `HtmlDocletWriter` could create and use a `TagletWriter` but then the overtones of "invalid tag" are a bit "smelly".

That's an interesting observation. I was not aware of this code. Obviously most tags are simple enough that problems are spotted early on in the doc comment parser. Snippets are probably the one exception where most/many problems are only detected later on. I think we need both mechanisms, so I'd propose to keep the new method in `TagletWriter[Impl]` but add a method to `HtmlDocletWriter` that generates the actual HTML and is used by both code paths. Is this something we want to do for 18? I'm not sure what the output would look like for other tags.

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

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