Re: [Integrated] RFR: 8191758: Match WebKit's font weight rendering with JavaFX
On Tue, 14 Apr 2020 10:25:52 GMT, Bhawesh Choudhary wrote: > As per JavaFx 700 font weight is considered to be bold but webkit is using > 600 font weight for text to become bold. to > fix issue, use boldWeightValue() function which uses 700 font weight rather > than isFontWeightBold() which compare > against 600 font weight. This pull request has now been integrated. Changeset: 8ad58052 Author:bhawesh Committer: Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/8ad58052 Stats: 26 lines in 2 files changed: 0 ins; 25 del; 1 mod 8191758: Match WebKit's font weight rendering with JavaFX Reviewed-by: kcr, ajoseph - PR: https://git.openjdk.java.net/jfx/pull/180
Re: [Rev 03] RFR: 8191758: Match WebKit's font weight rendering with JavaFX
On Wed, 29 Apr 2020 14:01:50 GMT, Bhawesh Choudhary wrote: >> As per JavaFx 700 font weight is considered to be bold but webkit is using >> 600 font weight for text to become bold. to >> fix issue, use boldWeightValue() function which uses 700 font weight rather >> than isFontWeightBold() which compare >> against 600 font weight. > > Bhawesh Choudhary has updated the pull request incrementally with one > additional commit since the last revision: > > style format changes Marked as reviewed by ajoseph (Committer). - PR: https://git.openjdk.java.net/jfx/pull/180
Re: [Rev 03] RFR: 8191758: Match WebKit's font weight rendering with JavaFX
On Wed, 29 Apr 2020 14:01:50 GMT, Bhawesh Choudhary wrote: >> As per JavaFx 700 font weight is considered to be bold but webkit is using >> 600 font weight for text to become bold. to >> fix issue, use boldWeightValue() function which uses 700 font weight rather >> than isFontWeightBold() which compare >> against 600 font weight. > > Bhawesh Choudhary has updated the pull request incrementally with one > additional commit since the last revision: > > style format changes Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/180
Re: [Rev 03] RFR: 8191758: Match WebKit's font weight rendering with JavaFX
> As per JavaFx 700 font weight is considered to be bold but webkit is using > 600 font weight for text to become bold. to > fix issue, use boldWeightValue() function which uses 700 font weight rather > than isFontWeightBold() which compare > against 600 font weight. Bhawesh Choudhary has updated the pull request incrementally with one additional commit since the last revision: style format changes - Changes: - all: https://git.openjdk.java.net/jfx/pull/180/files - new: https://git.openjdk.java.net/jfx/pull/180/files/f6fb1075..473eec6c Webrevs: - full: https://webrevs.openjdk.java.net/jfx/180/webrev.03 - incr: https://webrevs.openjdk.java.net/jfx/180/webrev.02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/180.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/180/head:pull/180 PR: https://git.openjdk.java.net/jfx/pull/180
Re: [Rev 02] RFR: 8191758: Match WebKit's font weight rendering with JavaFX
On Tue, 21 Apr 2020 16:34:11 GMT, Bhawesh Choudhary
wrote:
>> As per JavaFx 700 font weight is considered to be bold but webkit is using
>> 600 font weight for text to become bold. to
>> fix issue, use boldWeightValue() function which uses 700 font weight rather
>> than isFontWeightBold() which compare
>> against 600 font weight.
>
> Bhawesh Choudhary has refreshed the contents of this pull request, and
> previous commits have been removed. The
> incremental views will show differences compared to the previous content of
> the PR.
The fix and test look good. I confirm that your new test fails without your fix
and passes with your fix.
I left one style comment and will approve once you fix that.
modules/javafx.web/src/test/java/test/javafx/scene/web/WebViewTest.java line
111:
> 110: );
> 111: submit(()->{
> 112: assertFalse("Font weight test failed ",
Minor: there should be a space before and after the `->`
-
PR: https://git.openjdk.java.net/jfx/pull/180
Re: RFR: 8191758: Match WebKit's font weight rendering with JavaFX
On Fri, 17 Apr 2020 18:06:06 GMT, Phil Race wrote: >> Can you add a unit test to go along with this fix? > > Per the opentype spec, 700 is bold. 600 is semi-bold > https://docs.microsoft.com/en-us/typography/opentype/spec/os2#usweightclass > > CSS agrees : https://developer.mozilla.org/en-US/docs/Web/CSS/font-weight > > So are you saying webkit has been using bold at a lower weight than these > specs suggest ? > I see the logic all comes from > Source/WebCore/platform/graphics/FontSelectionAlgorithm.h > > I suppose the existing code thinks that if we have reached what that file > calls the bold threshold of 600 then we > should use bold. It isn't necessarily "wrong" but I think I agree that it is > more important to be consistent with the > rest of Java FX ... which I believe is the point of this change ? @prrace yes, webkit is set to use 600 weight where javafx consider 700 weight for it to be consider bold. https://docs.oracle.com/javafx/2/api/javafx/scene/text/FontWeight.html - PR: https://git.openjdk.java.net/jfx/pull/180
Re: [Rev 02] RFR: 8191758: Match WebKit's font weight rendering with JavaFX
> As per JavaFx 700 font weight is considered to be bold but webkit is using > 600 font weight for text to become bold. to > fix issue, use boldWeightValue() function which uses 700 font weight rather > than isFontWeightBold() which compare > against 600 font weight. Bhawesh Choudhary has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: added unit test for jdk-8191758 - Changes: - all: https://git.openjdk.java.net/jfx/pull/180/files - new: https://git.openjdk.java.net/jfx/pull/180/files/43c7cbf1..f6fb1075 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/180/webrev.02 - incr: https://webrevs.openjdk.java.net/jfx/180/webrev.01-02 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/180.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/180/head:pull/180 PR: https://git.openjdk.java.net/jfx/pull/180
Re: [Rev 01] RFR: 8191758: Match WebKit's font weight rendering with JavaFX
> As per JavaFx 700 font weight is considered to be bold but webkit is using > 600 font weight for text to become bold. to > fix issue, use boldWeightValue() function which uses 700 font weight rather > than isFontWeightBold() which compare > against 600 font weight. Bhawesh Choudhary has updated the pull request incrementally with one additional commit since the last revision: added unit test for 8191758 - Changes: - all: https://git.openjdk.java.net/jfx/pull/180/files - new: https://git.openjdk.java.net/jfx/pull/180/files/d3d3e716..43c7cbf1 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/180/webrev.01 - incr: https://webrevs.openjdk.java.net/jfx/180/webrev.00-01 Stats: 25 lines in 1 file changed: 25 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/180.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/180/head:pull/180 PR: https://git.openjdk.java.net/jfx/pull/180
Re: RFR: 8191758: Match WebKit's font weight rendering with JavaFX
On Thu, 16 Apr 2020 12:40:24 GMT, Kevin Rushforth wrote: >> As per JavaFx 700 font weight is considered to be bold but webkit is using >> 600 font weight for text to become bold. to >> fix issue, use boldWeightValue() function which uses 700 font weight rather >> than isFontWeightBold() which compare >> against 600 font weight. > > Can you add a unit test to go along with this fix? Per the opentype spec, 700 is bold. 600 is semi-bold https://docs.microsoft.com/en-us/typography/opentype/spec/os2#usweightclass CSS agrees : https://developer.mozilla.org/en-US/docs/Web/CSS/font-weight So are you saying webkit has been using bold at a lower weight than these specs suggest ? I see the logic all comes from Source/WebCore/platform/graphics/FontSelectionAlgorithm.h I suppose the existing code thinks that if we have reached what that file calls the bold threshold of 600 then we should use bold. It isn't necessarily "wrong" but I think I agree that it is more important to be consistent with the rest of Java FX ... which I believe is the point of this change ? - PR: https://git.openjdk.java.net/jfx/pull/180
RFR: 8191758: Match WebKit's font weight rendering with JavaFX
As per JavaFx 700 font weight is considered to be bold but webkit is using 600 font weight for text to become bold. to fix issue, use boldWeightValue() function which uses 700 font weight rather than isFontWeightBold() which compare against 600 font weight. - Commit messages: - 8191758: Match WebKit's font weight rendering with JavaFX Changes: https://git.openjdk.java.net/jfx/pull/180/files Webrev: https://webrevs.openjdk.java.net/jfx/180/webrev.00 Issue: https://bugs.openjdk.java.net/browse/JDK-8191758 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/180.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/180/head:pull/180 PR: https://git.openjdk.java.net/jfx/pull/180
Re: RFR: 8191758: Match WebKit's font weight rendering with JavaFX
On Tue, 14 Apr 2020 10:25:52 GMT, Bhawesh Choudhary wrote: > As per JavaFx 700 font weight is considered to be bold but webkit is using > 600 font weight for text to become bold. to > fix issue, use boldWeightValue() function which uses 700 font weight rather > than isFontWeightBold() which compare > against 600 font weight. Can you add a unit test to go along with this fix? - PR: https://git.openjdk.java.net/jfx/pull/180
