Skip to content
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_UNIT_LENGTHS #2223

Closed
wants to merge 7 commits into from

Conversation

mperktold
Copy link
Contributor

@mperktold mperktold commented Jan 25, 2021

@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:

Note that if the anchor unit is the pixel unit, the physical units might not match their physical measurements. Alternatively if the anchor unit is a physical unit, the pixel unit might not map to a whole number of device pixels.

Note that this definition of the pixel unit and the physical units differs from previous versions of CSS. In particular, in previous versions of CSS the pixel unit and the physical units were not related by a fixed ratio: the physical units were always tied to their physical measurements while the pixel unit would vary to most closely match the reference pixel. (This change was made because too much existing content relies on the assumption of 96dpi, and breaking that assumption breaks the content.)

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

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Integration blocker

 ⚠️ Title mismatch between PR and JBS for issue JDK-8231286

Issue

  • JDK-8231286: HTML font size too large with high-DPI scaling and W3C_LENGTH_UNITS ⚠️ Title mismatch between PR and JBS.

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2223/head:pull/2223
$ git checkout pull/2223

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 25, 2021

👋 Welcome back mperktold! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Jan 25, 2021

⚠️ @mperktold a branch with the same name as the source branch for this pull request (master) is present in the target repository. If you eventually integrate this pull request then the branch master in your personal fork will diverge once you sync your personal fork with the upstream repository.

To avoid this situation, create a new branch for your changes and reset the master branch. You can do this by running the following commands in a local repository for your personal fork. Note: you do not have to name the new branch NEW-BRANCH-NAME.

$ git checkout -b NEW-BRANCH-NAME
$ git branch -f master 1e03ca13cceb2431552ef475e48fb990460cdcf2
$ git push -f origin master

Then proceed to create a new pull request with NEW-BRANCH-NAME as the source branch and close this one.

@openjdk
Copy link

openjdk bot commented Jan 25, 2021

@mperktold The following label will be automatically applied to this pull request:

  • swing

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.

@openjdk openjdk bot added the swing client-libs-dev@openjdk.org label Jan 25, 2021
@openjdk openjdk bot added the rfr Pull request is ready for review label Jan 25, 2021
@mlbridge
Copy link

mlbridge bot commented Jan 25, 2021

Webrevs

* <a href="http://www.w3.org/TR/CSS21/syndata.html#length-units">
* <a href="https://www.w3.org/TR/CSS22/syndata.html#length-units">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this change require a CSR?

import javax.swing.text.html.StyleSheet;

public class HtmlFontSizeTest {
static Rectangle w3cFrameSize, stdFrameSize;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These fields are accessed from two different threads. Shall they be declared as volatile then?
Declaring each field on its own line would follow the Java Code Conventions.

test/jdk/javax/swing/text/html/HtmlFontSizeTest.java Outdated Show resolved Hide resolved
test/jdk/javax/swing/text/html/HtmlFontSizeTest.java Outdated Show resolved Hide resolved
test/jdk/javax/swing/text/html/HtmlFontSizeTest.java Outdated Show resolved Hide resolved
@aivanov-jdk
Copy link
Member

The comment in the bug description says, the subject contains W3C_UNIT_LENGTHS instead of W3C_LENGTH_UNITS. Shall we edit the bug subject so that it contains the correct constant name?

@mrserb
Copy link
Member

mrserb commented Jan 25, 2021

As of now, I did not check the change in the spec, just noted that the quoted text in the PR is the same for both links: CSS 2.1 and CSS 2.2.

@aivanov-jdk
Copy link
Member

As of now, I did not check the change in the spec, just noted that the quoted text in the PR is the same for both links: CSS 2.1 and CSS 2.2.

The text remains the same but the external link changes from CSS 2.1 to 2.2.
It probably does not change the spec; just updates the link to the more recent spec which Swing will follow after the fix.

@mrserb
Copy link
Member

mrserb commented Jan 26, 2021

The text remains the same but the external link changes from CSS 2.1 to 2.2.
It probably does not change the spec; just updates the link to the more recent spec which Swing will follow after the fix.

This is what I mean, the quoted text of the CSS spec is the same for both versions.

@mperktold
Copy link
Contributor Author

This is what I mean, the quoted text of the CSS spec is the same for both versions.

Oh right I misread that, I really thought it changed, but it didn't.
In that case I can also leave the link as it was, should I revert that change?

@aivanov-jdk
Copy link
Member

This is what I mean, the quoted text of the CSS spec is the same for both versions.

Oh right I misread that, I really thought it changed, but it didn't.
In that case I can also leave the link as it was, should I revert that change?

I believe we're talking about javadoc spec: its text has not changed.
CSS spec is updated. Since you follow the rules of CSS 2.2, it's better to link to CSS 2.2.

So, the changes are good as they are. Do I get right, @mrserb?

@mperktold
Copy link
Contributor Author

⚠️ @mperktold a branch with the same name as the source branch for this pull request (master) is present in the target repository. If you eventually integrate this pull request then the branch master in your personal fork will diverge once you sync your personal fork with the upstream repository.

To avoid this situation, create a new branch for your changes and reset the master branch. You can do this by running the following commands in a local repository for your personal fork. [...]

I am not sure whether I should still do this or not, since there is already some feedback, and I don't know what happens to that.
In particular, I didn't understand if this is just a suggestion or actually required.

@aivanov-jdk if it's not required, do you have any preferences?

@aivanov-jdk
Copy link
Member

⚠️ @mperktold a branch with the same name as the source branch for this pull request (master) is present in the target repository. If you eventually integrate this pull request then the branch master in your personal fork will diverge once you sync your personal fork with the upstream repository.
To avoid this situation, create a new branch for your changes and reset the master branch. You can do this by running the following commands in a local repository for your personal fork. [...]

I am not sure whether I should still do this or not, since there is already some feedback, and I don't know what happens to that.
In particular, I didn't understand if this is just a suggestion or actually required.

@aivanov-jdk if it's not required, do you have any preferences?

I see you opened the PR from master branch in your personal fork rather than a new branch.

Yes, I think you should go on as suggested: create a new PR from a new branch.

You can create the new PR with comments addressed and link to the old PR in the description.

In addition to that, if I get it right, you, @mperktold, have taken over this issue from @prsadhuk, thus #1628 is superseded by this PR and should be closed without integration. Eventually, this PR will be superseded by a new PR that you'll create.

Can you edit the subject of the JBS issue to amend the constant W3C_LENGTH_UNITS?

@mperktold
Copy link
Contributor Author

I see you opened the PR from master branch in your personal fork rather than a new branch.

Yes, I think you should go on as suggested: create a new PR from a new branch.

OK, will do. If you don't mind, I have a couple of noob questions, as I'm new to the whole process:

You can create the new PR with comments addressed and link to the old PR in the description.

Is there anything special I need to do to have "comments addressed"? Should I first apply your patches, and then proceed with the new fork afterwards?

Can you edit the subject of the JBS issue to amend the constant W3C_LENGTH_UNITS?

I don't think I have the rights to do this. I don't even have an JBS account, I submitted the bug through the https://bugs.java.com/bugdatabase/. I just recently signed the OCA, is that sufficient to create a JBS account?

@aivanov-jdk
Copy link
Member

You can create the new PR with comments addressed and link to the old PR in the description.

Is there anything special I need to do to have "comments addressed"? Should I first apply your patches, and then proceed with the new fork afterwards?

Whatever you like. You can incorporate the fixes to comments in your initial commit to the new branch, or you can fix them after you create the new branch with your exiting changeset.

Can you edit the subject of the JBS issue to amend the constant W3C_LENGTH_UNITS?

I don't think I have the rights to do this. I don't even have an JBS account, I submitted the bug through the https://bugs.java.com/bugdatabase/. I just recently signed the OCA, is that sufficient to create a JBS account?

No, it is not sufficient.
I have amended the subject of the bug.

I wondered why I didn't see OCA request in the PR. You had gotten it before you created your first PR.

@mrserb
Copy link
Member

mrserb commented Jan 26, 2021

I believe we're talking about javadoc spec: its text has not changed.
CSS spec is updated. Since you follow the rules of CSS 2.2, it's better to link to CSS 2.2.

No, I did not mean the JavaDoc, my question was - "what part of the CSS spec was updated"?

mperktold and others added 2 commits January 27, 2021 11:08
Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
@prsadhuk
Copy link
Contributor

@mperktold I guess this PR needs to be closed since this is from master branch. Please create a new PR from your fix branch with this latest changeset. The review will continue on top of that.

Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
@aivanov-jdk
Copy link
Member

I believe we're talking about javadoc spec: its text has not changed.
CSS spec is updated. Since you follow the rules of CSS 2.2, it's better to link to CSS 2.2.

No, I did not mean the JavaDoc, my question was - "what part of the CSS spec was updated"?

You're right. I looked closely at the text in CSS 2.1 and 2.2 and length definitions are the same. CSS 2.1 contains a warning that several sections have been updated. Yet this particular one seems to have remained the same.

Yet the fix is still required. Swing handles High DPI by scaling all UI therefore all length units factors should not depend on the screen resolution or print resolution, the changes in resolution are automatically handled by 2d transform.

However, changing the link to the CSS specification is optional. Shall we update it to the most recent one?

@kevinrushforth
Copy link
Member

Looks like this PR still needs to be closed.

@mperktold mperktold closed this Feb 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfr Pull request is ready for review swing client-libs-dev@openjdk.org
5 participants