On Thu, 4 Feb 2021 11:59:29 GMT, Alexey Ivanov <aiva...@openjdk.org> wrote:
>>> >>> >>> I have written automatic test for >>> [JDK-8260687](https://bugs.openjdk.java.net/browse/JDK-8260687): Inherited >>> font size is smaller than expected when using StyleSheet to add styles. You >>> can find it in my JDK-8260687 branch: >>> https://github.com/aivanov-jdk/jdk/commits/JDK-8260687 >>> There's also a commit which modifies the test from >>> [JDK-8257664](https://bugs.openjdk.java.net/browse/JDK-8257664) to run in >>> two modes with and without W3C_LENGTH_UNITS. >>> >>> We have several options: >>> >>> 1. As this PR contains the fix for JDK-8260687, you can cherry-pick the >>> test into your branch. >>> >>> 2. I can create a new JBS issue to add the test for JDK-8260687 >>> separately. >>> >>> 3. You can revert the fix in your PR, and I'll add it to the test and >>> then raise a new PR to resolve JDK-8260687. >>> >>> >>> I have no strong preference for any of the options. >> >> My preference will be >> 1) if @mperktold can pick the test from >> https://github.com/aivanov-jdk/jdk/commit/6d0b40db4400dac03567b5cab4ad45b103ea94d0 >> and add into this PR >> else 2) >> @mperktold can you also issue this comment "/issue add JDK-8260687" once >> @aivanov-jdk approves this PR. > >> My preference will be >> >> 1. if @mperktold can pick the test from >> [aivanov-jdk@6d0b40d](https://github.com/aivanov-jdk/jdk/commit/6d0b40db4400dac03567b5cab4ad45b103ea94d0) >> and add into this PR >> else 2) >> @mperktold can you also issue this comment "/issue add JDK-8260687" >> once @aivanov-jdk approves this PR. > > Good! > > The only thing is that the test file has three commits now: > aivanov-jdk@6d0b40d, aivanov-jdk@b50d650, aivanov-jdk@f9e99777 > The final version of the test file > [BodyInheritedFontSize.java](https://github.com/aivanov-jdk/jdk/blob/f9e997776fe478d99f0bbb6739ded10ec79b4caa/test/jdk/javax/swing/text/html/StyleSheet/8260687/BodyInheritedFontSize.java). > > Then this PR would include the fixes for two issues, thus the command > `/issue add JDK-8260687` > would add another resolved bug to the commit. > > Optionally, issuing the following commands: > `/contributor add psadhukhan` > `/contributor add aivanov` > will add Prasanta and myself as contributors. > > > > > 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 > > > > > > I guess so. > > Where does this `Test.java` come from? > > > 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. > > > ``` > > > > > > Does such a fix not bring back the problem with relative units for the case > > where W3C_LENGTH_UNIT is enabled? > > I guess no as this code is recently introduced by JDK-8257664 and I have > verified with this testcase and other jtreg regression tests. But if you have > a better fix, then please suggest. Indeed, it does. Why will it not? The modified [TestWrongCSSFontSize.java](https://github.com/openjdk/jdk/blob/16fdd939a4351519c8bc0ca36154443bd0986854/test/jdk/javax/swing/text/html/StyleSheet/TestWrongCSSFontSize.java) (regression test for [JDK-8257664](https://bugs.openjdk.java.net/browse/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: <h2>Foo</h2> The test uses `body { font-size: 14 }` rule in its `<style>`. But it fails the same way if this is replaced with the correct units: editor.setText(TEXT.replace("$FONT_SIZE", w3cUnits ? "14px" : "14pt")); Since this is the case, I suggest reverting the [proposed fix](https://github.com/openjdk/jdk/pull/2256/commits/ce2500ced9cf4007599972590e4924bdc87cc829) for [JDK-8260687](https://bugs.openjdk.java.net/browse/JDK-8260687). The modified test passes successfully in both modes when the proposed fix is reverted. ------------- PR: https://git.openjdk.java.net/jdk/pull/2256
