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

8230492: font-family not set in HTMLEditor if font name has a number in it #27

Closed
wants to merge 2 commits into from

Conversation

@arun-Joseph
Copy link
Member

arun-Joseph commented Oct 30, 2019

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, 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

Progress

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

Issue

JDK-8230492: font-family not set in HTMLEditor if font name has a number in it

Approvers

  • Kevin Rushforth (kcr - Reviewer) Note! Review applies to 5a1fbad
  • Hadzic Samir (shadzic - Author)
@bridgekeeper
Copy link

bridgekeeper bot commented Oct 30, 2019

👋 Welcome back arun-Joseph! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request (refresh this page to view it).

@openjdk openjdk bot added the rfr label Oct 30, 2019
@mlbridge
Copy link

mlbridge bot commented Oct 30, 2019

Webrevs

@Maxoudela
Copy link
Collaborator

Maxoudela commented Oct 30, 2019

Will this be in conflict with #12 ? I'll look at your unit tests in order to implement one similar

@kevinrushforth
Copy link
Member

kevinrushforth commented Oct 30, 2019

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.

@arun-Joseph
Copy link
Member Author

arun-Joseph commented Nov 5, 2019

I tested HTMLEditor with both patches applied, and both fixes are working fine without any conflict.

Copy link
Member

kevinrushforth left a comment

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).

@openjdk openjdk bot removed the rfr label Nov 5, 2019
@openjdk
Copy link

openjdk bot commented Nov 5, 2019

@arun-Joseph This change can now be integrated. The commit message will be:

8230492: font-family not set in HTMLEditor if font name has a number in it

Reviewed-by: kcr, shadzic
  • If you would like to add a summary, use the /summary command.
  • To list additional contributors, use the /contributor command.

Since the source branch of this PR was last updated there have been 2 commits pushed to the master branch:

  • f74f3af: 8233040: ComboBoxPopupControl: remove eventFilter for F4
  • a1cc4ab: 8232210: Update Mesa 3-D Headers to version 19.2.1

Since there are no conflicts, your changes will automatically be rebased on top of the above commits when integrating. If you prefer to do this manually, please merge master into your branch first.

As you are not a known OpenJDK Author, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth) 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 label Nov 5, 2019
@arun-Joseph
Copy link
Member Author

arun-Joseph commented Nov 6, 2019

/integrate

@openjdk openjdk bot added the sponsor label Nov 6, 2019
@openjdk
Copy link

openjdk bot commented Nov 6, 2019

@arun-Joseph
Your change (at version afc7f17) is now ready to be sponsored by a Committer.

@kevinrushforth
Copy link
Member

kevinrushforth commented Nov 6, 2019

/sponsor

@openjdk openjdk bot closed this Nov 6, 2019
@openjdk openjdk bot added integrated and removed sponsor ready labels Nov 6, 2019
@openjdk
Copy link

openjdk bot commented Nov 6, 2019

@kevinrushforth @arun-Joseph The following commits have been pushed to master since your change was applied:

  • f74f3af: 8233040: ComboBoxPopupControl: remove eventFilter for F4
  • a1cc4ab: 8232210: Update Mesa 3-D Headers to version 19.2.1

Your commit was automatically rebased without conflicts.

Pushed as commit 286d1b5.

@mlbridge
Copy link

mlbridge bot commented Nov 6, 2019

Mailing list message from Kevin Rushforth kcr@openjdk.org

Changeset: 286d1b5
Author: Arun Joseph <arun.aj.joseph at oracle.com>
Committer: Kevin Rushforth
Date: 2019-11-06 12:43:43 +0000
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

@arun-Joseph arun-Joseph deleted the arun-Joseph:8230492 branch Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.