On Tue, 22 Feb 2022 14:46:27 GMT, Hannes Wallnöfer <hann...@openjdk.org> wrote:
>> Please review a change to straighten out the mechanism to avoid generating >> empty HTML elements in javadoc. >> >> Currently this is implemented using the `Content.isValid()` method in >> `HtmlTree.add(Content)` to check whether the the content argument should be >> added or dropped. This seems wrong since we don't really want to check for >> validity but non-emptiness (we want to drop empty elements, not print a >> warning or error). >> >> So the first decision was to get rid of the `Content.isValid()` method and >> use `Content.isEmpty()` instead. I considered keeping `isValid()` and using >> it to actually do some validation checks, but in the end decided it didn't >> make much sense and scrapped the method. >> >> My initial approach was to take care of emptiness at the content creation >> site, but it soon turned out there are too many sites potentially creating >> empty content. After fixing 40+ sites using a new >> `Content.addNonEmpty(Content)` method I still got empty elements that >> shouldn't be there in the JDK documentation. >> >> Therefore `HtmlTree.add(Content)` still checks for and drops empty content >> arguments. Instead, I added a new `HtmlTree.addUnchecked(Content)` method >> that adds the argument without the checks. The new method is only used 5 >> times in the whole codebase. It replaces the use of the special >> `HtmlTree.EMPTY` content token which is no longer needed. >> >> A few remarks about the rewritten `HtmlTree.isEmpty()` method: its purpose >> is to determine whether content is missing in an element that should have >> content. It therefore always returns `false` for elements that are expected >> or allowed to be without content. This is maybe a bit counter-intuitive, but >> it is what is needed. We could do a combined check such as `isVoid() || >> !isEmpty()` but that would still leave out elements that *can* be empty, >> such as `<script>`. >> >> The implementation of `HtmlTree.isEmpty()` is deliberately simple and >> conservative. For instance, it doesn't look at attributes to decide whether >> an element is allowed to be empty. The rationale for this is to make the >> behavior as predictable as possible and avoid surprises. >> >> Since we no longer have a validity check in `HtmlTree.add(Content)` I had to >> remove a few lines of code in `TestHtmlDocument.java` that expected invalid >> tags to be dropped. Otherwise there are no test changes, as this is a >> noreg-cleanup task. I did compare the javadoc JDK documention before and >> after this change, the output is unchanged. > > Hannes Wallnöfer has updated the pull request incrementally with one > additional commit since the last revision: > > Introduce Content.isDiscardable() What I propose is to introduce a `Content.isDiscardable()` method to decide whether a Content instance should be added or discarded. The default implementation delegates to `isEmpty()`, but for `HtmlTree` the logic is a bit more involved and decides whether an element is required to have content depending on the tag name. I think this better reflects the purpose of the method than both `isValid()` and `isEmpty()` as it is neither about validity or emptiness, but whether there is any benefit in adding a piece of content. Let me know if you are happy with this approach and the name of the new method. ------------- PR: https://git.openjdk.java.net/jdk/pull/7159
