<Swing Dev> RFR: 8260687: Inherited font size is smaller than expected when using StyleSheet to add styles [v3]

Thu Feb 11 22:16:39 UTC 2021

On Thu, 11 Feb 2021 18:02:00 GMT, Stanimir Stamenkov <github.com+1247730+stanio at openjdk.org> wrote:

>> Proposed fix for [JDK-8260687][].
>> 
>> It is noted the issue appeared after the [JDK-8257664][] (PR #1759) fix landed but the underlying problem is present even before that as demonstrated by using the following HTML document variant:
>> 
>> <html>
>> <body>
>>   <p style="font-size: 100%">16pt inherited from body</p>
>>   <p style="font-size: 16pt">16pt paragraph</p>
>> </body>
>> </html>
>> 
>> with the program setup given in the ticket.  The PR #1759 fix basically does just that – implies `font-size: 100%` where no `font-size` specified explicitly to ensure correctly computed font-size gets inherited when an ancestor specifies a percentage font-size different from `100%` (or `1em`) – otherwise the percentage is calculated and multiplied at every level of the inheritance, producing wrong results.
>> 
>> ---
>> 
>> The underlying problem is probably a combination of inconsistencies one may observe with the complex implementation of the [`W3C_LENGTH_UNITS`](https://docs.oracle.com/en/java/javase/15/docs/api/java.desktop/javax/swing/JEditorPane.html#W3C_LENGTH_UNITS) editor property.  Starting with only the `HTMLDocument.styleSheet` gets its internal `StyleSheet.w3cLengthUnits` field set up according to the `JEditorPane.W3C_LENGTH_UNITS` property.  Thus when a `font-size: 16pt` rule is part of the `HTMLEditorKit.styleSheet` which `w3cLengthUnits` generally remains `false` – querying the font for it from that style sheet yields a different result from querying the same rule via the `HTMLDocument.styleSheet`:
>> 
>>         JEditorPane editor = new JEditorPane();
>>         editor.putClientProperty(JEditorPane.W3C_LENGTH_UNITS, true);
>> 
>>         HTMLEditorKit htmlKit = new HTMLEditorKit();
>>         editor.setEditorKit(htmlKit);
>> 
>>         HTMLDocument htmlDoc = (HTMLDocument) editor.getDocument();
>> 
>>         StyleSheet kitStyles = htmlKit.getStyleSheet();
>>         StyleSheet docStyles = htmlDoc.getStyleSheet();
>>         assert Arrays.asList(docStyles.getStyleSheets()).contains(kitStyles);
>> 
>>         System.out.println(docStyles.getFont(docStyles.getRule("body"))); // [1]
>> 
>>         kitStyles.addRule("body { font-family: sans-serif; font-size: 16pt; }");
>> 
>>         System.out.println(kitStyles.getFont(kitStyles.getRule("body"))); // font.size = 16 (subjectively not expected)
>>         System.out.println(docStyles.getFont(docStyles.getRule("body"))); // font.size ≠ [1] ≠ 16 (expected)
>> 
>> That's probably fine as one may programmatically create and add more style sheets to the `HTMLDocument.styleSheet`.  These style sheets may further turn out shared with multiple documents/editors with different `W3C_LENGTH_UNITS` settings.
>> 
>> The next issue with the `W3C_LENGTH_UNITS` implementation, this PR proposes a fix for, is related to the current implementation of `StyleSheet.ViewAttributeSet.getAttribute()`:
>> 
>> https://github.com/openjdk/jdk/blob/70b5b3119b2ed032b021a080f34a4fa28d092fb5/src/java.desktop/share/classes/javax/swing/text/html/StyleSheet.java#L2810-L2818
>> 
>> which in turn invokes `CSS.FontSize.toStyleConstants()`:
>> 
>> https://github.com/openjdk/jdk/blob/48c932e1f1e5a79a28211f72dc9f10d8fd30b955/src/java.desktop/share/classes/javax/swing/text/html/CSS.java#L2195-L2199
>> 
>> One may notice the latter loses the `StyleSheet` context necessary to properly resolve `w3cLengthUnits` from `CSS.FontSize.getValue()`:
>> 
>> https://github.com/openjdk/jdk/blob/48c932e1f1e5a79a28211f72dc9f10d8fd30b955/src/java.desktop/share/classes/javax/swing/text/html/CSS.java#L2049-L2061
>> 
>> This context is properly sent from `StyleSheet.getFont()`, for example:
>> 
>> https://github.com/openjdk/jdk/blob/70b5b3119b2ed032b021a080f34a4fa28d092fb5/src/java.desktop/share/classes/javax/swing/text/html/StyleSheet.java#L913-L914
>> 
>> ---
>> 
>> I'll further add a test based on `BodyInheritedFontSize.java` already given to [JDK-8260687][] – I just want to enhance it with the:
>> 
>>   <p style="font-size: 100%">16pt inherited from body</p>
>> 
>> testing the issue from a different angle as noted at the beginning of this PR description.
>> 
>> [JDK-8257664]: https://bugs.openjdk.java.net/browse/JDK-8257664 "HTMLEditorKit: Wrong CSS relative font sizes"
>> [JDK-8260687]: https://bugs.openjdk.java.net/browse/JDK-8260687 "Inherited font size is smaller than expected when using StyleSheet to add styles"
>
> Stanimir Stamenkov has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8260687: Replace line-endings in BodyInheritedFontSize.java

> > I have copied the changes from the [aivanov-jdk/jdk at f9e9977](https://github.com/aivanov-jdk/jdk/commit/f9e997776fe4) branch earlier and made my revision to include the `font-size: 100%` case. I have the following adjustments that don't appear included in @aivanov-jdk's latest change:
> >
> > • The `<p style="font-size: 100%">...</p>` has to be before the `<p>...</p>` to trigger the pre-existing problem (f.e. in Java 11);
> 
> 
> I do not think the order matters. The paragraph which does not specify the font size and the paragraph which specifies it as 100% have the same size. This can be confirmed with another added check, does it make sense?

Okay, the order does matter, it produces different results. With percentage value first, one more scenario is covered.

The third comparison does not make sense. If the two not-equal-to conditions are false, `fontSizeInherited != fontSizePercentage` is also false.

test/jdk/javax/swing/text/html/StyleSheet/8260687/BodyInheritedFontSize.java line 144:

> 142:                     + "Inherited: " + fontSizeInherited + " vs. "
> 143:                     + "Explicit: " + fontSizeExplicit);
> 144:         }

I compared two pairs: `fontSizeInherited != fontSizeExplicit` and `fontSizePrecentage != fontSizeExplicit`. These two checks also imply `fontSizeInherited == fontSizePrecentage` which is also what we want.

Comparing only one pair of the three items is not enough.

Typo: fontSizePrecentage → fontSizePercentage.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2515