On Tue, 2 Feb 2021 12:29:00 GMT, Prasanta Sadhukhan <psadhuk...@openjdk.org>
wrote:
>>>
>>>
>>> > It seems for screen with low resolution, this change might cause some
>>> > failure as can be seen in the testcase attached in JBS Test.java.
>>>
>>> I'm afraid we can't make 72pt be exactly 96px because of the nature of
>>> floating point calculations which are not precise.
>>> The font size for 72pt is 94px instead of the expected 96px, the line
>>> height is 120px instead of 123px.
>>>
>>> What we can do to improve the accuracy is not to hard-code the constant as
>>> suggested at the moment but put 96/72 to map 'pt' unit to pixels.
>>>
>>> So 1.3 * 72 = 93.6 which is rounded to 94. Then 1.33 * 72 = 95.76 which is
>>> rounded to 96; 1.333 * 72 = 95.976 and so on. If 96/72 is stored as float,
>>> we'll have the most precise value.
>>>
>>> However, I'm pretty sure there are size / unit combinations which could
>>> make your test fail. But still, it's a good way to estimate the accuracy.
>>> Shall we add it as another test for this issue?
>>>
>>> If you disable, W3C_LENGTH_UNITS, you'll get a dramatic difference: 72pt =
>>> 72px, line height 92px but 'font-size: 96px' results in font size of 125px
>>> and line height of 159.
>>>
>>> Before @mperktold's fix is applied, the difference in size with
>>> W3C_LENGTH_UNITS is also significant, the letter 'C' is twice as small as
>>> the other letters; the two letters are rendered on the second line. In this
>>> case 72pt = 192px and line height of 244px, but 'font-size: 96px' has the
>>> expected size of 96px and line height of 123 px.
>>
>> Yes, it seems right to not hardcode and use 96/72f for "pt". I guess this
>> needs to be fixed as this test is part of our regression test and that will
>> fail after this fix, if not taken care and it will be considered as
>> regression.
>> I will also prefer that JDK-8260687 is also fixed as part of this PR as that
>> also concerns JEditorPane.W3C_LENGTH_UNITS property.
>
> It seems Toolkit.getDefaultToolkit().getScreenResolution() can return
> 93/94/95 instead of 96 in mac/linux in internal mach5 testing systems causing
> failure in this test Test.java. Probably we need to make the testcase
> hardcoded to 96
>
> res = 93
> Font Size for InlineView #0 = 96; height = 96; element = {
> <content
> span=font-size=72pt
> font-size=72pt
> name=content
> >
> [1,2][A]
> }
> Font Size for InlineView #1 = 96; height = 96; element = {
> <content
> span=font-size=6pc
> font-size=6pc
> name=content
> >
> [2,3][B]
> }
> Font Size for InlineView #2 = 93; height = 94; element = {
> <content
> span=font-size=93px
> font-size=93px
> name=content
> >
> [3,4][C]
> }
> Font Size for InlineView #3 = 96; height = 96; element = {
> <content
> span=font-size=25.4mm
> font-size=25.4mm
> name=content
> >
> [4,5][D]
> }
> Font Size for InlineView #4 = 96; height = 96; element = {
> <content
> span=font-size=2.54cm
> font-size=2.54cm
> name=content
> >
> [5,6][E]
> }
> Font Size for InlineView #5 = 96; height = 96; element = {
> <content
> span=font-size=1in
> font-size=1in
> name=content
> >
> [6,7][F]
> }
> ----------System.err:(5/314)----------
> java.lang.RuntimeException: Test failed.
>
> I also suggest fixing JDK-8260687 to not use font inherit for W3C_LENGTH_UNIT
> case
>
> --- a/src/java.desktop/share/classes/javax/swing/text/html/StyleSheet.java
> +++ b/src/java.desktop/share/classes/javax/swing/text/html/StyleSheet.java
> @@ -2823,7 +2823,7 @@ public class StyleSheet extends StyleContext {
> }
>
> Object doGetAttribute(Object key) {
> - if (key == CSS.Attribute.FONT_SIZE && !isDefined(key)) {
> + if (key == CSS.Attribute.FONT_SIZE && !isDefined(key) &&
> !isW3CLengthUnits()) {
> // CSS.FontSize represents a specified value and we need
> // to inherit a computed value so don't resolve percentage
> // value from parent.
> It seems Toolkit.getDefaultToolkit().getScreenResolution() can return
> 93/94/95 instead of 96 in mac/linux in internal mach5 testing systems causing
> failure in this test Test.java. Probably we need to make the testcase
> hardcoded to 96
>
> ```
>
> res = 93
> Font Size for InlineView #0 = 96; height = 96; element = {
> <content
> span=font-size=72pt
> font-size=72pt
> name=content
> >
> [1,2][A]
> }
> [...]
> ----------System.err:(5/314)----------
> java.lang.RuntimeException: Test failed.
> ```
Sorry, but I don't understand. What do these examples represent? Are they the
results of some tests you ran? Or should I encorporate them into the test
somehow?
Also, regarding a resolution of 93/94/95, I think rather than hardcoding 96, we
should ensure that the behavior is correct for various screen sizes. I don't
know how to change the resolution just for the test, but if that is possible, I
think we should test several values, like 96, something below that and
something above.
> I also suggest fixing JDK-8260687 to not use font inherit for W3C_LENGTH_UNIT
> case
>
> ```
> --- a/src/java.desktop/share/classes/javax/swing/text/html/StyleSheet.java
> +++ b/src/java.desktop/share/classes/javax/swing/text/html/StyleSheet.java
> @@ -2823,7 +2823,7 @@ public class StyleSheet extends StyleContext {
> }
>
> Object doGetAttribute(Object key) {
> - if (key == CSS.Attribute.FONT_SIZE && !isDefined(key)) {
> + if (key == CSS.Attribute.FONT_SIZE && !isDefined(key) &&
> !isW3CLengthUnits()) {
> // CSS.FontSize represents a specified value and we need
> // to inherit a computed value so don't resolve percentage
> // value from parent.
> ```
Just the be sure before I do that, are there going to be problems when I pull
changes from master into this branch?
Or was [this bot
comment](https://github.com/openjdk/jdk/pull/2223#issuecomment-766934951) just
about the name of the branch, and everything should be fine?
-------------
PR: https://git.openjdk.java.net/jdk/pull/2256
