Re: [Integrated] RFR: 8230492: font-family not set in HTMLEditor if font name has a number in it
Changeset: 286d1b54 Author:Arun Joseph Committer: Kevin Rushforth Date: 2019-11-06 12:43:43 + URL: https://git.openjdk.java.net/jfx/commit/286d1b54 8230492: font-family not set in HTMLEditor if font name has a number in it Reviewed-by: kcr, shadzic ! modules/javafx.web/src/main/java/javafx/scene/web/HTMLEditorSkin.java ! tests/system/src/test/java/test/javafx/scene/web/HTMLEditorTest.java = tests/system/src/test/resources/test/javafx/scene/web/WebKit_Layout_Tests_2.ttf
Re: [Rev 01] RFR: 8230492: font-family not set in HTMLEditor if font name has a number in it
On Tue, 5 Nov 2019 11:17:55 GMT, Arun Joseph wrote: > The pull request has been updated with additional changes. > > > > Added commits: > - afc7f17a: Minor formatting > > Changes: > - all: https://git.openjdk.java.net/jfx/pull/27/files > - new: https://git.openjdk.java.net/jfx/pull/27/files/5a1fbade..afc7f17a > > Webrevs: > - full: https://webrevs.openjdk.java.net/jfx/27/webrev.01 > - incr: https://webrevs.openjdk.java.net/jfx/27/webrev.00-01 > > Issue: https://bugs.openjdk.java.net/browse/JDK-8230492 > Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod > Patch: https://git.openjdk.java.net/jfx/pull/27.diff > Fetch: git fetch https://git.openjdk.java.net/jfx pull/27/head:pull/27 Approved by shadzic (Author). PR: https://git.openjdk.java.net/jfx/pull/27
Re: [Rev 01] RFR: 8230492: font-family not set in HTMLEditor if font name has a number in it
The pull request has been updated with additional changes. Added commits: - afc7f17a: Minor formatting Changes: - all: https://git.openjdk.java.net/jfx/pull/27/files - new: https://git.openjdk.java.net/jfx/pull/27/files/5a1fbade..afc7f17a Webrevs: - full: https://webrevs.openjdk.java.net/jfx/27/webrev.01 - incr: https://webrevs.openjdk.java.net/jfx/27/webrev.00-01 Issue: https://bugs.openjdk.java.net/browse/JDK-8230492 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/27.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/27/head:pull/27 PR: https://git.openjdk.java.net/jfx/pull/27
Re: [Approved] RFR: 8230492: font-family not set in HTMLEditor if font name has a number in it
On Wed, 30 Oct 2019 10:07:42 GMT, Arun Joseph wrote: > In the HTMLEditor, when positioning the caret in a text and trying to set a > font-family that has a number in it is not working. > > Issue: In > [CSSPropertyParser.cpp](https://github.com/openjdk/jfx/blob/master/modules/javafx.web/src/main/native/Source/WebCore/css/parser/CSSPropertyParser.cpp), > concatenateFamilyName() function parses only identifiers. So, when a number > is introduced in a font-name, it fails. > > Fix: Pass the font-name as a string in HTMLEditorSkin.java by adding quotes. > > A new font is added as a resource for the test. This font is same as > modules/javafx.web/src/main/native/Tools/DumpRenderTree/fonts/WebKit Layout > Tests 2.ttf > > > > Commits: > - 5a1fbade: 8230492: font-family not set in HTMLEditor if font name has a > number in it > > Changes: https://git.openjdk.java.net/jfx/pull/27/files > Webrev: https://webrevs.openjdk.java.net/jfx/27/webrev.00 > Issue: https://bugs.openjdk.java.net/browse/JDK-8230492 > Stats: 62 lines in 3 files changed: 60 ins; 0 del; 2 mod > Patch: https://git.openjdk.java.net/jfx/pull/27.diff > Fetch: git fetch https://git.openjdk.java.net/jfx pull/27/head:pull/27 The fix and new test look good to me. I can confirm that the new test fails without the fix and passes with the fix. I left one minor formatting comment that you can correct before you integrate. Unless @Maxoudela has any concerns, I think this can go in with a single reviewer (or he may wish to be the second reviewer). Approved by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/27
Re: RFR: 8230492: font-family not set in HTMLEditor if font name has a number in it
On Wed, 30 Oct 2019 10:07:42 GMT, Arun Joseph
wrote:
> In the HTMLEditor, when positioning the caret in a text and trying to set a
> font-family that has a number in it is not working.
>
> Issue: In
> [CSSPropertyParser.cpp](https://github.com/openjdk/jfx/blob/master/modules/javafx.web/src/main/native/Source/WebCore/css/parser/CSSPropertyParser.cpp),
> concatenateFamilyName() function parses only identifiers. So, when a number
> is introduced in a font-name, it fails.
>
> Fix: Pass the font-name as a string in HTMLEditorSkin.java by adding quotes.
>
> A new font is added as a resource for the test. This font is same as
> modules/javafx.web/src/main/native/Tools/DumpRenderTree/fonts/WebKit Layout
> Tests 2.ttf
>
>
>
> Commits:
> - 5a1fbade: 8230492: font-family not set in HTMLEditor if font name has a
> number in it
>
> Changes: https://git.openjdk.java.net/jfx/pull/27/files
> Webrev: https://webrevs.openjdk.java.net/jfx/27/webrev.00
> Issue: https://bugs.openjdk.java.net/browse/JDK-8230492
> Stats: 62 lines in 3 files changed: 60 ins; 0 del; 2 mod
> Patch: https://git.openjdk.java.net/jfx/pull/27.diff
> Fetch: git fetch https://git.openjdk.java.net/jfx pull/27/head:pull/27
tests/system/src/test/java/test/javafx/scene/web/HTMLEditorTest.java line 331:
> 330: ComboBox fontFamilyComboBox = null;
> 331: int i = 0;
> 332: for(Node comboBox :
> htmlEditor.lookupAll(".font-menu-button")) {
Please add a space after the `for` keyword.
PR: https://git.openjdk.java.net/jfx/pull/27
Re: RFR: 8230492: font-family not set in HTMLEditor if font name has a number in it
On Wed, 30 Oct 2019 13:27:32 GMT, Kevin Rushforth wrote: > On Wed, 30 Oct 2019 11:09:38 GMT, Hadzic Samir wrote: > >> On Wed, 30 Oct 2019 10:07:42 GMT, Arun Joseph >> wrote: >> >>> In the HTMLEditor, when positioning the caret in a text and trying to set a >>> font-family that has a number in it is not working. >>> >>> Issue: In >>> [CSSPropertyParser.cpp](https://github.com/openjdk/jfx/blob/master/modules/javafx.web/src/main/native/Source/WebCore/css/parser/CSSPropertyParser.cpp), >>> concatenateFamilyName() function parses only identifiers. So, when a >>> number is introduced in a font-name, it fails. >>> >>> Fix: Pass the font-name as a string in HTMLEditorSkin.java by adding quotes. >>> >>> A new font is added as a resource for the test. This font is same as >>> modules/javafx.web/src/main/native/Tools/DumpRenderTree/fonts/WebKit Layout >>> Tests 2.ttf >>> >>> >>> >>> Commits: >>> - 5a1fbade: 8230492: font-family not set in HTMLEditor if font name has a >>> number in it >>> >>> Changes: https://git.openjdk.java.net/jfx/pull/27/files >>> Webrev: https://webrevs.openjdk.java.net/jfx/27/webrev.00 >>> Issue: https://bugs.openjdk.java.net/browse/JDK-8230492 >>> Stats: 62 lines in 3 files changed: 60 ins; 0 del; 2 mod >>> Patch: https://git.openjdk.java.net/jfx/pull/27.diff >>> Fetch: git fetch https://git.openjdk.java.net/jfx pull/27/head:pull/27 >> >> Will this be in conflict with https://github.com/openjdk/jfx/pull/12 ? I'll >> look at your unit tests in order to implement one similar > > I would think that the two fixes are independent of one another, but it would > be a good idea to test HTMLEditor with both patches applied. I tested HTMLEditor with both patches applied, and both fixes are working fine without any conflict. PR: https://git.openjdk.java.net/jfx/pull/27
Re: RFR: 8230492: font-family not set in HTMLEditor if font name has a number in it
On Wed, 30 Oct 2019 11:09:38 GMT, Hadzic Samir wrote: > On Wed, 30 Oct 2019 10:07:42 GMT, Arun Joseph > wrote: > >> In the HTMLEditor, when positioning the caret in a text and trying to set a >> font-family that has a number in it is not working. >> >> Issue: In >> [CSSPropertyParser.cpp](https://github.com/openjdk/jfx/blob/master/modules/javafx.web/src/main/native/Source/WebCore/css/parser/CSSPropertyParser.cpp), >> concatenateFamilyName() function parses only identifiers. So, when a number >> is introduced in a font-name, it fails. >> >> Fix: Pass the font-name as a string in HTMLEditorSkin.java by adding quotes. >> >> A new font is added as a resource for the test. This font is same as >> modules/javafx.web/src/main/native/Tools/DumpRenderTree/fonts/WebKit Layout >> Tests 2.ttf >> >> >> >> Commits: >> - 5a1fbade: 8230492: font-family not set in HTMLEditor if font name has a >> number in it >> >> Changes: https://git.openjdk.java.net/jfx/pull/27/files >> Webrev: https://webrevs.openjdk.java.net/jfx/27/webrev.00 >> Issue: https://bugs.openjdk.java.net/browse/JDK-8230492 >> Stats: 62 lines in 3 files changed: 60 ins; 0 del; 2 mod >> Patch: https://git.openjdk.java.net/jfx/pull/27.diff >> Fetch: git fetch https://git.openjdk.java.net/jfx pull/27/head:pull/27 > > Will this be in conflict with https://github.com/openjdk/jfx/pull/12 ? I'll > look at your unit tests in order to implement one similar I would think that the two fixes are independent of one another, but it would be a good idea to test HTMLEditor with both patches applied. PR: https://git.openjdk.java.net/jfx/pull/27
Re: RFR: 8230492: font-family not set in HTMLEditor if font name has a number in it
On Wed, 30 Oct 2019 10:07:42 GMT, Arun Joseph wrote: > In the HTMLEditor, when positioning the caret in a text and trying to set a > font-family that has a number in it is not working. > > Issue: In > [CSSPropertyParser.cpp](https://github.com/openjdk/jfx/blob/master/modules/javafx.web/src/main/native/Source/WebCore/css/parser/CSSPropertyParser.cpp), > concatenateFamilyName() function parses only identifiers. So, when a number > is introduced in a font-name, it fails. > > Fix: Pass the font-name as a string in HTMLEditorSkin.java by adding quotes. > > A new font is added as a resource for the test. This font is same as > modules/javafx.web/src/main/native/Tools/DumpRenderTree/fonts/WebKit Layout > Tests 2.ttf > > > > Commits: > - 5a1fbade: 8230492: font-family not set in HTMLEditor if font name has a > number in it > > Changes: https://git.openjdk.java.net/jfx/pull/27/files > Webrev: https://webrevs.openjdk.java.net/jfx/27/webrev.00 > Issue: https://bugs.openjdk.java.net/browse/JDK-8230492 > Stats: 62 lines in 3 files changed: 60 ins; 0 del; 2 mod > Patch: https://git.openjdk.java.net/jfx/pull/27.diff > Fetch: git fetch https://git.openjdk.java.net/jfx pull/27/head:pull/27 Will this be in conflict with https://github.com/openjdk/jfx/pull/12 ? I'll look at your unit tests in order to implement one similar PR: https://git.openjdk.java.net/jfx/pull/27
RFR: 8230492: font-family not set in HTMLEditor if font name has a number in it
In the HTMLEditor, when positioning the caret in a text and trying to set a font-family that has a number in it is not working. Issue: In [CSSPropertyParser.cpp](https://github.com/openjdk/jfx/blob/master/modules/javafx.web/src/main/native/Source/WebCore/css/parser/CSSPropertyParser.cpp), concatenateFamilyName() function parses only identifiers. So, when a number is introduced in a font-name, it fails. Fix: Pass the font-name as a string in HTMLEditorSkin.java by adding quotes. A new font is added as a resource for the test. This font is same as modules/javafx.web/src/main/native/Tools/DumpRenderTree/fonts/WebKit Layout Tests 2.ttf Commits: - 5a1fbade: 8230492: font-family not set in HTMLEditor if font name has a number in it Changes: https://git.openjdk.java.net/jfx/pull/27/files Webrev: https://webrevs.openjdk.java.net/jfx/27/webrev.00 Issue: https://bugs.openjdk.java.net/browse/JDK-8230492 Stats: 62 lines in 3 files changed: 60 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/27.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/27/head:pull/27 PR: https://git.openjdk.java.net/jfx/pull/27
