Re: RFR: 8322708: Global HTML attributes are not allowed [v4]
On Fri, 14 Jun 2024 19:01:56 GMT, Chen Liang wrote:
>> Nizar Benalla has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Simplify method
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/HtmlTag.java line
> 670:
>
>> 668: public AttrKind getAttrKind(Name attrName) {
>> 669: Attr attr = getAttr(attrName);
>> 670: if (attr==null){
>
> Suggestion:
>
> if (attr == null) {
Thanks, didn't run a formatter on this.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1640242180
Re: RFR: 8322708: Global HTML attributes are not allowed [v5]
> Can I please get a review for this change, that aims to add support for > Global HTML tags. > Here is the > [link](https://cr.openjdk.org/~nbenalla/javadocGlobalPR/pkg1/package-summary.html) > to the generated docs. > Thanks in advance. Nizar Benalla has updated the pull request incrementally with one additional commit since the last revision: whitespace - Changes: - all: https://git.openjdk.org/jdk/pull/19652/files - new: https://git.openjdk.org/jdk/pull/19652/files/0e792be4..cfde660d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19652&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19652&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19652.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19652/head:pull/19652 PR: https://git.openjdk.org/jdk/pull/19652
Re: RFR: 8322708: Global HTML attributes are not allowed [v4]
On Fri, 14 Jun 2024 19:00:41 GMT, Nizar Benalla wrote:
>> Can I please get a review for this change, that aims to add support for
>> Global HTML tags.
>> Here is the
>> [link](https://cr.openjdk.org/~nbenalla/javadocGlobalPR/pkg1/package-summary.html)
>> to the generated docs.
>> Thanks in advance.
>
> Nizar Benalla has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Simplify method
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/HtmlTag.java line
670:
> 668: public AttrKind getAttrKind(Name attrName) {
> 669: Attr attr = getAttr(attrName);
> 670: if (attr==null){
Suggestion:
if (attr == null) {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1640233115
Re: RFR: 8322708: Global HTML attributes are not allowed [v4]
> Can I please get a review for this change, that aims to add support for > Global HTML tags. > Here is the > [link](https://cr.openjdk.org/~nbenalla/javadocGlobalPR/pkg1/package-summary.html) > to the generated docs. > Thanks in advance. Nizar Benalla has updated the pull request incrementally with one additional commit since the last revision: Simplify method - Changes: - all: https://git.openjdk.org/jdk/pull/19652/files - new: https://git.openjdk.org/jdk/pull/19652/files/248f1f2a..0e792be4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19652&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19652&range=02-03 Stats: 7 lines in 1 file changed: 1 ins; 2 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/19652.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19652/head:pull/19652 PR: https://git.openjdk.org/jdk/pull/19652
RFR: 8334241: Adjust API docs side bar dimensions
Please review a change to slightly adjust the proportion between the side bar and the main content area in API documentation pages, making the side bar a bit slimmer and leaving more space for the main content area. The ratio between side bar and main content used to be 1 : 2.6, it is now 1 : 3. Updated API documentation[ can be viewed here](https://cr.openjdk.org/~hannesw/8334241/api.00/java.base/java/lang/Object.html). - Commit messages: - JDK-8334241: Adjust API docs side bar dimensions Changes: https://git.openjdk.org/jdk/pull/19726/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19726&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8334241 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19726.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19726/head:pull/19726 PR: https://git.openjdk.org/jdk/pull/19726
Re: RFR: 8322708: Global HTML attributes are not allowed [v3]
On Fri, 14 Jun 2024 12:15:45 GMT, Nizar Benalla wrote:
>> Can I please get a review for this change, that aims to add support for
>> Global HTML tags.
>> Here is the
>> [link](https://cr.openjdk.org/~nbenalla/javadocGlobalPR/pkg1/package-summary.html)
>> to the generated docs.
>> Thanks in advance.
>
> Nizar Benalla has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains ten additional
> commits since the last revision:
>
> - Null safety
> - Merge branch 'master' into html-attributes
> - remove trailing whitespace
> - -Add a boolean attribute to the enum type
>-Simply regex in `visitAttribute`
>-Simplified the Test
>-Added a negative test
> - no longer print summary
> - no longer print summary
> - Add small comment
> - Remove classpath exception
> - Allow global variables
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/HtmlTag.java line
672:
> 670: AttrKind kind = null;
> 671:
> 672: if (attr != null) {
Can the remainder of this method be simplified using `Map.getOrDefault`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1639952309
Re: RFR: 8325690: The scrollable element with non-interactive content is not tabbable [v38]
> The scrollable element `` with non-interactive content is not tabbable. > Grid columns in the javadoc stylesheet has overflow: auto, which is failing > Accessibility checks. > https://bugs.openjdk.org/browse/JDK-8325690 psoujany has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 42 additional commits since the last revision: - Merge branch 'openjdk:master' into remove-overflow - Add table role to summary tables - Add table role to summary tables - Add table role to summary tables - Add table role to summary tables - Add grid role to tables - Add grid role to tables - Merge branch 'remove-overflow' of github.com:psoujany/jdk into remove-overflow - Merge branch 'openjdk:master' into remove-overflow - Add grid role to tables - ... and 32 more: https://git.openjdk.org/jdk/compare/72559e1c...8748de2a - Changes: - all: https://git.openjdk.org/jdk/pull/17819/files - new: https://git.openjdk.org/jdk/pull/17819/files/a2b0bab7..8748de2a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17819&range=37 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17819&range=36-37 Stats: 1936 lines in 108 files changed: 1483 ins; 165 del; 288 mod Patch: https://git.openjdk.org/jdk/pull/17819.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17819/head:pull/17819 PR: https://git.openjdk.org/jdk/pull/17819
Re: RFR: 8322708: Global HTML attributes are not allowed [v3]
On Tue, 11 Jun 2024 13:03:47 GMT, Pavel Rappo wrote: >> Nizar Benalla has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains ten additional >> commits since the last revision: >> >> - Null safety >> - Merge branch 'master' into html-attributes >> - remove trailing whitespace >> - -Add a boolean attribute to the enum type >>-Simply regex in `visitAttribute` >>-Simplified the Test >>-Added a negative test >> - no longer print summary >> - no longer print summary >> - Add small comment >> - Remove classpath exception >> - Allow global variables > > test/langtools/jdk/javadoc/doclet/TestGlobalHtml/pkg1/C2.java line 104: > >> 102: * >> 103: * >> 104: * > > Since you use `ol`, `ul`, and `li`elements, you could add global attributes > to them too. Otherwise, you only have such attributes in `p` elements. The more you go down this path, the more this seems like a combo test for generated code ;-) - PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1636662684
Re: RFR: 8322708: Global HTML attributes are not allowed [v3]
On Tue, 11 Jun 2024 22:12:42 GMT, Jonathan Gibbons wrote: >> Nizar Benalla has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains ten additional >> commits since the last revision: >> >> - Null safety >> - Merge branch 'master' into html-attributes >> - remove trailing whitespace >> - -Add a boolean attribute to the enum type >>-Simply regex in `visitAttribute` >>-Simplified the Test >>-Added a negative test >> - no longer print summary >> - no longer print summary >> - Add small comment >> - Remove classpath exception >> - Allow global variables > > test/langtools/jdk/javadoc/doclet/TestGlobalHtml/pkg1/C1.java line 1: > >> 1: /* > > This example is way too big and/or insufficiently focused on global > attributes. As an example of this, compare the size of the input programs to > the size (number) of strings given to `checkOutput` in. the previous file. > > The challenge of writing good tests is to find the simplest example that > causes the newly-written code to be executed. > > Also, while not wrong, it's also "old-school" to write separate classes in > separate files to run through `javadoc`. You can often come up with small > enough examples to fit in text blocks in the main test program. > > In this case, I would suggest providing a class with a comment containing a > whole bunch of simple tags each containing one or more global attributes. It > doesn't have to make "sense" as a comment -- it just has to exercise code > paths in the new code. Also, consider what it might mean to write "negative" tests. That might be a bit tricky, given these are global attributes, accepted on all HTML tags that can appear in a doc comment, so at least make sure there is a test (there may already be a test) for something that is not a global attribute, just to test the error reporting. - PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1635550821
Re: RFR: 8322708: Global HTML attributes are not allowed [v3]
On Fri, 14 Jun 2024 12:12:51 GMT, Nizar Benalla wrote: >> Can I please get a review for this change, that aims to add support for >> Global HTML tags. >> Here is the >> [link](https://cr.openjdk.org/~nbenalla/javadocGlobalPR/pkg1/package-summary.html) >> to the generated docs. >> Thanks in advance. > > Nizar Benalla has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains ten additional > commits since the last revision: > > - Null safety > - Merge branch 'master' into html-attributes > - remove trailing whitespace > - -Add a boolean attribute to the enum type >-Simply regex in `visitAttribute` >-Simplified the Test >-Added a negative test > - no longer print summary > - no longer print summary > - Add small comment > - Remove classpath exception > - Allow global variables test/langtools/jdk/javadoc/doclet/TestGlobalHtml/pkg1/C1.java line 1: > 1: /* This example is way too big and/or insufficiently focused on global attributes. As an example of this, compare the size of the input programs to the size (number) of strings given to `checkOutput` in. the previous file. The challenge of writing good tests is to find the simplest example that causes the newly-written code to be executed. Also, while not wrong, it's also "old-school" to write separate classes in separate files to run through `javadoc`. You can often come up with small enough examples to fit in text blocks in the main test program. In this case, I would suggest providing a class with a comment containing a whole bunch of simple tags each containing one or more global attributes. It doesn't have to make "sense" as a comment -- it just has to exercise code paths in the new code. - PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1635537304
Re: RFR: 8322708: Global HTML attributes are not allowed [v3]
On Wed, 12 Jun 2024 09:03:38 GMT, Hannes Wallnöfer wrote: >> Wouldn't the easiest solution be to add a boolean `global`/`isGlobal` field >> and getter to `Attr`? >> >> That would give use some more opportunities to simplify the code: We could >> get rid of the `GLOBAL_ATTRS` map here *and* avoid putting global attributes >> in the the `attr` map of each `HtmlTag` instance by simply adding a shortcut >> to return `AttrKind.OK` in `getAttrKind(Name)` if the given attribute is >> global. > > BTW, this should also be done by global attributes that are already > respected, such as `id`, `class`, and the `aria-*` accessibility attributes. I agree with Hannes on both comments: - `class`, `id`, `slot`, and `aria-*` are effectively global attributes and should be modelled as such - global attributes could probably be constructed with a boolean argument `true` - PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1636148171
Re: RFR: 8322708: Global HTML attributes are not allowed [v3]
On Wed, 12 Jun 2024 09:02:20 GMT, Hannes Wallnöfer wrote: >> I suggest one of >> >> 1. Add a `boolean global;` member to the enum, and provide a second >> constructor that allows that boolean member to be set true. For example, >> >> WIDTH, >> STYLE(true), >> >> >> 2. Make the `Set` be static and init it in a static initializer. >> See the `javac` `Source` and `Target` classes for examples of this technique. > > Wouldn't the easiest solution be to add a boolean `global`/`isGlobal` field > and getter to `Attr`? > > That would give use some more opportunities to simplify the code: We could > get rid of the `GLOBAL_ATTRS` map here *and* avoid putting global attributes > in the the `attr` map of each `HtmlTag` instance by simply adding a shortcut > to return `AttrKind.OK` in `getAttrKind(Name)` if the given attribute is > global. BTW, this should also be done by global attributes that are already respected, such as `id`, `class`, and the `aria-*` accessibility attributes. - PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1636095371
Re: RFR: 8322708: Global HTML attributes are not allowed [v3]
On Tue, 11 Jun 2024 21:58:46 GMT, Jonathan Gibbons wrote:
>>> If we are ordering the entries, we can use comparable to check that an attr
>>> is greater than the start of the global attr, something like
>>>
>>> ```java
>>> private static boolean isGlobalAttr(Attr value) {
>>> return value.compareTo(Attr.ACCESSKEY) >= 0;
>>> }
>>> ```
>>
>> That would also work, yes. I still avoid relying on the order of enum
>> constants; but it's me.
>
> I suggest one of
>
> 1. Add a `boolean global;` member to the enum, and provide a second
> constructor that allows that boolean member to be set true. For example,
>
> WIDTH,
> STYLE(true),
>
>
> 2. Make the `Set` be static and init it in a static initializer.
> See the `javac` `Source` and `Target` classes for examples of this technique.
Wouldn't the easiest solution be to add a boolean `global`/`isGlobal` field and
getter to `Attr`?
That would give use some more opportunities to simplify the code: We could get
rid of the `GLOBAL_ATTRS` map here *and* avoid putting global attributes in the
the `attr` map of each `HtmlTag` instance by simply adding a shortcut to return
`AttrKind.OK` in `getAttrKind(Name)` if the given attribute is global.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1636093592
Re: RFR: 8322708: Global HTML attributes are not allowed [v3]
On Tue, 11 Jun 2024 13:41:58 GMT, Pavel Rappo wrote:
>> If we are ordering the entries, we can use comparable to check that an attr
>> is greater than the start of the global attr, something like
>>
>> private static boolean isGlobalAttr(Attr value) {
>> return value.compareTo(Attr.ACCESSKEY) >= 0;
>> }
>
>> If we are ordering the entries, we can use comparable to check that an attr
>> is greater than the start of the global attr, something like
>>
>> ```java
>> private static boolean isGlobalAttr(Attr value) {
>> return value.compareTo(Attr.ACCESSKEY) >= 0;
>> }
>> ```
>
> That would also work, yes. I still avoid relying on the order of enum
> constants; but it's me.
I suggest one of
1. Add a `boolean global;` member to the enum, and provide a second constructor
that allows that boolean member to be set true. For example,
WIDTH,
STYLE(true),
2. Make the `Set` be static and init it in a static initializer.
See the `javac` `Source` and `Target` classes for examples of this technique.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1635524009
Re: RFR: 8322708: Global HTML attributes are not allowed [v3]
On Tue, 11 Jun 2024 12:40:23 GMT, Nizar Benalla wrote: >> Either what Chen has suggested or re-sort the complete list alphabetically >> as it was prior to this change, well, almost. > >> Either what Chen has suggested or re-sort the complete list alphabetically >> as it was prior to this change. > > I will go with Chen's suggestions, will fix in a bit. > > I might move STYLE down as it's also a global tag I'm (mostly) with Pavel about `enum` ordering -- it's OK when there is an obvious natural order, but risky when it's not. It's generally better to find other ways to categorize members than relying on the order. - PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1635605329
Re: RFR: 8322708: Global HTML attributes are not allowed [v3]
On Wed, 12 Jun 2024 09:26:25 GMT, Pavel Rappo wrote:
>> I admit to being "lazy" when I wrote `startsWith("on")`. In general, I don't
>> think folk should be writing many if any event handlers in doc comments, and
>> it was easier to give forward compatibility with a weak test for `"on"`.
>> Likewise, there should not be many if any uses of `data-*` attributes, and
>> it is easy enough to give a simple regex. It's more important to not reject
>> valid uses of event attributes and custom data attributes than it is to
>> detect all invalid cases. The right place to detect all invalid cases is
>> downstream in recognized validation tools, like `tidy` which catch all
>> occurrences of bad stuff, wherever they originate (doc comment, command-line
>> options, the `javadoc` tool, post-processors, etc). DocLint is primarily an
>> "early warning system" for common errors.
>
> I agree with you, Jon. So, we can simply use `startsWith("data-")`, which
> would be good enough and most permissive (i.e. no false negatives).
Sure, I will go with that.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1636140103
Re: RFR: 8322708: Global HTML attributes are not allowed [v3]
On Tue, 11 Jun 2024 23:49:15 GMT, Jonathan Gibbons wrote:
>> As @jonathan-gibbons likes to point out, javadoc is not an HTML validation
>> tool. So I think, it's okay to leave the code simple. Maybe this would be
>> even simpler?
>>
>> data-[a-z][-a-z0-9]*
>
> I admit to being "lazy" when I wrote `startsWith("on")`. In general, I don't
> think folk should be writing many if any event handlers in doc comments, and
> it was easier to give forward compatibility with a weak test for `"on"`.
> Likewise, there should not be many if any uses of `data-*` attributes, and it
> is easy enough to give a simple regex. It's more important to not reject
> valid uses of event attributes and custom data attributes than it is to
> detect all invalid cases. The right place to detect all invalid cases is
> downstream in recognized validation tools, like `tidy` which catch all
> occurrences of bad stuff, wherever they originate (doc comment, command-line
> options, the `javadoc` tool, post-processors, etc). DocLint is primarily an
> "early warning system" for common errors.
I agree with you, Jon. So, we can simply use `startsWith("data-")`, which would
be good enough and most permissive (i.e. no false negatives).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1636126138
Re: RFR: 8322708: Global HTML attributes are not allowed [v3]
On Tue, 11 Jun 2024 15:09:03 GMT, Pavel Rappo wrote:
>> I think like being slightly restrictive and safe.
>
> As @jonathan-gibbons likes to point out, javadoc is not an HTML validation
> tool. So I think, it's okay to leave the code simple. Maybe this would be
> even simpler?
>
> data-[a-z][-a-z0-9]*
I admit to being "lazy" when I wrote `startsWith("on")`. In general, I don't
think folk should be writing many if any event handlers in doc comments, and it
was easier to give forward compatibility with a weak test for `"on"`. Likewise,
there should not be many if any uses of `data-*` attributes, and it is easy
enough to give a simple regex. It's more important to not reject valid uses of
event attributes and custom data attributes than it is to detect all invalid
cases. The right place to detect all invalid cases is downstream in recognized
validation tools, like `tidy` which catch all occurrences of bad stuff,
wherever they originate (doc comment, command-line options, the `javadoc` tool,
post-processors, etc). DocLint is primarily an "early warning system" for
common errors.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1635600065
Re: RFR: 8322708: Global HTML attributes are not allowed [v3]
On Tue, 11 Jun 2024 14:42:48 GMT, Nizar Benalla wrote:
>> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/Checker.java line
>> 695:
>>
>>> 693: // custom "data-*" attributes are also accepted
>>> 694: var attrName=name.toString();
>>> 695: if (!attrName.startsWith("on") &&
>>> !attrName.matches("data-[a-z]+(-[a-z]+)*")){
>>
>> I understand that `startsWith("on")` precedes this PR. However, it's more
>> permissive than it should be: in reality, there are only 70 global
>> attributes that start with "on". On the other hand,
>> `attrName.matches("data-[a-z]+(-[a-z]+)*")` is less permissive. For example,
>> if I read the XML spec correctly, such an attribute can contain `_` or start
>> with `data-.`.
>>
>> But maybe those checks are good enough?
>
> I think like being slightly restrictive and safe.
As @jonathan-gibbons likes to point out, javadoc is not an HTML validation
tool. So I think, it's okay to leave the code simple. Maybe this would be even
simpler?
data-[a-z][-a-z0-9]*
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1635059794
Re: RFR: 8322708: Global HTML attributes are not allowed [v2]
On Tue, 11 Jun 2024 13:24:48 GMT, Nizar Benalla wrote: >> Can I please get a review for this change, that aims to add support for >> Global HTML tags. >> Here is the >> [link](https://cr.openjdk.org/~nbenalla/javadocGlobalPR/pkg1/package-summary.html) >> to the generated docs. >> Thanks in advance. > > Nizar Benalla has updated the pull request incrementally with one additional > commit since the last revision: > > Remove classpath exception I've ran tests locally for doclint and they all passed. Waiting on new results from tier 1-3 but there shouldn't be anything else that breaks. - PR Comment: https://git.openjdk.org/jdk/pull/19652#issuecomment-2167889419
Re: RFR: 8322708: Global HTML attributes are not allowed [v3]
On Tue, 11 Jun 2024 13:26:28 GMT, Pavel Rappo wrote:
>> Nizar Benalla has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains ten additional
>> commits since the last revision:
>>
>> - Null safety
>> - Merge branch 'master' into html-attributes
>> - remove trailing whitespace
>> - -Add a boolean attribute to the enum type
>>-Simply regex in `visitAttribute`
>>-Simplified the Test
>>-Added a negative test
>> - no longer print summary
>> - no longer print summary
>> - Add small comment
>> - Remove classpath exception
>> - Allow global variables
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/Checker.java line
> 695:
>
>> 693: // custom "data-*" attributes are also accepted
>> 694: var attrName=name.toString();
>> 695: if (!attrName.startsWith("on") &&
>> !attrName.matches("data-[a-z]+(-[a-z]+)*")){
>
> I understand that `startsWith("on")` precedes this PR. However, it's more
> permissive than it should be: in reality, there are only 70 global attributes
> that start with "on". On the other hand,
> `attrName.matches("data-[a-z]+(-[a-z]+)*")` is less permissive. For example,
> if I read the XML spec correctly, such an attribute can contain `_` or start
> with `data-.`.
>
> But maybe those checks are good enough?
I think like being slightly restrictive and safe.
> test/langtools/jdk/javadoc/doclet/TestGlobalHtml/TestGlobalHtml.java line 41:
>
>> 39: var tester = new TestGlobalHtml();
>> 40: tester.runTests();
>> 41: tester.printSummary();
>
> Why is this summary needed? Usually, we don't print it.
Fixed in
[cf4dab6](https://github.com/openjdk/jdk/pull/19652/commits/cf4dab6ceefb8625e9e7b281073cba49b894d13c).
I was using it locally.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1635017243
PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1634982192
Re: RFR: 8322708: Global HTML attributes are not allowed [v3]
On Fri, 14 Jun 2024 12:12:51 GMT, Nizar Benalla wrote:
>> Can I please get a review for this change, that aims to add support for
>> Global HTML tags.
>> Here is the
>> [link](https://cr.openjdk.org/~nbenalla/javadocGlobalPR/pkg1/package-summary.html)
>> to the generated docs.
>> Thanks in advance.
>
> Nizar Benalla has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains ten additional
> commits since the last revision:
>
> - Null safety
> - Merge branch 'master' into html-attributes
> - remove trailing whitespace
> - -Add a boolean attribute to the enum type
>-Simply regex in `visitAttribute`
>-Simplified the Test
>-Added a negative test
> - no longer print summary
> - no longer print summary
> - Add small comment
> - Remove classpath exception
> - Allow global variables
I've updated the PR based on everyone's suggestions, here are the main changes.
Currently running tier 1-3
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/Checker.java line
695:
> 693: // custom "data-*" attributes are also accepted
> 694: var attrName = name.toString();
> 695: if (!attrName.startsWith("on") &&
> !attrName.startsWith("data-")) {
I've removed the regex here.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/HtmlTag.java line
441:
> 439: ALINK,
> 440: ALT,
> 441: ARIA_ACTIVEDESCENDANT(true),
The enum `Attr` holds two values now, the second representing whether it's a
global attribute.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/HtmlTag.java line
670:
> 668: public AttrKind getAttrKind(Name attrName) {
> 669: Attr attr= getAttr(attrName);
> 670: AttrKind k = attr.isGlobal()? AttrKind.OK: attrs.get(attr); //
> null-safe
As hannes suggested, `getAttrKind` now returns `AttrKind.OK` if we are dealing
with a global attribute.
Edit: It's no longer null safe so I will slightly change it
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/HtmlTag.java line
673:
> 671:
> 672: if (attr != null) {
> 673: kind = attr.isGlobal() ? AttrKind.OK : attrs.get(attr); //
> get is null-safe
As hannes suggested, `getAttrKind` now returns `AttrKind.OK` if we are dealing
with a global attribute.
I've added a null check here because of `attr.isGlobal()`
test/langtools/jdk/javadoc/doclet/TestGlobalHtml/TestGlobalHtml.java line 70:
> 68: public class C {
> 69: /**
> 70: *
Added a negative test, as `` and `` tags aren't allowed .
test/langtools/jdk/javadoc/doclet/TestGlobalHtml/pkg1/C1.java line 23:
> 21: * questions.
> 22: */
> 23:
I've simplified the `C1` class, it's smaller now and has many smaller examples.
-
PR Review: https://git.openjdk.org/jdk/pull/19652#pullrequestreview-2117318623
PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1639144111
PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1639145147
PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1639147374
PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1639726374
PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1639145878
PR Review Comment: https://git.openjdk.org/jdk/pull/19652#discussion_r1639146304
Re: RFR: 8322708: Global HTML attributes are not allowed [v3]
> Can I please get a review for this change, that aims to add support for > Global HTML tags. > Here is the > [link](https://cr.openjdk.org/~nbenalla/javadocGlobalPR/pkg1/package-summary.html) > to the generated docs. > Thanks in advance. Nizar Benalla has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains ten additional commits since the last revision: - Null safety - Merge branch 'master' into html-attributes - remove trailing whitespace - -Add a boolean attribute to the enum type -Simply regex in `visitAttribute` -Simplified the Test -Added a negative test - no longer print summary - no longer print summary - Add small comment - Remove classpath exception - Allow global variables - Changes: - all: https://git.openjdk.org/jdk/pull/19652/files - new: https://git.openjdk.org/jdk/pull/19652/files/a1292496..248f1f2a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19652&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19652&range=01-02 Stats: 24208 lines in 691 files changed: 13960 ins; 8261 del; 1987 mod Patch: https://git.openjdk.org/jdk/pull/19652.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19652/head:pull/19652 PR: https://git.openjdk.org/jdk/pull/19652
Re: RFR: 8325690: The scrollable element with non-interactive content is not tabbable [v37]
On Fri, 14 Jun 2024 11:31:30 GMT, psoujany wrote: >> The scrollable element `` with non-interactive content is not tabbable. >> Grid columns in the javadoc stylesheet has overflow: auto, which is failing >> Accessibility checks. >> https://bugs.openjdk.org/browse/JDK-8325690 > > psoujany has updated the pull request incrementally with one additional > commit since the last revision: > > Add table role to summary tables @hns Thank you for the review. I tried to read more about table/grids and their associated ARIAs. As per [MDN](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-label), I added `role=table aria-label` for a table which doesn't has unique name and `role=table aria-labelledby` for the table having unique name and `role=row tabindex=0` to the table rows making them tabbable. Could you please review these changes. Thank You. - PR Comment: https://git.openjdk.org/jdk/pull/17819#issuecomment-2167846820
Re: RFR: 8325690: The scrollable element with non-interactive content is not tabbable [v37]
> The scrollable element `` with non-interactive content is not tabbable. > Grid columns in the javadoc stylesheet has overflow: auto, which is failing > Accessibility checks. > https://bugs.openjdk.org/browse/JDK-8325690 psoujany has updated the pull request incrementally with one additional commit since the last revision: Add table role to summary tables - Changes: - all: https://git.openjdk.org/jdk/pull/17819/files - new: https://git.openjdk.org/jdk/pull/17819/files/7717936a..a2b0bab7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17819&range=36 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17819&range=35-36 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17819.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17819/head:pull/17819 PR: https://git.openjdk.org/jdk/pull/17819
Re: RFR: 8325690: The scrollable element with non-interactive content is not tabbable [v36]
> The scrollable element `` with non-interactive content is not tabbable. > Grid columns in the javadoc stylesheet has overflow: auto, which is failing > Accessibility checks. > https://bugs.openjdk.org/browse/JDK-8325690 psoujany has updated the pull request incrementally with one additional commit since the last revision: Add table role to summary tables - Changes: - all: https://git.openjdk.org/jdk/pull/17819/files - new: https://git.openjdk.org/jdk/pull/17819/files/1534112b..7717936a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17819&range=35 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17819&range=34-35 Stats: 8 lines in 4 files changed: 0 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/17819.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17819/head:pull/17819 PR: https://git.openjdk.org/jdk/pull/17819
Re: RFR: 8325690: The scrollable element with non-interactive content is not tabbable [v35]
> The scrollable element `` with non-interactive content is not tabbable. > Grid columns in the javadoc stylesheet has overflow: auto, which is failing > Accessibility checks. > https://bugs.openjdk.org/browse/JDK-8325690 psoujany has updated the pull request incrementally with one additional commit since the last revision: Add table role to summary tables - Changes: - all: https://git.openjdk.org/jdk/pull/17819/files - new: https://git.openjdk.org/jdk/pull/17819/files/86030317..1534112b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17819&range=34 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17819&range=33-34 Stats: 42 lines in 12 files changed: 0 ins; 0 del; 42 mod Patch: https://git.openjdk.org/jdk/pull/17819.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17819/head:pull/17819 PR: https://git.openjdk.org/jdk/pull/17819
Re: RFR: 8325690: The scrollable element with non-interactive content is not tabbable [v34]
> The scrollable element `` with non-interactive content is not tabbable. > Grid columns in the javadoc stylesheet has overflow: auto, which is failing > Accessibility checks. > https://bugs.openjdk.org/browse/JDK-8325690 psoujany has updated the pull request incrementally with one additional commit since the last revision: Add table role to summary tables - Changes: - all: https://git.openjdk.org/jdk/pull/17819/files - new: https://git.openjdk.org/jdk/pull/17819/files/e84e18d9..86030317 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17819&range=33 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17819&range=32-33 Stats: 133 lines in 23 files changed: 0 ins; 0 del; 133 mod Patch: https://git.openjdk.org/jdk/pull/17819.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17819/head:pull/17819 PR: https://git.openjdk.org/jdk/pull/17819
