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

8260687: Inherited font size is smaller than expected when using StyleSheet to add styles #2515

Closed
wants to merge 9 commits into from

Conversation

stanio
Copy link
Contributor

@stanio stanio commented Feb 10, 2021

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 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():

public Object getAttribute(Object key) {
if (key instanceof StyleConstants) {
Object cssKey = css.styleConstantsKeyToCSSKey
((StyleConstants)key);
if (cssKey != null) {
Object value = doGetAttribute(cssKey);
if (value instanceof CSS.CssValue) {
return ((CSS.CssValue)value).toStyleConstants
((StyleConstants)key, host);

which in turn invokes CSS.FontSize.toStyleConstants():

Object toStyleConstants(StyleConstants key, View v) {
if (v != null) {
return Integer.valueOf(getValue(v.getAttributes(), null));
}
return Integer.valueOf(getValue(null, null));

One may notice the latter loses the StyleSheet context necessary to properly resolve w3cLengthUnits from CSS.FontSize.getValue():

int getValue(AttributeSet a, StyleSheet ss) {
ss = getStyleSheet(ss);
if (index) {
// it's an index, translate from size table
return Math.round(getPointSize((int) value, ss));
}
else if (lu == null) {
return Math.round(value);
}
else {
if (lu.type == 0) {
boolean isW3CLengthUnits = (ss == null) ? false : ss.isW3CLengthUnits();
return Math.round(lu.getValue(isW3CLengthUnits));

This context is properly sent from StyleSheet.getFont(), for example:

public Font getFont(AttributeSet a) {
return css.getFont(this, a, 12, this);


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.


Progress

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

Issue

  • JDK-8260687: Inherited font size is smaller than expected when using StyleSheet to add styles

Reviewers

Contributors

  • Alexey Ivanov <aivanov@openjdk.org>

Download

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

Honor the main StyleSheet.w3cLengthUnits setting at all times.
@bridgekeeper
Copy link

bridgekeeper bot commented Feb 10, 2021

👋 Welcome back stanio! 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 openjdk bot added the rfr Pull request is ready for review label Feb 10, 2021
@openjdk
Copy link

openjdk bot commented Feb 10, 2021

@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 Feb 10, 2021
@mlbridge
Copy link

mlbridge bot commented Feb 10, 2021

Webrevs

@aivanov-jdk
Copy link
Member

Hi @stanio. Thank you for your new contribution. I have not run the build and tests for it yet. However, the suggested fix looks reasonable. I thought it was related to the way W3C_LENGTH_UNITS is handled, the behaviour changed based on that flag.

I propose that you copy the changes from my JDK-8260687 branch. There are two changes:

  1. Run TestWrongCSSFontSize in W3C_LENGTH_UNITS-enabled mode too / updated TestWrongCSSFontSize.java
  2. New test BodyInheritedFontSize.java, it consists of several commits.

You can then add another <p> to the HTML and modify the test accordingly.

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.

Fix looks ok, it seems. Please also add the testcase given in JBS, or you can add your own.

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.

The fix looks good to me too. It passes the set of the four tests related to the recent fixes:

  • javax/swing/text/html/CSS/8231286/HtmlFontSizeTest.java
  • javax/swing/text/html/CSS/4765271/bug4765271.java
  • javax/swing/text/html/StyleSheet/TestWrongCSSFontSize.java
  • javax/swing/text/html/StyleSheet/8260687/BodyInheritedFontSize.java

I've just updated BodyInheritedFontSize.java on my JDK-8260687 branch to include your test case with font-size: 100%.

-   Add <p style="font-size: 100%">...</p> element/case

    It is important to have it before the <p>...</p> one, so it could
    trigger the pre-existing problem;

-   Replace StyleConstants.getFontSize() for GlyphView.getFont().getSize()

    The former is unreliable as it doesn't explicitly send a StyleSheet
    context and depends on generally unknown state of the
    CSS.styleSheet.w3cLengthUnits the FontSize declaration has been
    instantiated from.  At the end it doesn't report the actual font
    set to the view.
@openjdk openjdk bot removed the rfr Pull request is ready for review label Feb 11, 2021
@openjdk openjdk bot added the rfr Pull request is ready for review label Feb 11, 2021
@stanio
Copy link
Contributor Author

stanio commented Feb 11, 2021

I have copied the changes from the aivanov-jdk/jdk@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);

  • Replace StyleConstants.getFontSize() for GlyphView.getFont().getSize()

    The former is unreliable as it doesn't explicitly send a StyleSheet context and depends on a generally unknown state of the CSS.styleSheet.w3cLengthUnits the FontSize declaration has been instantiated from. At the end, it doesn't report the actual font set to the view – it computes a new result that might differ from the actual.

@aivanov-jdk
Copy link
Member

I have copied the changes from the aivanov-jdk/jdk@f9e9977 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?

* Replace `StyleConstants.getFontSize()` for `GlyphView.getFont().getSize()`
  The former is unreliable as it doesn't explicitly send a `StyleSheet` context and depends on a generally unknown state of the `CSS.styleSheet.w3cLengthUnits` the `FontSize` declaration has been instantiated from.  At the end, it doesn't report the actual font set to the view – it computes a new result that might differ from the actual.

Does it not? I can't see it has much of difference: visually the views have different font sizes which is confirmed by the existing check. But the font size in all three cases is expected to be the same.

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.

I have copied the changes from the aivanov-jdk/jdk@f9e9977 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.

@stanio
Copy link
Contributor Author

stanio commented Feb 11, 2021

I do not think the order matters...

It doesn't matter just after the #1759 fix. If you try this with Java 11 or 15 you'll see the problem is present even before that fix. That's the point.

Does it not? I can't see it has much of difference: visually...

That's the point, what you see visually is what you get from GlyphView.getFont() not necessarily what you get from StyleConstants.getFontSize() after the document has been completely laid out and the styles to all views computed. The latter depends on the last CSS.styleSheet state after the last style gets evaluated. This could be seen again by trying the test by having <p style="font-size: 100%"> as the first element and Java 11, 15 – the test passes programmatically although visually it obviously failed.

@azuev-java
Copy link
Member

Shouldn't you update copyright years in the headers of edited files?

@aivanov-jdk
Copy link
Member

Shouldn't you update copyright years in the headers of edited files?

Sure! The copyright header in StyleSheet.java needs updating.

@stanio
Copy link
Contributor Author

stanio commented Feb 11, 2021

Shouldn't you update copyright years in the headers of edited files?

I don't know if I should. I'll do it if I'm told it is necessary.

@stanio
Copy link
Contributor Author

stanio commented Feb 11, 2021

Copyright (c) 1997, 2018

Should I update it like: "Copyright (c) 1997, 2021"? That is, update the second year, or should I add a new one: "Copyright (c) 1997, 2018, 2021"?

@openjdk
Copy link

openjdk bot commented Feb 12, 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:

8260687: Inherited font size is smaller than expected when using StyleSheet to add styles

Co-authored-by: Alexey Ivanov <aivanov@openjdk.org>
Reviewed-by: psadhukhan, aivanov, kizune

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

  • 3882fda: 8260414: Remove unused set_single_threaded_mode() method in task executor
  • c6eedda: 8261500: Shenandoah: reconsider region live data memory ordering
  • df0897e: 8261504: Shenandoah: reconsider ShenandoahJavaThreadsIterator::claim memory ordering
  • 745c0b9: 8261493: Shenandoah: reconsider bitmap access memory ordering
  • 4642730: 8261496: Shenandoah: reconsider pacing updates memory ordering
  • 7c93159: 8261503: Shenandoah: reconsider verifier memory ordering
  • d9744f6: 8261608: Move common CDS archive building code to archiveBuilder.cpp
  • 235da6a: 8261672: Reduce inclusion of classLoaderData.hpp
  • 95d7312: 8261585: Restore HandleArea used in Deoptimization::uncommon_trap
  • 849390a: 8260401: StackOverflowError on open WindowsPreferences
  • ... and 49 more: https://git.openjdk.java.net/jdk/compare/a7726390cc00eed9833574ca716f724bd9689952...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 (@prsadhuk, @aivanov-jdk, @azuev-java) 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 Feb 12, 2021
-  Always save image capture to the current working directory on test
   failure;
-  Save successful image capture with '-capture' program argument;
-  Always save image capture with ".png" extension;
-  Use '-w3cUnits' program argument to control the test.w3cUnits flag.
@aivanov-jdk
Copy link
Member

I'd appreciate if you add me as contributor (for the test):
/contributor add aivanov

@stanio
Copy link
Contributor Author

stanio commented Feb 12, 2021

/contributor add aivanov

@openjdk
Copy link

openjdk bot commented Feb 12, 2021

@stanio
Contributor Alexey Ivanov <aivanov@openjdk.org> successfully added.

@stanio
Copy link
Contributor Author

stanio commented Feb 15, 2021

@aivanov-jdk, please tell me when I should /integrate. I think we've resolved all comments so far, but I might be missing some.

@aivanov-jdk
Copy link
Member

@aivanov-jdk, please tell me when I should /integrate. I think we've resolved all comments so far, but I might be missing some.

I think we're good to go…
The only thing is @prsadhuk's comment for the test. You've addressed it, the test saves the image on failure. Yet he hasn't approved it yet. I propose to wait for couple of days.

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. Thanks @stanio for the contribution.

@stanio
Copy link
Contributor Author

stanio commented Feb 15, 2021

/integrate

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

openjdk bot commented Feb 15, 2021

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

@aivanov-jdk
Copy link
Member

/sponsor

@openjdk openjdk bot closed this Feb 15, 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 Feb 15, 2021
@openjdk
Copy link

openjdk bot commented Feb 15, 2021

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

  • 3882fda: 8260414: Remove unused set_single_threaded_mode() method in task executor
  • c6eedda: 8261500: Shenandoah: reconsider region live data memory ordering
  • df0897e: 8261504: Shenandoah: reconsider ShenandoahJavaThreadsIterator::claim memory ordering
  • 745c0b9: 8261493: Shenandoah: reconsider bitmap access memory ordering
  • 4642730: 8261496: Shenandoah: reconsider pacing updates memory ordering
  • 7c93159: 8261503: Shenandoah: reconsider verifier memory ordering
  • d9744f6: 8261608: Move common CDS archive building code to archiveBuilder.cpp
  • 235da6a: 8261672: Reduce inclusion of classLoaderData.hpp
  • 95d7312: 8261585: Restore HandleArea used in Deoptimization::uncommon_trap
  • 849390a: 8260401: StackOverflowError on open WindowsPreferences
  • ... and 49 more: https://git.openjdk.java.net/jdk/compare/a7726390cc00eed9833574ca716f724bd9689952...master

Your commit was automatically rebased without conflicts.

Pushed as commit 2e610f5.

💡 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