-
Notifications
You must be signed in to change notification settings - Fork 5.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8231286: HTML font size too large with high-DPI scaling and W3C_LENGTH_UNITS #2256
Conversation
Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
👋 Welcome back mperktold! A progress list of the required criteria for merging this PR into |
@mperktold The following label 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 list. If you would like to change these labels, use the /label pull request command. |
You should edit the title of the PR to be the same as the subject of the bug in JBS: Please also provide the additional information in the description from the PR #2223. |
Webrevs
|
w3cLengthMapping.put("cm", res / 2.54f); | ||
w3cLengthMapping.put("pc", res / 6f); | ||
w3cLengthMapping.put("in", (float) res); | ||
// mapping according to the CSS2.2 spec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not put the latest CSS link here in addition/instead of in JEditorPane?
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. |
w3cLengthMapping.put("cm", res / 2.54f); | ||
w3cLengthMapping.put("pc", res / 6f); | ||
w3cLengthMapping.put("in", (float) res); | ||
// mapping according to the CSS2.2 spec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not put the latest CSS link here in addition/instead of in JEditorPane?
I agree the link to Length Units should also be added here:
// mapping according to the CSS2.2 spec | |
// Mapping according to the CSS2.2 spec | |
// https://www.w3.org/TR/CSS22/syndata.html#length-units |
Or https://www.w3.org/TR/CSS22/syndata.html#x39 which leads directly to Absolute length units.
w3cLengthMapping.put("pt", 1.3f); //1/72 of 1in | ||
w3cLengthMapping.put("px", 1f); //1/96 of 1in | ||
w3cLengthMapping.put("mm", 3.77952f); //1/10 of 1cm | ||
w3cLengthMapping.put("cm", 37.7952f); //96px/2.54 | ||
w3cLengthMapping.put("pc", 16f); //1/6 of 1in | ||
w3cLengthMapping.put("in", 96f); //96px |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest adding a space after //
.
Align the comments?
As for px, CSS defines, “1px is equal to 0.75pt”, then 1pt is 4/3px — this is where 1.3f comes from (=96/72).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copyright year in CSS.java needs updating to 2021 too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for px, CSS defines, “1px is equal to 0.75pt”, then 1pt is 4/3px — this is where 1.3f comes from (=96/72).
This makes sense.
What makes me wonder is why the length mappings for px
and pt
without W3C_LENGTH_UNITS
are exactly the opposite. In particular, it means that with W3C, 1pt > 1px
, while without W3C 1px > 1pt
.
That seems odd.
On the other hand, I don't know what the goal of those length mappings is, and they are not the sope of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for px, CSS defines, “1px is equal to 0.75pt”, then 1pt is 4/3px — this is where 1.3f comes from (=96/72).
This makes sense.
What makes me wonder is why the length mappings for
px
andpt
withoutW3C_LENGTH_UNITS
are exactly the opposite. In particular, it means that with W3C,1pt > 1px
, while without W3C1px > 1pt
.
That seems odd.On the other hand, I don't know what the goal of those length mappings is, and they are not the sope of this PR.
It's for historical reasons, I believe. JDK considers 1pt = 1px, as if the default screen resolution was 72dpi rather than 96dpi.
frame.setVisible(true); | ||
frame.setLocationRelativeTo(null); | ||
|
||
return frame.getBounds(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
frame
is redundant, htmlPane.getPreferredSize()
does the job.
float ratio = (float)w3cFrameSize.width / (float)stdFrameSize.width; | ||
System.out.println("w3cFrameSize.width/stdFrameSize.width " + ratio); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall the ratio be (float)w3cFrameSize.height / (float)stdFrameSize.height
?
You measure the font size so height seems a better candidate. However, the width does change as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with height is that the values are much smaller so they lead to higher rounding errors, and we do not achieve a 1.3 ratio. To account for that, we would need to either accept a wider range of ratios (e.g. everything between 1.15 and 1.45) or increase the font size.
I went with width for now, but I can change to height and, say, double the font size if you prefer.
String str = String.format("%.1g%n", ratio); | ||
if (str.compareTo(String.format("%.1g%n", 1.3f)) != 0) { | ||
throw new RuntimeException("HTML font size too large with high-DPI scaling and W3C_LENGTH_UNITS"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test passes for me with the fresh build of JDK, even without the fix applied.
Indeed, I get the same frame dimensions in both cases: 150×42.
However, the test fails when run with JDK 11.0.10 or JDK 15, the dimension of htmlPane
is 409×76.
The formatted string, str
, contains only the number before the decimal point. I mean for 1.3f
, the formatted string is "1"
. I'd expect the formatted string to have at least one digit after the decimal point but there's none.
Does
if ("1.3".equals(String.format("%1.1", ratio))
look clearer?
I guess the test does not fail if the system scaling (of the main monitor) is set to 100% because res
value read from Toolkit.getDefaultToolkit().getScreenResolution()
is 96/72 which is 1.3.
In my testing, sun.java2d.uiScale
has no effect on the result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose to move the test into CSS folder, and probably to create its own folder, 8231286
or 'font-size`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test passes for me with the fresh build of JDK, even without the fix applied.
Indeed, I get the same frame dimensions in both cases: 150×42.However, the test fails when run with JDK 11.0.10 or JDK 15, the dimension of
htmlPane
is 409×76.
JDK-8257664: HTMLEditorKit: Wrong CSS relative font sizes
has changed the behaviour. With the fix reverted, I get frame size of 409×76 as in previous versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test passes for me with the fresh build of JDK, even without the fix applied.
Indeed, I get the same frame dimensions in both cases: 150×42.
However, the test fails when run with JDK 11.0.10 or JDK 15, the dimension ofhtmlPane
is 409×76.JDK-8257664: HTMLEditorKit: Wrong CSS relative font sizes
has changed the behaviour. With the fix reverted, I get frame size of 409×76 as in previous versions.
I have submitted JDK-8260687: Inherited font size is smaller than expected when using StyleSheet to add styles.
I suggest declaring the font-size
property on the <body>
element in style
attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does
if ("1.3".equals(String.format("%1.1", ratio))look clearer?
I went with something similar to this now. Unfortunately, I also needed to use Locale.ENGLISH
explicitely to get a dot as the decimal separator.
I guess the test does not fail if the system scaling (of the main monitor) is set to 100% because res value read from Toolkit.getDefaultToolkit().getScreenResolution() is 96/72 which is 1.3.
In my testing, sun.java2d.uiScale has no effect on the result.
To be honest, this is unclear to me as well, I just copied what @prsadhuk had. When testing with older JDKs, I got some different dimensions, but at least now with the fix applied it doesn't seem to change anything.
I propose to move the test into CSS folder, and probably to create its own folder,
8231286
or 'font-size`.
Done, moved into ./CSS/8231286
.
I suggest declaring the
font-size
property on the<body>
element instyle
attribute.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does
if ("1.3".equals(String.format("%1.1", ratio))look clearer?
I went with something similar to this now. Unfortunately, I also needed to use
Locale.ENGLISH
explicitely to get a dot as the decimal separator.
Interesting. With Locale.ENGLISH
, it will be more robust.
I guess the test does not fail if the system scaling (of the main monitor) is set to 100% because res value read from Toolkit.getDefaultToolkit().getScreenResolution() is 96/72 which is 1.3.
In my testing, sun.java2d.uiScale has no effect on the result.To be honest, this is unclear to me as well, I just copied what @prsadhuk had. When testing with older JDKs, I got some different dimensions, but at least now with the fix applied it doesn't seem to change anything.
It is likely because sun.java2d.uiScale
gets applied to both frames. If the font size is the same, 2d surface gets scaled up to the same level, which results in the same size of the frames.
Yet the calculations of the conversion ratios in CSS depend only on the resolution of the main screen. If the main screen resolution is 96dpi (100% scaling), the font size will be the same as after your fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is likely because sun.java2d.uiScale gets applied to both frames. If the font size is the same, 2d surface gets scaled up to the same level, which results in the same size of the frames.
Yet the calculations of the conversion ratios in CSS depend only on the resolution of the main screen. If the main screen resolution is 96dpi (100% scaling), the font size will be the same as after your fix.
That makes sense.
What should we do about it? Is there a way to inject a different screen resolution for testing?
And should I remove sun.java2d.uiScale
from the doc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should we do about it? Is there a way to inject a different screen resolution for testing?
I do not know. You can change the scale factor on your main display, the Toolkit.getScreenResolution()
will return the new value.
And should I remove
sun.java2d.uiScale
from the doc?
I think so. It does not affect the test in any way.
I'm afraid we can't make 72pt be exactly 96px because of the nature of floating point calculations which are not precise. 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. |
frame.setVisible(true); | ||
frame.setLocationRelativeTo(null); | ||
|
||
return htmlPane.getPreferredSize(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant you don't need frame at all. You can remove the frame
variable.
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. |
|
This looks OK now (from a commit history point of view). As for the above Skara message:
Yes, you can ignore it. |
@mperktold 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 56 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. 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 (@aivanov-jdk, @prsadhuk) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
/issue add JDK-8260687 |
@prsadhuk Only the author (@mperktold) is allowed to issue the |
I have written automatic test for JDK-8260687: Inherited font size is smaller than expected when using StyleSheet to add styles. You can find it in my JDK-8260687 branch: We have several options:
I have no strong preference for any of the options. |
My preference will be
|
Good! The only thing is that the test file has three commits now: Then this PR would include the fixes for two issues, thus the command Optionally, issuing the following commands: |
Indeed, it does. Why will it not? The modified TestWrongCSSFontSize.java (regression test for JDK-8257664) fails when W3C_LENGTH_UNIT is enabled with the proposed fix: the font size gets bigger than it should as 150% scale gets applied twice to this fragment:
The test uses
Since this is the case, I suggest reverting the proposed fix for JDK-8260687. The modified test passes successfully in both modes when the proposed fix is reverted. |
test/jdk/javax/swing/text/html/CSS/8231286/HtmlFontSizeTest.java
Outdated
Show resolved
Hide resolved
This reverts commit ce2500c.
Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
OK I did that now. So, I take it I should't add issue JDK-8260687 then. |
No, you shouldn't. JDK-8260687 remains unresolved.
No, it was for JDK-8260687 where @prsadhuk suggested the fix and where I contributed a test. |
/integrate |
@mperktold |
/sponsor |
@aivanov-jdk @mperktold Since your change was applied there have been 74 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 48c932e. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
This PR supersedes #2223.
The original PR was created from master, whereas this PR is created from a fresh branch.
Below is a copy of the original description, but as @mrserb correctly pointed out, there is actually no relevant change from CSS 2.1 to CSS 2.2, so I
striked outthat part.Original Description
@prsadhuk asked me to take over his pull request #1628, since I filed the bug an suggested this fix.
I thought the current behavior would be buggy, but actually the units are quite precise. I checked the size of a text in font-size of 1 in, and it really was approximately 1 inch. The problem is just that browsers behave differently.
Swing follows the CSS 2.1 spec: https://www.w3.org/TR/CSS21/syndata.html#length-units. But in CSS 2.2, length units where redefined: https://www.w3.org/TR/CSS22/syndata.html#length-units. Now px is also an absolute unit, and there are constant conversion factors between all absolute units.The CSS 2.2 spec includes the following two notes regarding this change:
So the spec changed the behavior to better support high-DPI devices with existing content, and that is exactly my intention with this PR as well.
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/2256/head:pull/2256
$ git checkout pull/2256