Re: RFR: 8332109: Convert remaining tests using com.sun.tools.classfile to ClassFile API
On Sun, 12 May 2024 08:36:44 GMT, Chen Liang wrote: > Some tests are not migrated to the ClassFile API in previous migrations. > > - Some are simple oversights that didn't remove usages of > com.sun.tools.classfile; > - The CallerSensitive ones used an old utility, replaced by CF API-based new > code; > - many in javac are because the files are compiled with older source > compatibility. Those patches are converted to have the source code stored in > text blocks and compiled within tests using `ToolBox#writeJavaFiles` and > `CompilerUtils.compile`; > - As described in the JBS issue, there are a few other tests not covered; > one is in #19193 while the others are blocked by CreateSymbols migration or > bugs. > > Testing: all modified tests pass. Looks good to me, great job, thanks! - Marked as reviewed by asotona (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19206#pullrequestreview-2062430122
Re: RFR: 8332109: Convert remaining tests using com.sun.tools.classfile to ClassFile API
On Fri, 17 May 2024 06:03:00 GMT, Adam Sotona wrote:
>> Some tests are not migrated to the ClassFile API in previous migrations.
>>
>> - Some are simple oversights that didn't remove usages of
>> com.sun.tools.classfile;
>> - The CallerSensitive ones used an old utility, replaced by CF API-based
>> new code;
>> - many in javac are because the files are compiled with older source
>> compatibility. Those patches are converted to have the source code stored in
>> text blocks and compiled within tests using `ToolBox#writeJavaFiles` and
>> `CompilerUtils.compile`;
>> - As described in the JBS issue, there are a few other tests not covered;
>> one is in #19193 while the others are blocked by CreateSymbols migration or
>> bugs.
>>
>> Testing: all modified tests pass.
>
> test/jdk/jdk/internal/reflect/CallerSensitive/CheckCSMs.java line 151:
>
>> 149:
>> 150: boolean needsCsm = false;
>> 151: for (var element : code) {
>
> Scanning the instructions is a bit different approach than in the original
> test.
Same as the other method name check, it was manually extracted ReferenceFinder
logic, we should retire ReferenceFinder with the old class file library.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19206#discussion_r1604405594
Re: RFR: 8332109: Convert remaining tests using com.sun.tools.classfile to ClassFile API
On Fri, 17 May 2024 06:07:47 GMT, Adam Sotona wrote:
>> Some tests are not migrated to the ClassFile API in previous migrations.
>>
>> - Some are simple oversights that didn't remove usages of
>> com.sun.tools.classfile;
>> - The CallerSensitive ones used an old utility, replaced by CF API-based
>> new code;
>> - many in javac are because the files are compiled with older source
>> compatibility. Those patches are converted to have the source code stored in
>> text blocks and compiled within tests using `ToolBox#writeJavaFiles` and
>> `CompilerUtils.compile`;
>> - As described in the JBS issue, there are a few other tests not covered;
>> one is in #19193 while the others are blocked by CreateSymbols migration or
>> bugs.
>>
>> Testing: all modified tests pass.
>
> test/jdk/jdk/internal/reflect/CallerSensitive/CallerSensitiveFinder.java line
> 125:
>
>> 123: && invoke.method() instanceof MethodRefEntry ref
>> 124: && ref.owner().name().equalsString(className)
>> 125: && ref.name().equalsString(methodName)) {
>
> Is this additional test necessary, I don't see it in the original test.
This was part of the now-removed ReferenceFinder logic; it scans cp as a fast
path and then does instruction scan for all methods to find relevant methods to
pass to visitor.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19206#discussion_r1604402631
Re: RFR: 8332109: Convert remaining tests using com.sun.tools.classfile to ClassFile API
On Sun, 12 May 2024 08:36:44 GMT, Chen Liang wrote: > Some tests are not migrated to the ClassFile API in previous migrations. > > - Some are simple oversights that didn't remove usages of > com.sun.tools.classfile; > - The CallerSensitive ones used an old utility, replaced by CF API-based new > code; > - many in javac are because the files are compiled with older source > compatibility. Those patches are converted to have the source code stored in > text blocks and compiled within tests using `ToolBox#writeJavaFiles` and > `CompilerUtils.compile`; > - As described in the JBS issue, there are a few other tests not covered; > one is in #19193 while the others are blocked by CreateSymbols migration or > bugs. > > Testing: all modified tests pass. test/langtools/tools/javac/7166455/CheckACC_STRICTFlagOnclinitTest.java line 81: > 79: ToolBox toolBox = new ToolBox(); > 80: toolBox.writeJavaFiles(in, SOURCE); > 81: CompilerUtils.compile(in, out, "--release", "16"); Smart move 👍 - PR Review Comment: https://git.openjdk.org/jdk/pull/19206#discussion_r1604399988
Re: RFR: 8332109: Convert remaining tests using com.sun.tools.classfile to ClassFile API
On Sun, 12 May 2024 08:36:44 GMT, Chen Liang wrote:
> Some tests are not migrated to the ClassFile API in previous migrations.
>
> - Some are simple oversights that didn't remove usages of
> com.sun.tools.classfile;
> - The CallerSensitive ones used an old utility, replaced by CF API-based new
> code;
> - many in javac are because the files are compiled with older source
> compatibility. Those patches are converted to have the source code stored in
> text blocks and compiled within tests using `ToolBox#writeJavaFiles` and
> `CompilerUtils.compile`;
> - As described in the JBS issue, there are a few other tests not covered;
> one is in #19193 while the others are blocked by CreateSymbols migration or
> bugs.
>
> Testing: all modified tests pass.
test/jdk/jdk/internal/reflect/CallerSensitive/CallerSensitiveFinder.java line
125:
> 123: && invoke.method() instanceof MethodRefEntry ref
> 124: && ref.owner().name().equalsString(className)
> 125: && ref.name().equalsString(methodName)) {
Is this additional test necessary, I don't see it in the original test.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19206#discussion_r1604394473
Re: RFR: 8332109: Convert remaining tests using com.sun.tools.classfile to ClassFile API
On Sun, 12 May 2024 08:36:44 GMT, Chen Liang wrote:
> Some tests are not migrated to the ClassFile API in previous migrations.
>
> - Some are simple oversights that didn't remove usages of
> com.sun.tools.classfile;
> - The CallerSensitive ones used an old utility, replaced by CF API-based new
> code;
> - many in javac are because the files are compiled with older source
> compatibility. Those patches are converted to have the source code stored in
> text blocks and compiled within tests using `ToolBox#writeJavaFiles` and
> `CompilerUtils.compile`;
> - As described in the JBS issue, there are a few other tests not covered;
> one is in #19193 while the others are blocked by CreateSymbols migration or
> bugs.
>
> Testing: all modified tests pass.
test/jdk/jdk/internal/reflect/CallerSensitive/CheckCSMs.java line 151:
> 149:
> 150: boolean needsCsm = false;
> 151: for (var element : code) {
Scanning the instructions is a bit different approach than in the original test.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19206#discussion_r1604390936
Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]
On Thu, 16 May 2024 16:44:51 GMT, Hannes Wallnöfer wrote: >> Jonathan Gibbons has updated the pull request incrementally with one >> additional commit since the last revision: >> >> update tests for dangling doc comments, per review feedback > > src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java > line 1465: > >> 1463: private final HtmlRenderer renderer = HtmlRenderer.builder() >> 1464: .nodeRendererFactory(headingRendererFactory) >> 1465: .extensions(List.of(tablesExtn/*, headingIdsExtn*/)) > > Is there a reason to keep the commented argument? no, it should go; thanks for catching - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1603864642
Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v70]
> Please review a patch to add support for Markdown syntax in documentation > comments, as described in the associated JEP. > > Notable features: > > * support for `///` documentation comments in `JavaTokenizer` > * new module `jdk.internal.md` -- a private copy of the `commonmark-java` > library > * updates to `DocCommentParser` to treat `///` comments as Markdown > * updates to the standard doclet to render Markdown comments in HTML Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision: address review feedback - Changes: - all: https://git.openjdk.org/jdk/pull/16388/files - new: https://git.openjdk.org/jdk/pull/16388/files/bfa35bd4..a0df7a4b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=69 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=68-69 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16388.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16388/head:pull/16388 PR: https://git.openjdk.org/jdk/pull/16388
Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]
On Wed, 15 May 2024 21:04:36 GMT, Jonathan Gibbons wrote:
>> Please review a patch to add support for Markdown syntax in documentation
>> comments, as described in the associated JEP.
>>
>> Notable features:
>>
>> * support for `///` documentation comments in `JavaTokenizer`
>> * new module `jdk.internal.md` -- a private copy of the `commonmark-java`
>> library
>> * updates to `DocCommentParser` to treat `///` comments as Markdown
>> * updates to the standard doclet to render Markdown comments in HTML
>
> Jonathan Gibbons has updated the pull request incrementally with one
> additional commit since the last revision:
>
> update tests for dangling doc comments, per review feedback
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 276:
> 274: // The following {@code transform} methods invoke {@code
> transform} on
> 275: // any children that may contain Markdown. If the
> transformations on
> 276: // the children are all identify transformations (that is the
> result
Typo: identify -> identity
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java
line 1465:
> 1463: private final HtmlRenderer renderer = HtmlRenderer.builder()
> 1464: .nodeRendererFactory(headingRendererFactory)
> 1465: .extensions(List.of(tablesExtn/*, headingIdsExtn*/))
Is there a reason to keep the commented argument?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1603439954
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1603716675
Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]
On Thu, 16 May 2024 15:05:32 GMT, Jonathan Gibbons wrote: > There were some community comments objecting the use of `///` for markdown > documentation, and called for alternative syntaxes like `/*markdown */`. This was [addressed](https://openjdk.org/jeps/467#Using-///-for-Markdown-documentation-comments) in an update to JEP 467. - PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-2115749018
Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]
On Thu, 16 May 2024 15:03:26 GMT, Pavel Rappo wrote: > If you are concerned with it being a lint warning rather than a **doc**lint > warning, then it's a technicality: doclint sees less than lint sees, and > sadly not enough for that check. Thus, that check was put in `lint. To clarify that a bit ... There's a subtle difference between _documentation comments_ and _documentation comments as used with the standard doclet_. From the beginning, documentation comments have been those enclosed in `/**...*/` but there has never been a formal specification that the only content is that accepted by the standard doclet. Indeed, over the years, there have been various doclets, not all from Sun/Oracle, and not all accepting the content syntax used by the standard doclet. This practice continues with the introduction of `///` comments. So, there are actually two issues conflated in this work: 1. introducing the use of `///` for documentation comments 2. introducing the use of Markdown for some comments They go together because it would be silly/confusing to introduce `///` comments by themselves, and figuring out what to do with them until the use of Markdown was introduced. Arguably, the first bullet by itself could have been a Preview feature, but it probably falls below any appropriate threshold. You might also note that `Elements.getDocComment` and friends say nothing about the syntactic form of the content of any doc comment after the lexical wrappings have been removed. In particular, words like HTML and Markdown do not appear in the specs for these methods. This is intentional and means that a different doclet could interpret the content of the doc comment in any way that it chooses. Back to this thread, and `-Xlint` vs `-Xdoclint`: `doclint` is specifically about issues within the content of doc comments as defined in the `JavaDoc Documentation Comment Specification for the Standard Docket` and interpreted by the standard doclet. `dangling-doc-comments` is not specific to comments seen by the standard doclet. It applies to all doc comments, whatever the form of their content. As such, it is a `javac` lexical check, and belongs in `-Xlint`. - PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-2115724002
Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]
On Wed, 15 May 2024 21:04:36 GMT, Jonathan Gibbons wrote: >> Please review a patch to add support for Markdown syntax in documentation >> comments, as described in the associated JEP. >> >> Notable features: >> >> * support for `///` documentation comments in `JavaTokenizer` >> * new module `jdk.internal.md` -- a private copy of the `commonmark-java` >> library >> * updates to `DocCommentParser` to treat `///` comments as Markdown >> * updates to the standard doclet to render Markdown comments in HTML > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > update tests for dangling doc comments, per review feedback Thanks for these explanations! Makes sense, also great that we can have Javadoc features delivered easily because its loose coupling with Java language. - PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-2115515659
Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]
On Thu, 16 May 2024 15:03:26 GMT, Pavel Rappo wrote: > My rationale for a potential preview is that we changed > `-Xlint:dangling-doc-comments` as `///` is now dangling doc comment. Is this > considered a Java programming language change? There were some community > comments objecting the use of `///` for markdown documentation, and called > for alternative syntaxes like `/*markdown */`. There is no change to the Java Language Specification in this work. In fact, JLS does not mention documentation comments in any form -- either traditional or end-of-line comments. (The original version of JLS did mention them; all subsequent versions do not.) As a result, even though `javadoc` is used to _generate_ the Java SE specifications, `javadoc`, the standard doclet, and documentation comments are all JDK features, not Java SE features. Given the relative size of this feature, compared to other new `javadoc` work, I think it is fair to say that if there had been a way to _preview_ the work, we would have considered doing so. But "preview" and JEP 12 are specifically for Java SE: >A preview feature is a new feature of the Java language, Java Virtual Machine, >or Java SE API that is fully specified, fully implemented, and yet >impermanent. It is available in a JDK feature release to provoke developer >feedback based on real world use; this may lead to it becoming permanent in a >future Java SE Platform. - PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-2115502498
Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]
On Thu, 16 May 2024 14:53:17 GMT, Chen Liang wrote: > My rationale for a potential preview is that we changed > `-Xlint:dangling-doc-comments` as `///` is now dangling doc comment. Is this > considered a Java programming language change? There were some community > comments objecting the use of `///` for markdown documentation, and called > for alternative syntaxes like `/*markdown */`. It is certainly not a Java language change. The Java language and JLS have never bothered with the javadoc comments. If you are concerned with it being a lint warning rather than a **doc**lint warning, then it's a technicality: doclint sees less than lint sees, and sadly not enough for that check. Thus, that check was put in `lint. - PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-2115497564
Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]
On Wed, 15 May 2024 21:04:36 GMT, Jonathan Gibbons wrote: >> Please review a patch to add support for Markdown syntax in documentation >> comments, as described in the associated JEP. >> >> Notable features: >> >> * support for `///` documentation comments in `JavaTokenizer` >> * new module `jdk.internal.md` -- a private copy of the `commonmark-java` >> library >> * updates to `DocCommentParser` to treat `///` comments as Markdown >> * updates to the standard doclet to render Markdown comments in HTML > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > update tests for dangling doc comments, per review feedback My rationale for a potential preview is that we changed `-Xlint:dangling-doc-comments` as `///` is now dangling doc comment. Is this considered a Java programming language change? There were some community comments objecting the use of `///` for markdown documentation, and called for alternative syntaxes like `/*markdown */`. - PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-2115466695
Re: RFR: 8332239: Improve CSS for block tags
On Thu, 16 May 2024 10:56:26 GMT, Hannes Wallnöfer wrote: > Please review a change to improve the layout of definition lists used to > display block tags: > > - Add indentation to the `` elements used for block tag details > - Set the margin of lists within block tag details so they do not appear as > nested lists, except for lists with CSS classes `tag-list` or > `tag-list-long`, which are used for block tags containing lists, such as > `@see`. > > Before/after comparison (contains `java.base` and `java.compiler` modules): > https://cr.openjdk.org/~hannesw/8332239/api.00/index.html > https://cr.openjdk.org/~hannesw/8332239/api.01/index.html > > Comparison for block tag layout: > https://cr.openjdk.org/~hannesw/8332239/api.00/java.base/java/lang/Object.html#hashCode() > https://cr.openjdk.org/~hannesw/8332239/api.01/java.base/java/lang/Object.html#hashCode() > > Example of very long block tag details containing a list: > https://cr.openjdk.org/~hannesw/8332239/api.00/java.compiler/javax/lang/model/util/package-summary.html > https://cr.openjdk.org/~hannesw/8332239/api.01/java.compiler/javax/lang/model/util/package-summary.html > > Note that the indentation also applies to the definition lists at the top of > class/interface documentation as they use the same markup as block tags: > https://cr.openjdk.org/~hannesw/8332239/api.00/java.base/java/lang/Exception.html > https://cr.openjdk.org/~hannesw/8332239/api.01/java.base/java/lang/Exception.html > > This was not an intended change but might or might not be desirable. It > could be avoided with some additional CSS changes. Nice. It works well for short collections of block tags. It works less well for the really long ones,, but it is definitely better than before. Maybe we should still consider other ways of rendering very large block tags. But this is a good first step. - Marked as reviewed by jjg (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19264#pullrequestreview-2061063161
Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]
On Wed, 15 May 2024 21:04:36 GMT, Jonathan Gibbons wrote: >> Please review a patch to add support for Markdown syntax in documentation >> comments, as described in the associated JEP. >> >> Notable features: >> >> * support for `///` documentation comments in `JavaTokenizer` >> * new module `jdk.internal.md` -- a private copy of the `commonmark-java` >> library >> * updates to `DocCommentParser` to treat `///` comments as Markdown >> * updates to the standard doclet to render Markdown comments in HTML > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > update tests for dangling doc comments, per review feedback > > A meta question about the JEP: Why don't Javadoc features (like snippets > > and markdown comments) don't go through preview, while Compiler features > > mostly do? Is there any bar for previews? > > Jon could probably reply more substantially, but my understanding is that > [_JEP 12: Preview Features_](https://openjdk.org/jeps/12) is barely > applicable here (emphasis mine): > > > ## Summary > > A _preview feature_ is a new feature of the Java language, Java Virtual > > Machine, or **Java SE API** that is fully specified, fully implemented, and > > yet impermanent. It is available in a JDK feature release to provoke > > developer feedback based on real world use; this may lead to it becoming > > permanent in a future Java SE Platform. > > Technically, the only reason we could invoke JEP 12 here would be a tiny > change to `Elements`, which is Java SE. But let's be honest, that change is > not what _JEP 467: Markdown Documentation Comments_ is about. Additionally, JavaDoc supports Preview APIs, but does not support previews (meta-previews?) of its own features. We simply don't have a mechanism to say: "Hey, that thing you are looking at is a new feature of JavaDoc. Click _here_ to learn more." - PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-2115404225
Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]
On Thu, 16 May 2024 13:05:39 GMT, Chen Liang wrote: > A meta question about the JEP: Why don't Javadoc features (like snippets and > markdown comments) don't go through preview, while Compiler features mostly > do? Is there any bar for previews? Jon could probably reply more substantially, but my understanding is that [_JEP 12: Preview Features_](https://openjdk.org/jeps/12) is barely applicable here (emphasis mine): > ## Summary > > A *preview feature* is a new feature of the Java language, Java Virtual > Machine, or **Java SE API** that is fully specified, fully implemented, and > yet impermanent. It is available in a JDK feature release to provoke > developer feedback based on real world use; this may lead to it becoming > permanent in a future Java SE Platform. Technically, the only reason we could invoke JEP 12 here would be a tiny change to `Elements`, which is Java SE. But let's be honest, that change is not what _JEP 467: Markdown Documentation Comments_ is about. - PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-2115367104
Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]
On Wed, 15 May 2024 21:04:36 GMT, Jonathan Gibbons wrote: >> Please review a patch to add support for Markdown syntax in documentation >> comments, as described in the associated JEP. >> >> Notable features: >> >> * support for `///` documentation comments in `JavaTokenizer` >> * new module `jdk.internal.md` -- a private copy of the `commonmark-java` >> library >> * updates to `DocCommentParser` to treat `///` comments as Markdown >> * updates to the standard doclet to render Markdown comments in HTML > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > update tests for dangling doc comments, per review feedback A meta question about the JEP: Why don't Javadoc features (like snippets and markdown comments) don't go through preview, while Compiler features mostly do? Is there any bar for previews? - PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-2115199037
Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]
On Wed, 15 May 2024 21:04:36 GMT, Jonathan Gibbons wrote: >> Please review a patch to add support for Markdown syntax in documentation >> comments, as described in the associated JEP. >> >> Notable features: >> >> * support for `///` documentation comments in `JavaTokenizer` >> * new module `jdk.internal.md` -- a private copy of the `commonmark-java` >> library >> * updates to `DocCommentParser` to treat `///` comments as Markdown >> * updates to the standard doclet to render Markdown comments in HTML > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > update tests for dangling doc comments, per review feedback It's probably the biggest update JavaDoc has seen in ages. Tremendous, good work. Thanks, Jon. - Marked as reviewed by prappo (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16388#pullrequestreview-2060613760
Re: RFR: 8332239: Improve CSS for block tags
On Thu, 16 May 2024 11:21:14 GMT, Pavel Rappo wrote: >> Please review a change to improve the layout of definition lists used to >> display block tags: >> >> - Add indentation to the `` elements used for block tag details >> - Set the margin of lists within block tag details so they do not appear as >> nested lists, except for lists with CSS classes `tag-list` or >> `tag-list-long`, which are used for block tags containing lists, such as >> `@see`. >> >> Before/after comparison (contains `java.base` and `java.compiler` modules): >> https://cr.openjdk.org/~hannesw/8332239/api.00/index.html >> https://cr.openjdk.org/~hannesw/8332239/api.01/index.html >> >> Comparison for block tag layout: >> https://cr.openjdk.org/~hannesw/8332239/api.00/java.base/java/lang/Object.html#hashCode() >> https://cr.openjdk.org/~hannesw/8332239/api.01/java.base/java/lang/Object.html#hashCode() >> >> Example of very long block tag details containing a list: >> https://cr.openjdk.org/~hannesw/8332239/api.00/java.compiler/javax/lang/model/util/package-summary.html >> https://cr.openjdk.org/~hannesw/8332239/api.01/java.compiler/javax/lang/model/util/package-summary.html >> >> Note that the indentation also applies to the definition lists at the top of >> class/interface documentation as they use the same markup as block tags: >> https://cr.openjdk.org/~hannesw/8332239/api.00/java.base/java/lang/Exception.html >> https://cr.openjdk.org/~hannesw/8332239/api.01/java.base/java/lang/Exception.html >> >> This was not an intended change but might or might not be desirable. It >> could be avoided with some additional CSS changes. > > Looks good in a rendered form. However, that mix of CSS units that we have -- > %, px, em -- always looked suspicious to me. Thanks @pavelrappo. > However, that mix of CSS units that we have -- %, px, em -- always looked > suspicious to me. There are reasons for using one or the other. `em` is the font size of the local element, so it can be used to space things relative to the element's font size. `1em` is the default top and bottom margin for top-level lists, so the list will be laid out correctly independent of the list's font. `px` on the other hand is an absolute unit. We use it to set the default font size to `14px` as the default `16px` will look too big, and to set up the basic layout of the page. In this case, the indentation should be the same regardless of font size (which might vary for proportional and monospaced font, for example). - PR Comment: https://git.openjdk.org/jdk/pull/19264#issuecomment-2115027428
Re: RFR: 8332239: Improve CSS for block tags
On Thu, 16 May 2024 10:56:26 GMT, Hannes Wallnöfer wrote: > Please review a change to improve the layout of definition lists used to > display block tags: > > - Add indentation to the `` elements used for block tag details > - Set the margin of lists within block tag details so they do not appear as > nested lists, except for lists with CSS classes `tag-list` or > `tag-list-long`, which are used for block tags containing lists, such as > `@see`. > > Before/after comparison (contains `java.base` and `java.compiler` modules): > https://cr.openjdk.org/~hannesw/8332239/api.00/index.html > https://cr.openjdk.org/~hannesw/8332239/api.01/index.html > > Comparison for block tag layout: > https://cr.openjdk.org/~hannesw/8332239/api.00/java.base/java/lang/Object.html#hashCode() > https://cr.openjdk.org/~hannesw/8332239/api.01/java.base/java/lang/Object.html#hashCode() > > Example of very long block tag details containing a list: > https://cr.openjdk.org/~hannesw/8332239/api.00/java.compiler/javax/lang/model/util/package-summary.html > https://cr.openjdk.org/~hannesw/8332239/api.01/java.compiler/javax/lang/model/util/package-summary.html > > Note that the indentation also applies to the definition lists at the top of > class/interface documentation as they use the same markup as block tags: > https://cr.openjdk.org/~hannesw/8332239/api.00/java.base/java/lang/Exception.html > https://cr.openjdk.org/~hannesw/8332239/api.01/java.base/java/lang/Exception.html > > This was not an intended change but might or might not be desirable. It > could be avoided with some additional CSS changes. Marked as reviewed by liach (Author). - PR Review: https://git.openjdk.org/jdk/pull/19264#pullrequestreview-2060496263
Re: RFR: 8332239: Improve CSS for block tags
On Thu, 16 May 2024 10:56:26 GMT, Hannes Wallnöfer wrote: > Please review a change to improve the layout of definition lists used to > display block tags: > > - Add indentation to the `` elements used for block tag details > - Set the margin of lists within block tag details so they do not appear as > nested lists, except for lists with CSS classes `tag-list` or > `tag-list-long`, which are used for block tags containing lists, such as > `@see`. > > Before/after comparison (contains `java.base` and `java.compiler` modules): > https://cr.openjdk.org/~hannesw/8332239/api.00/index.html > https://cr.openjdk.org/~hannesw/8332239/api.01/index.html > > Comparison for block tag layout: > https://cr.openjdk.org/~hannesw/8332239/api.00/java.base/java/lang/Object.html#hashCode() > https://cr.openjdk.org/~hannesw/8332239/api.01/java.base/java/lang/Object.html#hashCode() > > Example of very long block tag details containing a list: > https://cr.openjdk.org/~hannesw/8332239/api.00/java.compiler/javax/lang/model/util/package-summary.html > https://cr.openjdk.org/~hannesw/8332239/api.01/java.compiler/javax/lang/model/util/package-summary.html > > Note that the indentation also applies to the definition lists at the top of > class/interface documentation as they use the same markup as block tags. This > was not an intended change, but is hard to avoid without deeper changes to > the markup or CSS: > https://cr.openjdk.org/~hannesw/8332239/api.00/java.base/java/lang/Exception.html > https://cr.openjdk.org/~hannesw/8332239/api.01/java.base/java/lang/Exception.html Looks good in a rendered form. However, that mix of CSS units that we have -- %, px, em -- always looked suspicious to me. - Marked as reviewed by prappo (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19264#pullrequestreview-2060440321
RFR: 8332239: Improve CSS for block tags
Please review a change to improve the layout of definition lists used to display block tags: - Add indentation to the `` elements used for block tag details - Set the margin of lists within block tag details so they do not appear as nested lists, except for lists with CSS classes `tag-list` or `tag-list-long`, which are used for block tags containing lists, such as `@see`. Before/after comparison (contains `java.base` and `java.compiler` modules): https://cr.openjdk.org/~hannesw/8332239/api.00/index.html https://cr.openjdk.org/~hannesw/8332239/api.01/index.html Comparison for block tag layout: https://cr.openjdk.org/~hannesw/8332239/api.00/java.base/java/lang/Object.html#hashCode() https://cr.openjdk.org/~hannesw/8332239/api.01/java.base/java/lang/Object.html#hashCode() Example of very long block tag details containing a list: https://cr.openjdk.org/~hannesw/8332239/api.00/java.compiler/javax/lang/model/util/package-summary.html https://cr.openjdk.org/~hannesw/8332239/api.01/java.compiler/javax/lang/model/util/package-summary.html Note that the indentation also applies to the definition lists at the top of class/interface documentation as they use the same markup as block tags. This was not an intended change, but is hard to avoid without deeper changes to the markup or CSS: https://cr.openjdk.org/~hannesw/8332239/api.00/java.base/java/lang/Exception.html https://cr.openjdk.org/~hannesw/8332239/api.01/java.base/java/lang/Exception.html - Commit messages: - JDK-8332239: Improve CSS for block tags Changes: https://git.openjdk.org/jdk/pull/19264/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19264&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8332239 Stats: 8 lines in 1 file changed: 5 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/19264.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19264/head:pull/19264 PR: https://git.openjdk.org/jdk/pull/19264
