On Tue, 2 Nov 2021 18:54:48 GMT, Hannes Wallnöfer <hann...@openjdk.org> wrote:
>> Please review a change to add a nested `<code>` element for snippet tags as
>> well as HTML attributes for the snippet's `id` and `lang` attributes. The
>> change is quite simple, I did however encounter some gotchas. Below is a
>> short list of notes to explain some aspects of the change.
>>
>> - Both `id` and `lang` are only meaningful if they have non-empty values,
>> so defining them with empty or missing values will cause the respective HTML
>> attributes to be not defined.
>> - I decided to put the logic for extracting the `id` and `lang` attributes
>> into the already quite long `SnippetTaglet.getInlineTagOutput` method as
>> this is where the required preparation and infrastructure are located. Both
>> values are then passed as additional arguments to
>> `TagletWriter.snippetTagOutput` which is not great. I think it's ok for now,
>> but maybe if we need to pass more values along that way we should reconsider
>> this design.
>> - I'm not sure whether we should assume `lang=java` for internal snippets.
>> This change currently does not, it only derives implicit `lang` attributes
>> for external snippets based on the extension of the snippet source file.
>> - In `SnippetTaglet.languageFromFilename()` I tried to cover what I
>> reckoned would be the most useful languages for javadoc users, including a
>> few major JVM languages. I'm sure I forgot some, but I hope I got at least
>> those file extensions right.
>> - We previously added a newline character to the snippet's `<pre>` element.
>> I think this was done in order to make sure the element is rendered even if
>> the snippet content is empty. This was fine with just the `<pre>` element,
>> but with a nested `<code>` element that newline is visible in the browser. I
>> therefore replaced the newline with `HtmlTree.EMPTY`.
>> - For the test, I was able to convert the existing
>> `testIdAndLangAttributes` test to cover explicit `id` and `lang` attributes.
>> I added a new test called `testExternalImplicitAttributes` to test derived
>> `lang` attributes.
>
> Hannes Wallnöfer has refreshed the contents of this pull request, and
> previous commits have been removed. The incremental views will show
> differences compared to the previous content of the PR. The pull request
> contains one new commit since the last revision:
>
> Minimize list of recognized file extensions
> Thanks for the pointers, I didn't know this although I realized there had to
> be special rules with leading newlines in `<pre>` content. The rule does not
> apply to the content of a `<code>` element inside a `<pre>`. We could keep
> the newline before the `<code>` element, but it is no longer needed so I
> think it can be removed.
Given the HTML representation of `@snippet` that this PR proposes, we should
not keep that leading newline. Here's why. If we push it into `<code>`, this
newline will become significant and as such will be visible in the browser:
<pre><code>
content line 1
content line 2
...</code></pre>
```
If we leave it as is, the HTML will no longer resemble snippet indentation
because the content will be misaligned: the first content line will be shifted
6 positions to the right (the length of the `<code>` string):
<pre>
<code>content line 1
content line 2
...</code></pre>
-------------
PR: https://git.openjdk.java.net/jdk/pull/6165
