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

WIP: 8230231: font-family not updated in HTMLEditor #12

Open
wants to merge 1 commit into
base: master
from

Conversation

@Maxoudela
Copy link

Maxoudela commented Oct 9, 2019

Fix for javafxports/openjdk-jfx#573

Issue on JBS bug tracking : https://bugs.openjdk.java.net/browse/JDK-8230231

I tried to add a test but I do not succeed at even running the existing Web tests.. I will need some help on that side..


Progress

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

Download

$ git fetch https://git.openjdk.java.net/jfx pull/12/head:pull/12
$ git checkout pull/12

@Maxoudela Maxoudela changed the title JDK-8230231 : Adding double-quote for HTMLEditorSkin font-family 8230231 : Adding double-quote for HTMLEditorSkin font-family Oct 9, 2019
@bridgekeeper
Copy link

bridgekeeper bot commented Oct 9, 2019

👋 Welcome back shadzic! 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).

@kevinrushforth
Copy link
Member

kevinrushforth commented Oct 9, 2019

@Maxoudela please edit the title as follows:

  1. Remove the space before the : (that extra space is why jcheck failed)
  2. Change the text of the title to match the JBS bug summary exactly. You can edit the JBS bug summary if you feel it needs to be changed, but in this case, the JBS bug has a title that is more in line with our usual practice of having the bug title be descriptive of what the problem is and not what the solution happens to be.

As for unit tests, you will very likely need to add this as a system test under tests/system/src/main/test. See tests/system/src/test/java/test/javafx/scene/web/HTMLEditorTest.java. Presuming that you can add your test to that existing class, you would run it as follows:

gradle -PFULL_TEST=true :systemTests:test --tests HTMLEditorTest
@Maxoudela Maxoudela changed the title 8230231 : Adding double-quote for HTMLEditorSkin font-family 8230231: font-family not updated in HTMLEditor Oct 9, 2019
@openjdk openjdk bot added the rfr label Oct 9, 2019
@mlbridge
Copy link

mlbridge bot commented Oct 9, 2019

Webrevs

@Maxoudela
Copy link
Author

Maxoudela commented Oct 9, 2019

Thanks @kevinrushforth . I'm sorry for posting the Pull request like that, I will thoroughly read the contributing guidelines and updates my PR accordingly.

I'll try to add a test asap, thanks for the pointer.

@kevinrushforth
Copy link
Member

kevinrushforth commented Oct 9, 2019

I'm sorry for posting the Pull request like that

No problem. I mainly wanted to make sure that you knew why the RFR wasn't sent. As for the note about the title matching, the contributing guidelines don't mention that and I now realize that they should -- I'll add that along with some other improvements I'll be making.

I'll try to add a test asap, thanks for the pointer.

Great, thanks.

@Maxoudela Maxoudela changed the title 8230231: font-family not updated in HTMLEditor WIP: 8230231: font-family not updated in HTMLEditor Oct 10, 2019
@openjdk openjdk bot removed the rfr label Oct 10, 2019
@Maxoudela
Copy link
Author

Maxoudela commented Oct 10, 2019

Hum I do not succeed in running the existing test either. Here is my log. Apparently, the tests are failing because the WebView is null and not initialized. Do you have any clue of what I've been doing wrong?

Should I try to update my gradle version and JDK version maybe?

gradle -PFULL_TEST=true :systemTests:test --tests HTMLEditorTest
Starting a Gradle Daemon (subsequent builds will be faster)

> Configure project :
MACOSX_MIN_VERSION = 10.9
MACOSX_SDK_PATH = /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk
*****************************************************************
Unsupported gradle version 4.8 in use.
Only version 5.3 is supported. Use this version at your own risk
*****************************************************************
gradle.gradleVersion: 4.8
OS_NAME: mac os x
OS_ARCH: x86_64
JAVA_HOME: /Library/Java/JavaVirtualMachines/jdk-11.0.1.jdk/Contents/Home
JDK_HOME: /Library/Java/JavaVirtualMachines/jdk-11.0.1.jdk/Contents/Home
java.runtime.version: 11.0.1+13
java version: 11.0.1
java build number: 13
jdk.runtime.version: 11.0.1+13
jdk version: 11.0.1
jdk build number: 13
minimum jdk version: 11
minimum jdk build number: 28
XCODE version: Xcode10.1-MacOSX10.14+1.0
cmake version: 3.13.3
ninja version: 1.8.2
ant version: 1.10.5
HAS_JAVAFX_MODULES: false
STUB_RUNTIME: /Library/Java/JavaVirtualMachines/jdk-11.0.1.jdk/Contents/Home
CONF: Debug
NUM_COMPILE_THREADS: 1
COMPILE_TARGETS: mac
COMPILE_FLAGS_FILES: buildSrc/mac.gradle
HUDSON_JOB_NAME: not_hudson
HUDSON_BUILD_NUMBER: 0000
PROMOTED_BUILD_NUMBER: 0
PRODUCT_NAME: OpenJFX
RELEASE_VERSION: 14
RELEASE_SUFFIX: -internal
RELEASE_VERSION_SHORT: 14-internal
RELEASE_VERSION_LONG: 14-internal+0-2019-10-10-110056
RELEASE_VERSION_PADDED: 14.0.0.0
MAVEN_VERSION: 14-internal+0-2019-10-10-110056
UPDATE_STUB_CACHE: false
Building Webkit configuration /Release/ into /Users/shadzic/jfx/modules/javafx.web/build/mac
module: project ':apps' (buildModule=NO)
module: project ':base' (buildModule=YES)
module: project ':controls' (buildModule=YES)
module: project ':fxml' (buildModule=YES)
module: project ':graphics' (buildModule=YES)
module: project ':media' (buildModule=YES)
module: project ':swing' (buildModule=YES)
module: project ':swt' (buildModule=NO)
module: project ':systemTests' (buildModule=NO)
module: project ':web' (buildModule=YES)

> Task :web:compileJava
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.

> Task :web:compileShimsJava
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.

> Task :web:compileTestJava
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.

> Task :systemTests:compileTestJava
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.

> Task :systemTests:test

test.javafx.scene.web.HTMLEditorTest > checkStyleWithCSS FAILED
    java.lang.NullPointerException
        at test.javafx.scene.web.HTMLEditorTest.lambda$checkStyleWithCSS$7(HTMLEditorTest.java:192)

test.javafx.scene.web.HTMLEditorTest > checkStyleProperty FAILED
    java.lang.NullPointerException
        at test.javafx.scene.web.HTMLEditorTest.lambda$checkStyleProperty$11(HTMLEditorTest.java:266)

3 tests completed, 2 failed, 1 skipped

> Task :systemTests:test FAILED

@kevinrushforth
Copy link
Member

kevinrushforth commented Oct 10, 2019

The test failure is almost certainly due to not having the native WebKit libraries. Two options:

  1. You can build WebKit yourself (it's fairly painless on Mac) by running gradle with -PCOMPILE_WEBKIT=true
  2. You can download libjfxwebkit.dylib from the openjfx14+1 EA build and put it in ../caches/modular-sdk/modules_libs/javafx.web/libjfxwebkit.dylib
@Maxoudela
Copy link
Author

Maxoudela commented Oct 10, 2019

Allright, I've been able to build the WebKit.

I am now trying to add my test. I took some snippets from MiscellaneousTest and add my method to HTMLEditorTest:


/**
     * @test
     * @bug 8230231
     * FIXME
     */
    @Test
    public void checkFontFamily() throws Exception {
        final FontFaceTestHelper fontFaceHelper = new FontFaceTestHelper("src/test/resources/test/javafx/scene/web/Ahem.ttf");


        final CountDownLatch editorStateLatch = new CountDownLatch(1);
        final AtomicReference<String> result = new AtomicReference<>();
        Util.runAndWait(() -> {
            webView.getEngine().loadContent("<body>\n" +
                    "<span id='probe1' style='font-size: 100px;'>Toto</span> Tutu\n" +
                    "</body>\n", "text/html");

            final JSObject window = (JSObject) webView.getEngine().executeScript("window");
            assertNotNull(window);
            assertEquals("undefined", window.getMember("fontFaceHelper"));
            window.setMember("fontFaceHelper", fontFaceHelper);
            assertTrue(window.getMember("fontFaceHelper") instanceof FontFaceTestHelper);
            // Create font-face object from byte[]
            webView.getEngine().executeScript(
                    "var byteArray = new Uint8Array(fontFaceHelper.ttfFileContent);\n" +
                            "var arrayBuffer = byteArray.buffer;\n" +
                            "window.fontFace1 = new FontFace('WebFont test', arrayBuffer, {});"
            );
            webView.getEngine().executeScript("document.execCommand('selectAll', false, 'true');");
            assertEquals("loaded", webView.getEngine().executeScript("fontFace1.status"));
            // Add font-face to Document, so that it could be usable on styles
            //webView.getEngine().executeScript(
            //        "document.fonts.add(fontFace1);\n" +
            //                "document.getElementById('probe1').style.fontFamily = 'WebFont test';\n"
            //);
//            webView.getEngine().getLoadWorker().stateProperty().
//                    addListener((observable, oldValue, newValue) -> {
//                        if (newValue == SUCCEEDED) {
//                            htmlEditor.requestFocus();
//                        }
//                    });

            assertNotNull(fontFamilyComboBox);
            assertFalse(fontFamilyComboBox.getItems().isEmpty());
            String theChosenOne = null;
            for(String fontFamily: fontFamilyComboBox.getItems()){
                if(fontFamily.contains(" ")){
                    theChosenOne = fontFamily;
                    break;
                }
            }
            assertNotNull(theChosenOne);
            //Select the font
            fontFamilyComboBox.getSelectionModel().select(theChosenOne);
            assertEquals(theChosenOne, fontFamilyComboBox.getSelectionModel().getSelectedItem());

            webView.getEngine().executeScript("document.execCommand('selectAll', false, 'true');");

            assertEquals(theChosenOne, fontFamilyComboBox.getSelectionModel().getSelectedItem());



            editorStateLatch.countDown();
        });

        assertTrue("Timeout when waiting for focus change ", Util.await(editorStateLatch));
    }

where fontFamilyComboBox is retrieved like that :

fontFamilyComboBox = (ComboBox<String>) htmlEditor.lookup(".font-menu-button");
            assertNotNull(fontFamilyComboBox);

This idea is to add a Font family with a space in it, then apply it on the text. But I'm struggling to select a text portion, apply the font, then select elsewhere, and select back the text to demonstrate that the ComboBox is not updated. I tried to trigger some selectAll command but it's not working.

If anyone has some ideas, I'm all ears.

@arun-Joseph
Copy link
Contributor

arun-Joseph commented Nov 5, 2019

This bug still exists when the caret is placed on a text with the default font ("").

@arun-Joseph
Copy link
Contributor

arun-Joseph commented Nov 5, 2019

Applying this patch creates a new bug: Selecting text with multiple fonts in HTMLEditor sets the text to a single font.

Steps to reproduce:
Run the same sample program.
Type "Hello world".
Set "Hello" to FontA and "world" to FontB
Select all (or right to left) using keyboard

Expectation: "Hello world" is selected
Observation: "Hello world" is selected and font changed to FontA

@kevinrushforth
Copy link
Member

kevinrushforth commented Feb 12, 2020

@Maxoudela are you interesting in pursuing this?

@Maxoudela
Copy link
Author

Maxoudela commented Feb 12, 2020

I have indeed let this bug on the side. I would like to investigate but my time is limited these days. Especially that my patch is apparently creating another issue. I will try to give it another shot in the upcoming week.
But if anyone has some ideas or is willing to carry the issue, feel free

@kevinrushforth
Copy link
Member

kevinrushforth commented Feb 12, 2020

No hurry. I was just going through PRs that hadn't been updated in a while.

@arun-Joseph
Copy link
Contributor

arun-Joseph commented Apr 7, 2020

The second issue regarding selecting texts with multiple fonts will be fixed when #155 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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