On Thu, 15 Jun 2023 03:50:50 GMT, Prasanta Sadhukhan <[email protected]>
wrote:
>> src/java.desktop/share/classes/javax/swing/text/html/HTMLDocument.java line
>> 550:
>>
>>> 548: if (t.isBlock()) {
>>> 549: // TBD
>>> 550: return null;
>>
>> This doesn't look right to me. The comment suggests the implementation of
>> something like `BlockIterator` is missing.
>>
>> I would expect that iterator for `HTML.Tag.HEAD` returns all elements inside
>> `<head>` element. Yet the comment for the `next` method
>>
>> https://github.com/openjdk/jdk/blob/931625a9304ec2761ca9035d69fd33f6beadb124/src/java.desktop/share/classes/javax/swing/text/html/HTMLDocument.java#L1993-L1997
>>
>> suggests, the iterator goes over all instances of the specified tag in the
>> document.
>>
>> What does the iterator return if you pass it `HTML.Tag.HEAD` or `HTML.Tag.P`?
>
> It returns a LeafIterator object with HEAD or P tag respectively
Obviously. :)
Does it do what the `getIterator` method promise to do? It doesn't because it
iterates over `LeafElement`-s only:
https://github.com/openjdk/jdk/blob/931625a9304ec2761ca9035d69fd33f6beadb124/src/java.desktop/share/classes/javax/swing/text/html/HTMLDocument.java#L1993-L1998
So, the `next` method moves the iterator to the next leaf element:
https://github.com/openjdk/jdk/blob/931625a9304ec2761ca9035d69fd33f6beadb124/src/java.desktop/share/classes/javax/swing/text/html/HTMLDocument.java#L2037-L2044
Note that it uses `isLeaf` to stop the generic `ElementIterator`.
---
It looks as if `HTMLDocument.getIterator` is public as an implementation
detail: it's used to iterate over `HTML.Tag.A` to scroll the text component to
the anchor and to determine whether the document is a frame set or not.
Removing the `if` condition removes the NPE but the method doesn't support
Block Elements.
Should we update the javadoc to state the method shouldn't be used by apps? And
to throw `UnsupportedOperationException` if a block tag is passed instead of
returning null?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14466#discussion_r1230580722
