RFR: 8247957: remove doclint support for HTML 4
Yoshiki Sato
ysatowse at openjdk.java.net
Fri Dec 11 07:23:22 UTC 2020
On Thu, 12 Nov 2020 02:28:51 GMT, Jonathan Gibbons <jjg at openjdk.org> wrote:
>> HTML4 is no longer supported in javadoc.
>>
>> doclint needs to drop HTML4 support as well.
>> The changes consist of:
>> * Removing jdk.javadoc.internal.doclint.HtmlVersion and its references.
>> * Sorting out supported tags and attributes in HTML5 (including fix incorrect permission of valign in TH, TR, TD, THEAD and TBODY)
>> * Modifying test code and expected outputs to be checked in HTML5
>
> Generally nice; congratulations on figuring this all out.
>
> Some comments in files need response and/or action.
@jonathan-gibbons The new commit includes the changes for your review comments and "8256312: Valid anchor 'id' value not allowed". Please take a look over at your convenience.
> Please make the review non-draft as well
Thanks for reviewing @jonathan-gibbons
This request should have got changed to "non-draft".
FYI. all tests in jdk-tier1 are still green with the latest changes.
https://mach5.us.oracle.com/mdash/jobs/yoshiki-jdk-20201211-0555-16616611
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/resources/doclint_zh_CN.properties line 30:
>
>> 28: dc.attr.lacks.value = \u5C5E\u6027\u7F3A\u5C11\u503C
>> 29: dc.attr.not.number = \u5C5E\u6027\u503C\u4E0D\u662F\u6570\u5B57
>> 30: dc.attr.not.supported.html4 = \u5C5E\u6027\u5728 HTML4 \u4E2D\u4E0D\u53D7\u652F\u6301: {0}
>
> In general, we (Dev) do not edit any localized resource files. That happens "automatically" by the localization team.
I see. Will discard all changes done in the localized resource files.
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/HtmlTag.java line 410:
>
>> 408: OBSOLETE,
>> 409: UNSUPPORTED
>> 410: }
>
> On one hand, I don't think we need this level of detail, but on the other, I see it closely matches `AttrKind`, so OK.
>
> Is there are useful distinction between INVALID / OBSOLETE / UNSUPPORTED ?
OK: valid
OBSOLETE: obsolete, deprecated, but still supported (valid)
UNSUPPORTED: ever supported but no longer supported (invalid)
INVALID: the rest of others (invalid)
UNSUPPORTED can be used if we would like to choose a friendly message instead of saying "unknown tag" only.
OBSOLETE is not used anywhere in this commit. Although HTML5 has some obsolete features, [JDK-8215577](https://bugs.openjdk.java.net/browse/JDK-8215577) didn't define them as valid features if my understanding is correct. So I chose not to allow obsolete features in order to avoid inconsistency.
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/Checker.java line 692:
>
>> 690: }
>> 691: } catch (NumberFormatException ex) {
>> 692: env.messages.error(HTML, tree, "dc.attr.img.border", attr);
>
> It's generally better practice to use unique message keys at each call site. This is so that if there is a downstream problem when someone reports a problem with an error message, we know the exact single place in the code where the message comes from.
Agreed. I would like to change 675-695 as below
case BORDER:
if (currTag == HtmlTag.TABLE) {
String v = getAttrValue(tree);
try {
if (v == null || (!v.isEmpty() && Integer.parseInt(v) != 1)) {
env.messages.error(HTML, tree, "dc.attr.table.border.not.valid", attr);
}
} catch (NumberFormatException ex) {
env.messages.error(HTML, tree, "dc.attr.table.border.not.number", attr);
}
} else if (currTag == HtmlTag.IMG) {
String v = getAttrValue(tree);
try {
if (v == null || (!v.isEmpty() && Integer.parseInt(v) != 0)) {
env.messages.error(HTML, tree, "dc.attr.img.border.not.valid", attr);
}
} catch (NumberFormatException ex) {
env.messages.error(HTML, tree, "dc.attr.img.border.not.number", attr);
}
}
break;
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/DocLint.java line 229:
>
>> 227: String argVersion = arg.substring(arg.indexOf(":") + 1);
>> 228: if (argVersion == null || !argVersion.equals("html5")) {
>> 229: throw new BadArgs("dc.bad.value.for.option", arg, argVersion);
>
> It might be friendly to give a warning when this option is used, saying that `html5` is the default and only supported version, and that this option may be removed in a future release.
Ok
> test/langtools/tools/doclint/html/TableTagTest.out line 22:
>
>> 20: * <table summary="abc" width="50%"> <tr> <td> <tfoot> <tr> </tfoot></table>
>> 21: ^
>> 22: 7 errors
>
> Does it work to add the final newline: various tools give warnings if files do not end with newline.
It works though. But I will add a blank line at the end for consistency.
> test/langtools/tools/doclint/html/TextNotAllowed.out line 30:
>
>> 28: TextNotAllowed.java:16: error: attribute not supported in HTML5: summary
>> 29: * <table summary=description> abc </table>
>> 30: ^
>
> The test is 'Text Not Allowed', so the test file should be valid except for text where it is not allowed. Don't add spurious other errors. In this case, provide a `<caption>` to keep DocLint happy.
>
> This applies throughout this test.
Thanks. I was lacking of such perspective.
> test/langtools/tools/doclint/tidy/TextNotAllowed.out line 2:
>
>> 1: TextNotAllowed.java:14: error: attribute not supported in HTML5: summary
>> 2: * <table summary=description> abc </table>
>
> See previous comments about spurious error messages
Ok.
> test/langtools/tools/doclint/tidy/TrimmingEmptyTag.out line 5:
>
>> 3: ^
>> 4: TrimmingEmptyTag.java:15: error: attribute not supported in HTML5: summary
>> 5: * <table summary=description></table>
>
> ditto
Ok.
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/HtmlTag.java line 51:
>
>> 49: * public.
>> 50: *
>> 51: * @see <a href="http://www.w3.org/TR/REC-html40/">HTML 4.01 Specification</a>
>
> Given the presence of UNSUPPORTED in the ensuing code, I'd recommend leaving this link, for historical purposes.
Ok.
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/HtmlTag.java line 360:
>
>> 358: attrs(AttrKind.OK, COLSPAN, ROWSPAN, HEADERS, SCOPE, Attr.ABBR),
>> 359: attrs(AttrKind.UNSUPPORTED, WIDTH, BGCOLOR, HEIGHT, NOWRAP, AXIS, ALIGN, CHAR, CHAROFF),
>> 360: attrs(AttrKind.OK, VALIGN)), // Removed after JDK-8255214 fixed.
>
> Does this need attention? JDK-8255215 has been fixed
Yes, now that this can be removed. I will do.
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/DocLint.java line 348:
>
>> 346: String argVersion = arg.substring(arg.indexOf(":") + 1);
>> 347: if (argVersion == null || !argVersion.equals("html5")) {
>> 348: throw new IllegalArgumentException(argVersion);
>
> These lines are only used when invoked from javac/javadoc etc, so it would be reasonable to delete them entirely, provided those tools never try and use this option.
These lines are likely to be used as long as the "--doclint-format html5" option is permitted. For example, this option is still used in the make/common/JavaCompilation.gmk.
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/HtmlTag.java line 541:
>
>> 539: OBSOLETE,
>> 540: UNSUPPORTED
>> 541: }
>
> Is there a useful distinction between INVALID / OBSOLETE / UNSUPPORTED ?
Same distinction as mentioned for ElemKind
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/resources/doclint.properties line 32:
>
>> 30: dc.attr.not.supported.html5 = attribute not supported in HTML5: {0}
>> 31: dc.attr.obsolete = attribute obsolete: {0}
>> 32: dc.attr.obsolete.use.css = attribute obsolete, use CSS instead: {0}
>
> Is this still required?
Do you mean ".html5" would be better to be removed?
If so, yes. I just left it because the message would be friendly if it still say "attribute not supported in HTML5".
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/resources/doclint.properties line 35:
>
>> 33: dc.attr.repeated = repeated attribute: {0}
>> 34: dc.attr.img.border = attribute border for img only accepts "0", use CSS instead: {0}
>> 35: dc.attr.table.border = attribute border for table only accepts "" or "1", use CSS instead: {0}
>
> I suggest quoting the attribute name "border"
Ok.
> test/langtools/tools/doclint/AccessibilityTest.out line 28:
>
>> 26: * <table><tr><th>head<tr><td>data</table>
>> 27: ^
>> 28: AccessibilityTest.java:66: error: no summary or caption for table
>
> summary is not a valid attribute in HTML5, so I would omit it from the message.
You are right. I will remove "summary" from the message, and then remove two of the tests that still use "summary".
> test/langtools/tools/doclint/AnchorTest.java line 3:
>
>> 1: /*
>> 2: * @test /nodynamiccopyright/
>> 3: * @bug 8004832 8247957
>
> The bug number should only be added when the test can be considered to be a significant test of the work.
>
> In general, don't bother to add the bug number when a test is just incidentally affected by the work.
>
> In this case, there appears to be no other change to the file, but I guess OK, you've changed to golden output file.
Understood. Thanks for your instruction.
> test/langtools/tools/doclint/html/HtmlVersionTagsAttrsTest.java line 11:
>
>> 9: * @run main DocLintTester -badargs -XhtmlVersion: HtmlVersionTagsAttrsTest.java
>> 10: *
>> 11: *
>
> I'm guessing these blank lines are to maintain line numbers, right?
Right
> test/langtools/tools/doclint/tidy/InvalidName.out line 3:
>
>> 1: InvalidName.java:17: error: invalid name for anchor: "foo()"
>> 2: * <a id="foo()">invalid</a>
>> 3: ^
>
> This seems incorrect. In HTML5 all names are valid unless they contain whitespace.
Understood. Need to file it as another DocLint bug.
-------------
PR: https://git.openjdk.java.net/jdk/pull/893
More information about the compiler-dev
mailing list