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

8257664: HTMLEditorKit: Wrong CSS relative font sizes #1759

Closed
wants to merge 5 commits into from
Closed

8257664: HTMLEditorKit: Wrong CSS relative font sizes #1759

wants to merge 5 commits into from

Conversation

stanio
Copy link
Contributor

@stanio stanio commented Dec 13, 2020

Fix for JDK-8257664 – HTMLEditorKit: Wrong CSS relative font sizes.

Disclaimer: I'm the reporter of the issue and I've been advised the best chance to get it addressed is to submit a pull request against this repository. I haven't built the JDK myself, I'll need guidance if required. I have a proof-of-concept example – demonstrating the bug and a workaround available as a public gist. I have included a sample test though I don't know if it is annotated properly.

Current behavior

<style>
  h2, .h2 { font-size: 150% }
</style>

<body>

  <h2>Foo</h2>            <!-- Displayed font size: body * pow(1.5, 2) -->

  <div class=h2>Bar</div> <!-- Displayed font size: body * pow(1.5, 3) -->

  <ol class=h2>           <!-- Displayed marker font size: body * pow(1.5, 2), and -->
    <li>Baz</li>          <!-- displayed list item font size: body * pow(1.5, 4) -->
  </ol>

  <table class=h2>
    <tr>
      <td>Qux</td>        <!-- Displayed font size: body * pow(1.5, 5) -->
    </tr>
  </table>

</body>

Expected behavior

All text should be displayed with a font size of the computed <body> font-size × 1.5.


Progress

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

Issue

  • JDK-8257664: HTMLEditorKit: Wrong CSS relative font sizes

Reviewers

Download

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

@bridgekeeper bridgekeeper bot added the oca Needs verification of OCA signatory status label Dec 13, 2020
@bridgekeeper
Copy link

bridgekeeper bot commented Dec 13, 2020

Hi @stanio, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user stanio" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@openjdk
Copy link

openjdk bot commented Dec 13, 2020

@stanio 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 Dec 13, 2020
@stanio
Copy link
Contributor Author

stanio commented Dec 13, 2020

I'll need a few days to find a printer and a scanner for submitting OCA.

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 10, 2021

@stanio This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@stanio
Copy link
Contributor Author

stanio commented Jan 11, 2021

I'll try to get my OCA done this week.

@stanio
Copy link
Contributor Author

stanio commented Jan 13, 2021

I've just emailed my scanned OCA.

@stanio stanio changed the title 8257664: Fix font-size inheritance with percentage values 8257664: HTMLEditorKit: Wrong CSS relative font sizes Jan 13, 2021
@bridgekeeper bridgekeeper bot removed the oca Needs verification of OCA signatory status label Jan 13, 2021
@openjdk openjdk bot added the rfr Pull request is ready for review label Jan 13, 2021
@mlbridge
Copy link

mlbridge bot commented Jan 13, 2021

Webrevs

@robilad
Copy link

robilad commented Jan 13, 2021

Thank you. Your OCA has been processed and your account has been marked as verified.

test/jdk/javax/swing/text/html/StyleSheet/bug8257665.java Outdated Show resolved Hide resolved

/*
* @test
* @key headless
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to specify headless tag. If it's not @key headful, then it is implicitly headless

Copy link
Member

Choose a reason for hiding this comment

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

I believe the test is headful because it uses SwingUtilities.invokeAndWait even though it does not show any UI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've used SwingUtilities.invokeAndWait just to be on the safe side that "everything Swing should be executed on the EDT" but I guess I could drop it as no UI ever shown?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure that if we use invokeAndWait(), it needs to be headful..There are many test where we use invokeAndWait but they are headless. But it will be good to see what the test is doing visually so you probably can make it headful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please, see if the option I've added to save an image capture of the editor component appears sufficient for the visual inspection.

test/jdk/javax/swing/text/html/StyleSheet/bug8257665.java Outdated Show resolved Hide resolved
* property values, Cascading, and Inheritance</a>
*/
static final Object FONT_SIZE_INHERIT =
new CSS().new FontSize().parseCssValue("100%");
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to instantiate new CSS object here? Can't we reuse "css" variable which is already initialized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hadn't previously realized the ViewAttributeSet class is not static, so it should be possible to reuse the instance css. Not sure if a separate instance per style sheet is necessary, but I'll prepare a revision.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have now made it use the instance StyleSheet.css variable.

-   Renamed bug8257665.java -> TestWrongCSSFontSize.java
-   Added full copyright statement
-   Removed redundant @key headless
-   Corrected bug no. 8257665 -> 8257664
@aivanov-jdk
Copy link
Member

Providing a way to display the rendered HTML, especially when the test fails, for visual inspection could be a bonus. This feature could be activated with a parameter passed to the test. It's not a requirement, just a suggestion.

-   Correct test type/ctor name in main()
-   Add program option to save image capture of the editor
@stanio
Copy link
Contributor Author

stanio commented Jan 15, 2021

Added an option to save editor image capture to a file. Here's the current (wrong) output:

wrongCssFontSize

All text should use the same font size – here's a side-by-side (wrong-right) comparison using the workaround I've referenced in the PR description:

wrong-right

Init the "fontSizeInherit" value using the instance "StyleSheet.css".
Copy link
Member

@aivanov-jdk aivanov-jdk left a comment

Choose a reason for hiding this comment

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

Looks good to me.

The test runs successfully on headless hosts in mach5.
As for the visual inspection, I thought about creating a frame with JEditorPane but saving an image is will also work on headless systems. That is to say, saving the image is even better.

Declare fontSizeInherit() accessor private.
Copy link
Contributor

@prsadhuk prsadhuk left a comment

Choose a reason for hiding this comment

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

Looks good to me. Internal testing is also ok.
However, I have one question, html text is honouring "font-size" value(say 1.5 as per testcase) but if Display resolution in windows system setting or via -Dsun.java2d.uiScale is set to something different(say 1.25), then should it honour the font-size property (1.5) or Display resolution (1.25)?

@aivanov-jdk
Copy link
Member

Shouldn't UI scaling be applied automatically? The font size in user space should remain the same, yet the actual font size would be scaled up according to the DPI setting. That is in my understanding, CSS rule applies 150% to the base font size according to its rules; then uiScale is applied to whatever the computed font size is.

Does it make sense to run the test with different uiScale explicitly?
I ran the test on my laptop with 150% scaling, it passed successfully.

Copy link
Member

@aivanov-jdk aivanov-jdk left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@openjdk
Copy link

openjdk bot commented Jan 18, 2021

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

8257664: HTMLEditorKit: Wrong CSS relative font sizes

Reviewed-by: aivanov, psadhukhan

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 443 new commits pushed to the master branch:

  • 0b01d69: 8260005: Shenandoah: Remove unused AlwaysTrueClosure in ShenandoahConcurrentRootScanner::roots_do()
  • 0529480: 8259867: Move encoding checks into ZipCoder
  • 7c32ffe: 8258383: vmTestbase/gc/g1/unloading/tests/unloading_compilation_level[1,2,3] time out without TieredCompilation
  • 9f21bb6: 8259983: do not use uninitialized expand_ms value in G1CollectedHeap::expand_heap_after_young_collection
  • cf25383: Merge
  • f7b96d3: 8259796: timed CompletableFuture.get may swallow InterruptedException
  • a37cd5a: 8259859: Missing metaspace NMT memory tag
  • 33dcc00: 8132984: incorrect type for Reference.discovered
  • 3edf393: 8259978: PPC64 builds broken after JDK-8258004
  • 5d8861b: 8259995: Missing comma to separate years in copyright header
  • ... and 433 more: https://git.openjdk.java.net/jdk/compare/74b79c6e191d8c39da7be37d9c01ccbbbd103857...master

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 /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jan 18, 2021
@stanio
Copy link
Contributor Author

stanio commented Jan 18, 2021

The display dpi shouldn't matter. I've tried it in a couple of different monitor configurations: 1.25, 1.5, and 2.0 scaling. I've also tried it with non-dpi-aware Java 8. I've used body { font-size: 14 } which should always evaluate to Font.size=14 (no matter the w3cLengthUnits value, for example) – the other elements use relative to that base font-size.

Before I type /integrate, is the rebase smart enough to squash the fixup! commits, or should I rebase manually once for that purpose? Maybe it would squash the commits in any case? @aivanov-jdk or @prsadhuk, would you sponsor this PR?

@aivanov-jdk
Copy link
Member

Before I type /integrate, is the rebase smart enough to squash the fixup! commits, or should I rebase manually once for that purpose? Maybe it would squash the commits in any case? @aivanov-jdk or @prsadhuk, would you sponsor this PR?

Yes, your commits in the branch will be squashed into one commit in the upstream.

I am ready to sponsor, I think @prsadhuk wouldn't mind to sponsor too. However, @prsadhuk hasn't approved it yet; I'd like to wait until he does so.

Thank you @stanio for contributing the fix!

@prsadhuk
Copy link
Contributor

The display dpi shouldn't matter. I've tried it in a couple of different monitor configurations: 1.25, 1.5, and 2.0 scaling. I've also tried it with non-dpi-aware Java 8. I've used body { font-size: 14 } which should always evaluate to Font.size=14 (no matter the w3cLengthUnits value, for example) – the other elements use relative to that base font-size.

OK. Good to know dpi factor is not affecting this usecase, but speaking of w3cLengthUnits value, it seems there is another JBS issue https://bugs.openjdk.java.net/browse/JDK-8231286 where "font-size" value with this w3cLengthUnits attribute set is affected by the hidpi factor, so was wondering
if this fix will have any impact on that issue as well, but it seems it's not and need something more to be done. Anyway, this is good to go.

@stanio
Copy link
Contributor Author

stanio commented Jan 20, 2021

Thank you @prsadhuk for reviewing this. Should/could I go with /integrate now?

@prsadhuk
Copy link
Contributor

Thank you @prsadhuk for reviewing this. Should/could I go with /integrate now?

yes sure

@stanio
Copy link
Contributor Author

stanio commented Jan 20, 2021

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Jan 20, 2021
@openjdk
Copy link

openjdk bot commented Jan 20, 2021

@stanio
Your change (at version 10a6101) is now ready to be sponsored by a Committer.

@aivanov-jdk
Copy link
Member

/sponsor

@openjdk openjdk bot closed this Jan 20, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 20, 2021
@openjdk
Copy link

openjdk bot commented Jan 20, 2021

@aivanov-jdk @stanio Since your change was applied there have been 443 commits pushed to the master branch:

  • 0b01d69: 8260005: Shenandoah: Remove unused AlwaysTrueClosure in ShenandoahConcurrentRootScanner::roots_do()
  • 0529480: 8259867: Move encoding checks into ZipCoder
  • 7c32ffe: 8258383: vmTestbase/gc/g1/unloading/tests/unloading_compilation_level[1,2,3] time out without TieredCompilation
  • 9f21bb6: 8259983: do not use uninitialized expand_ms value in G1CollectedHeap::expand_heap_after_young_collection
  • cf25383: Merge
  • f7b96d3: 8259796: timed CompletableFuture.get may swallow InterruptedException
  • a37cd5a: 8259859: Missing metaspace NMT memory tag
  • 33dcc00: 8132984: incorrect type for Reference.discovered
  • 3edf393: 8259978: PPC64 builds broken after JDK-8258004
  • 5d8861b: 8259995: Missing comma to separate years in copyright header
  • ... and 433 more: https://git.openjdk.java.net/jdk/compare/74b79c6e191d8c39da7be37d9c01ccbbbd103857...master

Your commit was automatically rebased without conflicts.

Pushed as commit 70b5b31.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated swing client-libs-dev@openjdk.org
4 participants