8247957: remove doclint support for HTML 4 #893
Conversation
|
Hi @satoyoshiki, welcome to this OpenJDK project and thanks for contributing! We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user satoyoshiki" as summary for the issue. If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing |
|
@satoyoshiki The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
openjdk
bot
added
javadoc
| @@ -27,12 +27,12 @@ dc.anchor.already.defined = \u951A\u5B9A\u70B9\u5DF2\u5B9A\u4E49: "{0}" | |||
| dc.anchor.value.missing = \u6CA1\u6709\u4E3A\u951A\u5B9A\u70B9\u6307\u5B9A\u503C | |||
| dc.attr.lacks.value = \u5C5E\u6027\u7F3A\u5C11\u503C | |||
| dc.attr.not.number = \u5C5E\u6027\u503C\u4E0D\u662F\u6570\u5B57 | |||
| dc.attr.not.supported.html4 = \u5C5E\u6027\u5728 HTML4 \u4E2D\u4E0D\u53D7\u652F\u6301: {0} | |||
There was a problem hiding this comment.
In general, we (Dev) do not edit any localized resource files. That happens "automatically" by the localization team.
There was a problem hiding this comment.
I see. Will discard all changes done in the localized resource files.
| public static enum ElemKind { | ||
| OK, | ||
| INVALID, | ||
| OBSOLETE, | ||
| UNSUPPORTED | ||
| } |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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 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.
There was a problem hiding this comment.
For both ElemKind and AttrKind there only seem to be two kinds:
- valid
- previously valid
For these two cases, OK is obviously reasonable for valid, but OBSOLETE seems a better fit than UNSUPPORTED, but you could also use HTML4 or OLD_HTML4 or something like that to indicate why we're keeping the name around for better messages. Or, stay with UNSUPPORTED but add a doc comment explaining that it was previously supported but no longer supported
| env.messages.error(HTML, tree, "dc.attr.img.border", attr); | ||
| } | ||
| } catch (NumberFormatException ex) { | ||
| env.messages.error(HTML, tree, "dc.attr.img.border", attr); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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;
| String argVersion = arg.substring(arg.indexOf(":") + 1); | ||
| if (argVersion == null || !argVersion.equals("html5")) { | ||
| throw new BadArgs("dc.bad.value.for.option", arg, argVersion); |
There was a problem hiding this comment.
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.
| } else if (arg.startsWith(XHTML_VERSION_PREFIX)) { | ||
| String argsVersion = arg.substring(arg.indexOf(":") + 1); | ||
| HtmlVersion htmlVersion = HtmlVersion.getHtmlVersion(argsVersion); | ||
| if (htmlVersion != null) { | ||
| env.setHtmlVersion(htmlVersion); | ||
| } else { | ||
| throw new IllegalArgumentException(argsVersion); | ||
| String argVersion = arg.substring(arg.indexOf(":") + 1); | ||
| if (argVersion == null || !argVersion.equals("html5")) { | ||
| throw new IllegalArgumentException(argVersion); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
- we can update make/common/JavaCompilation.gmk
- the option in javac/javadoc should be converted to a no-op so that it does not get passed down to doclint
There was a problem hiding this comment.
(updating make/common/JavaCompilation.gmk should be done with a separate JBS issue, filed against the build.)
There was a problem hiding this comment.
In javac Arguments class, we should delete these lines at about 855
String format = options.get(Option.DOCLINT_FORMAT);
if (format != null) {
doclintOpts.add(DocLint.XHTML_VERSION_PREFIX + format);
}
This could be a separate JBS issue, if you prefer.
| TableTagsTest.java:43: error: attribute not supported in HTML5: width | ||
| * <table summary="abc" width="50%"> <tr> <td> <tfoot> <tr> </tfoot></table> | ||
| ^ | ||
| 7 errors |
There was a problem hiding this comment.
Does it work to add the final newline: various tools give warnings if files do not end with newline.
There was a problem hiding this comment.
It works though. But I will add a blank line at the end for consistency.
| TextNotAllowed.java:16: error: attribute not supported in HTML5: summary | ||
| * <table summary=description> abc </table> | ||
| ^ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks. I was lacking of such perspective.
| InvalidName.java:17: error: invalid name for anchor: "foo()" | ||
| * <a name="foo()">invalid</a> | ||
| * <a id="foo()">invalid</a> | ||
| ^ |
There was a problem hiding this comment.
This seems incorrect. In HTML5 all names are valid unless they contain whitespace.
There was a problem hiding this comment.
Understood. Need to file it as another DocLint bug.
| @@ -1,9 +1,18 @@ | |||
| TextNotAllowed.java:14: error: attribute not supported in HTML5: summary | |||
| * <table summary=description> abc </table> | |||
There was a problem hiding this comment.
See previous comments about spurious error messages
| @@ -1,6 +1,9 @@ | |||
| TrimmingEmptyTag.java:14: warning: empty <b> tag | |||
| * <b></b> | |||
| ^ | |||
| TrimmingEmptyTag.java:15: error: attribute not supported in HTML5: summary | |||
| * <table summary=description></table> | |||
8256312: Valid anchor 'id' value not allowed
|
@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. |
| @@ -144,6 +144,8 @@ public void run(PrintWriter out, String... args) throws BadArgs, IOException { | |||
| } else if (noFiles) { | |||
| out.println(localize("dc.main.no.files.given")); | |||
| return; | |||
| } else if (useXhtmlVersion) { | |||
| System.err.println(localize("dc.main.use.xhtmlversion")); | |||
| @@ -356,8 +357,7 @@ public boolean accepts(HtmlTag t) { | |||
| TH(BlockType.TABLE_ITEM, EndKind.OPTIONAL, | |||
| EnumSet.of(Flag.ACCEPTS_BLOCK, Flag.ACCEPTS_INLINE), | |||
| attrs(AttrKind.OK, COLSPAN, ROWSPAN, HEADERS, SCOPE, Attr.ABBR), | |||
| attrs(AttrKind.UNSUPPORTED, WIDTH, BGCOLOR, HEIGHT, NOWRAP, AXIS, ALIGN, CHAR, CHAROFF), | |||
| attrs(AttrKind.OK, VALIGN)), // Removed after JDK-8255214 fixed. | |||
| attrs(AttrKind.UNSUPPORTED, WIDTH, BGCOLOR, HEIGHT, NOWRAP, AXIS, ALIGN, CHAR, CHAROFF, VALIGN)), // Removed after JDK-8255214 fixed. | |||
| @@ -372,8 +372,7 @@ public boolean accepts(HtmlTag t) { | |||
| TITLE(BlockType.OTHER, EndKind.REQUIRED), | |||
|
|
|||
| TR(BlockType.TABLE_ITEM, EndKind.OPTIONAL, | |||
| attrs(AttrKind.UNSUPPORTED, ALIGN, CHAR, CHAROFF, BGCOLOR), | |||
| attrs(AttrKind.OK, VALIGN)) { // Removed after JDK-8255215 fixed | |||
| attrs(AttrKind.UNSUPPORTED, ALIGN, CHAR, CHAROFF, BGCOLOR, VALIGN)) { // Removed after JDK-8255215 fixed | |||
| @@ -87,6 +88,7 @@ dc.value.not.a.constant=value does not refer to a constant | |||
|
|
|||
| dc.main.ioerror=IO error: {0} | |||
| dc.main.no.files.given=No files given | |||
| dc.main.use.xhtmlversion=html5 is the default and only supported version for -XhtmlVersion, and this option may be removed in a future release. | |||
There was a problem hiding this comment.
change version to value
change , and to ;
html5 is the default and only supported value for -XhtmlVersion; this option may be removed in a future release.
|
Thanks for reviewing @jonathan-gibbons FYI. all tests in jdk-tier1 are still green with the latest changes. |
Webrevs
|
|
This commit includes the changes for The changes presume the changes made by a 8247957. And there is no need to separate them from 8247957, so I would like to pull all changes into this. |
|
@satoyoshiki This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 2 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jonathan-gibbons, @magicus) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
I found an issue when called from javac, so will convert to draft until the issue resolved. |
8257204: Remove usage of -Xhtmlversion option from javac 8256313: JavaCompilation.gmk needs to be updated not to use --doclint-format html5 option
src/jdk.compiler/share/man/javac.1
Outdated
| .B \f[CB]\-\-doclint\-format\f[R] [\f[CB]html4\f[R]|\f[CB]html5\f[R]] | ||
| Specifies the format for documentation comments. | ||
| .RS | ||
| .RE | ||
| .TP |
There was a problem hiding this comment.
In general, you should not edit files like this; these files are generated from upstream Markdown files.
There was a problem hiding this comment.
Ok. I will revert this with the next commit.
| // http://www.w3.org/TR/html401/types.html#type-name | ||
| private static final Pattern validName = Pattern.compile("[A-Za-z][A-Za-z0-9-_:.]*"); | ||
| // https://html.spec.whatwg.org/#the-id-attribute | ||
| private static final Pattern validId = Pattern.compile("[^\s]+"); |
There was a problem hiding this comment.
The regular expression is invalid and needs to be fixed. It should be Pattern.compile("[^\\s]+")
Note the extra \ character. This is because you need to escape the \ character in the string constant, so that the \ is seen in the pattern as part of \s.
There was a problem hiding this comment.
Correct. Thanks a lot for finding this error.
Now that I have doubts why this line could have been compiled without error. This line should cause a compiler error.
Let me review all anchor tests again because the logic should be checked there.
There was a problem hiding this comment.
For some reason, "[^\s]+" might have been dealt with as a text block, thus \s was regarded as a whitespace...
I believe a text block is defined with triple double quotes, but javac seems to accept "[^\s]+" as a text block.
That aside, all anchor tests look fine. This is because there is no specific test to use whitespace characters such as \n, \t, \r and \f in an anchor name. Also I confirmed the discrepancy of the results for "[^\s]+" and "[^\\s]+". It shows that the former is not exactly what we want to do.
$ jshell -J-Duser.language=en -J--show-version
java 15 2020-09-15
Java(TM) SE Runtime Environment (build 15+36-1562)
Java HotSpot(TM) 64-Bit Server VM (build 15+36-1562, mixed mode, sharing)
| Welcome to JShell -- Version 15
| For an introduction type: /help intro
jshell> Pattern validId1 = Pattern.compile("[^\s]+")
validId1 ==> [^ ]+
jshell> Pattern validId2 = Pattern.compile("[^\\s]+")
validId2 ==> [^\s]+
jshell> validId1.matcher("aaa").matches()
$3 ==> true
jshell> validId1.matcher("aaa ").matches()
$4 ==> false
jshell> validId1.matcher("aaa\n").matches()
$5 ==> true
jshell> validId2.matcher("aaa").matches()
$6 ==> true
jshell> validId2.matcher("aaa ").matches()
$7 ==> false
jshell> validId2.matcher("aaa\n").matches()
$8 ==> false
| env.messages.error(HTML, tree, "dc.attr.not.supported.html5", name); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| private boolean checkAnchor(String name) { |
There was a problem hiding this comment.
I was going to let it slide for this round of cleanup, but if you're editing this file again (see comment on line 736) it might be worth changing the use of anchor to id. anchor is a term that was more appropriate in the days before the id attribute, when we used <a name="...">. This is an optional suggestion. It might equally be worth focussing on the must-fix items, and postpone this cleanup for later.
There was a problem hiding this comment.
I understand. But is it really no problem to be done in part of the cleanup of doclint?
Looking at the classes in jdk/javadoc/internal/doclint, the term (anchor|Anchor) looks like only used in Checker.java and resource files. But a lot of other files, for instance in jdk/javadoc/internal/doclets, use this term to refer to the id or name attribute. I would be fine if it is supposed to be done in each cleanup in the future.
There was a problem hiding this comment.
If we would apply similar changing to doclint.properties, what you are thinking is like below right?
-dc.anchor.already.defined = anchor already defined: "{0}"
-dc.anchor.value.missing = no value given for anchor
+dc.id.already.defined = attribute "id" already defined: "{0}"
+dc.id.value.missing = no value given for attribute "id"
-dc.invalid.anchor = invalid name for anchor: "{0}"
-dc.invalid.id = invalid name for attribute "id": "{0}"
|
On 12/22/20 10:42 PM, Yoshiki SATO wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/Checker.java
<https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/893*discussion_r547716025__;Iw!!GqivPVa7Brio!Oae1uD8yALBU6ReCJTwKccZjdEsGyXddDLOOeF2SJLi8x7SoU5jr-Jr0KVl2EVajBRHM-g$>:
> - }
-
- private void validateHtml5Attrs(AttributeTree tree, Name name, AttrKind k) {
- switch (k) {
- case ALL:
- case HTML5:
- break;
-
- case INVALID:
- case OBSOLETE:
- case USE_CSS:
- case HTML4:
- env.messages.error(HTML, tree, "dc.attr.not.supported.html5", name);
- break;
- }
- }
private boolean checkAnchor(String name) {
I understand. But is it really no problem to be done in part of the
cleanup of doclint?
Looking at the classes in jdk/javadoc/internal/doclint, the term
|(anchor|Anchor)| looks like only used in Checker.java and resource
files. But a lot of other files, for instance in
jdk/javadoc/internal/doclets, use this term to refer to the |id| or
|name| attribute. I would be fine if it is supposed to be done in each
cleanup in the future.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/893*discussion_r547716025__;Iw!!GqivPVa7Brio!Oae1uD8yALBU6ReCJTwKccZjdEsGyXddDLOOeF2SJLi8x7SoU5jr-Jr0KVl2EVajBRHM-g$>,
or unsubscribe
<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AOUXBRQS4FULCERHKMAZ5OLSWGGNVANCNFSM4TBXTH2Q__;!!GqivPVa7Brio!Oae1uD8yALBU6ReCJTwKccZjdEsGyXddDLOOeF2SJLi8x7SoU5jr-Jr0KVl2EVYEkdTG2A$>.
If you were to cleanup the "anchor" terminology, it should only be done
in doclint, meaning Checker.java and resource files. Cleaning up
terminology in the rest of javadoc is another project for another day.
But, it's OK to skip this terminology cleanup in doclint at this time,
since it is only peripherally related to the primary goal to remove
HTML4 support.
…-- Jon
|
|
Although `\s` was introduced as part of the work for text blocks, it was
added to the set of escape characters accepted in string and character
literals. So, no, that constant is not being treated as a text block,
because text blocks only begin and end with triple-quotes: `"""`
See the JLS links I gave in my previous response.
…-- Jon
On 12/22/20 11:50 PM, Yoshiki SATO wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/Checker.java
<https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/893*discussion_r547768833__;Iw!!GqivPVa7Brio!JzRqq_cZpzUEFer1m5NX92ABR7tcna9povirJpPwfSxuO4EFI0tRAmRLw80UwpJdsJ1BgQ$>:
> @@ -765,8 +732,8 @@ private Element getEnclosingPackageOrClass(Element e) {
return e;
}
- //http://www.w3.org/TR/html401/types.html#type-name <https://urldefense.com/v3/__http://www.w3.org/TR/html401/types.html*type-name__;Iw!!GqivPVa7Brio!JzRqq_cZpzUEFer1m5NX92ABR7tcna9povirJpPwfSxuO4EFI0tRAmRLw80UwpJRSKFcFA$>
- private static final Pattern validName = Pattern.compile("[A-Za-z][A-Za-z0-9-_:.]*");
+ //https://html.spec.whatwg.org/#the-id-attribute <https://urldefense.com/v3/__https://html.spec.whatwg.org/*the-id-attribute__;Iw!!GqivPVa7Brio!JzRqq_cZpzUEFer1m5NX92ABR7tcna9povirJpPwfSxuO4EFI0tRAmRLw80UwpLwqIk5gQ$>
+ private static final Pattern validId = Pattern.compile("[^\s]+");
For some reason, |"[^\s]+"| might have been dealt with as a text
block, thus |\s| was regarded as a whitespace...
I believe a text block is defined with triple double quotes, but javac
seems to accept |"[^\s]+"| as a text block.
That aside, all anchor tests look fine. This is because there is no
specific test to use whitespace characters such as |\n|, |\t|, |\r|
and |\f| in an anchor name. Also I confirmed the discrepancy of the
results for |"[^\s]+"| and |"[^\\s]+"|. It shows that the former is
not exactly what we want to do.
/var/tmp/jib-yoshiki/install/jdk/15/36/bundles/osx-x64/jdk-15_osx-x64_bin.tar.gz/jdk-15.jdk/Contents/Home/bin/jshell
-J-Duser.language=en -J--show-version
java 15 2020-09-15
Java(TM) SE Runtime Environment (build 15+36-1562)
Java HotSpot(TM) 64-Bit Server VM (build 15+36-1562, mixed mode, sharing)
| Welcome to JShell -- Version 15
| For an introduction type: /help intro
jshell> Pattern validId1 = Pattern.compile("[^\s]+")
validId1 ==> [^ ]+
jshell> Pattern validId2 = Pattern.compile("[^\s]+")
validId2 ==> [^\s]+
jshell> validId1.matcher("aaa").matches()
$3 ==> true
jshell> validId1.matcher("aaa ").matches()
$4 ==> false
jshell> validId1.matcher("aaa\n").matches()
$5 ==> true
jshell> validId2.matcher("aaa").matches()
$6 ==> true
jshell> validId2.matcher("aaa ").matches()
$7 ==> false
jshell> validId2.matcher("aaa\n").matches()
$8 ==> false
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/893*discussion_r547768833__;Iw!!GqivPVa7Brio!JzRqq_cZpzUEFer1m5NX92ABR7tcna9povirJpPwfSxuO4EFI0tRAmRLw80UwpJdsJ1BgQ$>,
or unsubscribe
<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AOUXBRQZVY3MLTBJ7SZR7ETSWGONXANCNFSM4TBXTH2Q__;!!GqivPVa7Brio!JzRqq_cZpzUEFer1m5NX92ABR7tcna9povirJpPwfSxuO4EFI0tRAmRLw80UwpK72GWUdA$>.
|
Ok. I didn't know that. When I change my project SDK to 14, the escape sequence showed up with red squiggly. |
|
@satoyoshiki this pull request can not be integrated into git checkout JDK-8247957_2
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
openjdk
bot
added
merge-conflict
|
/issue add 8257204, 8256313, 8258460, 8256312 |
|
@satoyoshiki Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: |
openjdk
bot
added
ready
|
/integrate |
|
@satoyoshiki |
|
/sponsor |
openjdk
bot
added
integrated
|
@jonathan-gibbons @satoyoshiki Since your change was applied there have been 52 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 28e1f4d. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
HTML4 is no longer supported in javadoc.
doclint needs to drop HTML4 support as well.
The changes consist of:
Progress
Issues
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/893/head:pull/893$ git checkout pull/893