JDK-8160837 : WebEngine doesn't handle html5 color picker
  • Type: Bug
  • Component: javafx
  • Sub-Component: web
  • Affected Version: 8u92,9
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2016-07-05
  • Updated: 2017-09-07
  • Resolved: 2016-07-22
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 8 JDK 9
8u112Fixed 9 masterFixed
Related Reports
Blocks :  
JDK-8161449 - Enhance CustomColorDialog to have flexibility to hide 'Opacity', 'Use' and 'Save' Button
Relates :  
JDK-8161449 - Enhance CustomColorDialog to have flexibility to hide 'Opacity', 'Use' and 'Save' Button
Relates :  
JDK-8167100 - Minor source diffs introduced in backports of JDK-8160837 and JDK-8163582
Description
This is a Sustaining managed bug. Further details will be added later.

Steps to reproduce : (Any of below) 
1. use hello.HelloWebView , load any search engine + search for "html5 input = color" (Preferably from http://www.w3schools.com/html/tryit.asp?filename=tryhtml_input_color) 
2. use attached HelloWebColorChooser and launch
Comments
+1 Approved to backport to 8u-dev for 8u112.
22-07-2016
Updated webrev for 8u-dev : http://cr.openjdk.java.net/~ghb/8160837/webrev.06/ Tested on Ubuntu and windows, didn't see crash (Compared to earlier webrev.05).
22-07-2016
And I just verified this with my testing. This must be fixed by removing the WebPage argument from that method.
22-07-2016
The signature for the following method is wrong: + private void fwkShowColorChooser(WebPage webPage, int r, int g, int b) { It is different from the 9 main fix and differs from the signature in the .cpp code: + PG_GetColorChooserClass(env), + "fwkShowColorChooser", + "(III)V"); This means that if reattachColorChooser is ever called it will crash with a NULL pointer access.
22-07-2016
Webrev for 8u-dev : http://cr.openjdk.java.net/~ghb/8160837/webrev.05/
22-07-2016
Changeset: c82cdfad0fc8 Author: ghb Date: 2016-07-22 06:33 +0530 URL: http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/c82cdfad0fc8
22-07-2016
Please post the webrev for the 8u backport and then I will do a quick sanity check and "+1" it.
21-07-2016
Answering my own question above, the backport to 8u-dev will require a slight modification in the diffs for CustomColorDialog.java : 1: the flie moved between 8u and 9, so you will need to adjust the path to the file in the patch (it is in the "skin" subdir in 8u). 2: The location method is different; rather than calling Properties.getColorPickerString you just need to call getString (which is a statically imported method from ColorPickerSkin)
21-07-2016
+1
21-07-2016
The change in #1 is fine to me, +1.
21-07-2016
For #1 please make sure Jonathan is happy with it, but it looks like a cleaner solution than what you had in version .03 and avoids the need for the additional qualified export. The rest looks good. I will perform a quick test and then finish the review. Also, please indicate whether the patch applies cleanly to 8u-dev or whether it needs modification.
21-07-2016
Updated webrev : http://cr.openjdk.java.net/~ghb/8160837/webrev.04/ 1. "com.sun.javafx.scene.control.skin.resources.ControlResources" is used to get localized string from "OK". Modified the helper method to read the localized string in "CustomColorDialog" from setSaveBtnText(String saveBtnText) to setSaveBtnToOk(). 2. Removed the assert 3. call to 'setSelectedColor' can occur from DOM manipulation (Called from javascript). Followed the WebColorPicker. Call back from Java Context will call 'setSelectedColor' 4. Done
21-07-2016
Here are my comments on the .03 webrev. Overall, this looks good and I tested it (in legacy mode) and it runs fine. modules/web/src/main/java/com/sun/webkit/ColorChooser.java 1. New dependency: 29 import com.sun.javafx.scene.control.skin.resources.ControlResources; This adds a dependency on a new internal package from the javafx.controls module, which will cause an IllegalAccessException when run in Jigsaw mode: java.lang.IllegalAccessError: class com.sun.webkit.ColorChooser (in module javafx.web) cannot access class com.sun.javafx.scene.control.skin.resources.ControlResources (in module javafx.controls) because module javafx.controls does not export com.sun.javafx.scene.control.skin.resources to module javafx.web You must add the needed qualified export in module-info.java in the controls module. 2. Assert statement: 56 assert (client != null); We don't normally leave asserts in our code. In this case, it seems not really useful anyway. modules/web/src/main/native/Source/WebCore/platform/java/ColorChooserJava.cpp 3. Question: 59 void ColorChooserJava::setSelectedColor(const Color&) 60 { 61 notImplemented(); 62 } should it be? 4. Code convention: 91 if (cc->getClient()) 92 cc->getClient()->didChooseColor(Color(r, g, b)); Since this is a Java port platform-specific file, I recommend surrounding this with curly braces. The only serious problem is the missing qualified export for com.sun.javafx.scene.control.skin.resources
20-07-2016
Alternatively, if the backport of JDK-8161449 turns out to be not straight-forward, then the JDK 8u backport of this bug will have to be adjusted to not call the new methods (and thus the 8u version of html5 color picker would have the unwanted "Use" button and "Opacity" slider).
20-07-2016
Since this makes use of the new internal methods added for JDK-8161449 I have updated the link indicating that this bug is blocked by that one. In particular, it means that the fix for this bug cannot be pushed until the backport for JDK-8161449 is approved and pushed to 8u-dev.
20-07-2016
Updated webrev : http://cr.openjdk.java.net/~ghb/8160837/webrev.03/ Some of the review comments which didn't included : 1. "Is it possible to implement this function similar like WebKit2? Refer WebColorChooser.cpp". CustomColorChooser is designed to support ColorChooser (which is comobo box based), Limitation is "Use" button exposed by CustomColorChooser hides its visibility. 2. Unit test and test case similar JDK-8151459 (Memory leak test) is little tricky Dialogue uses new stage and callback / notification of when current webview stage is received is not known. a. Getting new stage and its window (dialog) b. sending a mouse event c. sending button click event all the above 3 can't be done unless we have exposed observer from CustomColorChooser. 3. "All browsers shows color dialogue in the middle of the application window, ours is shown at the right corner." Same ans as point 1. Need enhancement on Both web (native) as well as CustomColorChooser(Control). Except chrome Safari on OS X doesn't support this feature and Firefox on all platform Show color picker in Right top of the screen.
20-07-2016
+ if (pdata != this.pdata && colorChooserDialog != null) { + if (colorChooserDialog.isVisible()) + colorChooserDialog.hide(); + colorChooserDialog = null; + } If there is no valid case for above condition, better avoid handling it. + WebPageClient<WebView> client = webPage.getPageClient(); + assert (client != null); + if (colorChooserDialog == null) { + colorChooserDialog = new CustomColorDialog(client.getContainer().getScene().getWindow()); + colorChooserDialog.setSaveBtnText(ControlResources.getString("Dialog.ok.button")); + colorChooserDialog.setShowUseBtn(false); + colorChooserDialog.setShowOpacitySlider(false); + + colorChooserDialog.setOnSave(() -> { + log.fine("setOnSave()"); + twkHandleColorSelected(pdata, + (int) Math.round(colorChooserDialog.getCustomColor().getRed() * 255.0), + (int) Math.round(colorChooserDialog.getCustomColor().getGreen() * 255.0), + (int) Math.round(colorChooserDialog.getCustomColor().getBlue() * 255.0)); + }); + } Better move the above piece code into Constructor. + new Color( r / 255.0, g / 255.0, b / 255.0, 1.0) Remove extra white space after ( + log.fine(String.format("fwkShow() RGBA (%d, %d, %d)", r, g, b)); Change RGBA to Color or RGB. + private void showDialog(WebPage webPage, Color color) { Rename it to reattachColorChooser. It will match exactly with the WebCore's +void ColorChooserJava::setSelectedColor(const Color&) Is it possible to implement this function similar like WebKit2? Refer WebColorChooser.cpp +JNIEXPORT void JNICALL Java_com_sun_webkit_ColorChooser_twkHandleColorSelected Rename to twkDidChooseColor. (It has similar name like ColorChooserClient) + if (cc->getClient() != nullptr) if (cc->getClient())
19-07-2016
Updated webrev : http://cr.openjdk.java.net/~ghb/8160837/webrev.02/ Except "All browsers shows color dialogue in the middle of the application window, ours is shown at the right corner." Other review comments are incorporated. Tested on Windows 7, Ubuntu 14.04 and OS X.
18-07-2016
All browsers shows color dialogue in the middle of the application window, ours is shown at the right corner.
15-07-2016
lgtm with few styling nits: +namespace WebCore { + + class Color; + class ColorChooserClient; + Don't indent code inside namespace. Refer some WebKit headers like Document.h. Also remove Color forward declaration. It is not necessary because ColorChooser has already declaring it. + show(new ColorChooserData(webPage, new Color((double) r / 255.0, + (double) g / 255.0, (double) b / 255.0, (double) a / 255.0), pdata)); + } Remove typecasting to double, it will automatically raise the type to double since you are dividing by a double number.
15-07-2016
I have only reviewed the change to the one controls file - and for that I am fine with it: +1
14-07-2016
updated webrev with review comments in-corporate : http://cr.openjdk.java.net/~ghb/8160837/webrev.01/ ShowColorChooser - to - ColorChooserData Currently we have WebEvents for 'Resized', 'Status changed', 'visitiblity changed' and 'alert' there are all one way calls / notifiers (From Native --> JavaFX). Either we can provide the dialog (With current implementation) or give end user an event and let them have their own color picker dialog handled in their application.
14-07-2016
Can we expose WebEngine.setOnColorChooser event handler similar to WebEngine.setOnAlert? So that the application developer can choose how to display the color chooser based on their application look and feel.?
14-07-2016
modules/web/src/main/native/Source/WebCore/platform/java/ColorChooserJava.h: + void cancel(); + virtual ~ColorChooserJava(); + virtual void reattachColorChooser(const Color&); + virtual void setSelectedColor(const Color&); + virtual void endChooser(); Remove virtual keyword and add override for all overridden methods. modules/web/src/main/native/Source/WebCore/platform/java/ColorChooserJava.cpp: +#include "IntRect.h" Remove unused header file. +void ColorChooserJava::cancel() Isn't cancel equivalent to endChooser()..? Can we just reuse the same? +ColorChooserJava::~ColorChooserJava() +{ + cancel(); + m_webPage.clear(); + m_client = nullptr; + m_colorChooserClient.clear(); +} No need to clear all RAII objects(m_webPage, m_client , m_colorChooserClient) . It will be automatically cleared. +static jclass getJColorChooserClass() +{ + JNIEnv* env = WebCore_GetJavaEnv(); + static JGClass jColorChooserClass(env->FindClass("com/sun/webkit/ColorChooser")); + ASSERT(jColorChooserClass); + return (jclass)jColorChooserClass; +} + Move the function to JavaEnv.cpp, it is a common places for all such classes. modules/web/src/main/native/Source/WebCore/page/java/ChromeClientJava.cpp: +#include "ColorChooserClient.h" No need to include above header. + return std::make_unique<ColorChooserJava>(*new ColorChooserJava(m_webPage, initialColor, client)); Change to "return std::make_unique<ColorChooserJava>(m_webPage, initialColor, client);", no need to call copy constructor explicitly. modules/web/src/main/java/com/sun/javafx/webkit/UtilitiesImpl.java: -import com.sun.webkit.Pasteboard; -import com.sun.webkit.PopupMenu; -import com.sun.webkit.Utilities; +import com.sun.javafx.webkit.theme.ColorChooserImpl; +import com.sun.webkit.*; Don't use wildcard import, include the classes which are necessary to compile modules/web/src/main/java/com/sun/javafx/webkit/theme/ColorChooserImpl.java: + colorChooserDialog.setOnSave(new Runnable() { + ... + colorChooserDialog.setOnUse(new Runnable() { Use lamdas. + private CustomColorDialog colorChooserDialog = null; No need to initialize. By default it will be initialized to null. modules/web/src/main/java/com/sun/webkit/ColorChooser.java: + public final class ShowColorChooser { What is the need of ShowColorChooser, May be "ColorChooserData" (or just Data) shows the purpose. Otherwise remove it and move all fields to ColorChooser.
14-07-2016
Webrev : http://cr.openjdk.java.net/~ghb/8160837/webrev.00/ Platform Implementation of ColorChooser (called as Color picker in JavaFX) Used CustomColorDialog to show modal dialog when ColorChooser (native) object is created Use case : 1. Dialogue Show, 2. Dialogue Cancel N : Native , J : JavaFX 1 : a. ColorChooserJava constructor (Native) object is created (N -> J) b. ColorChooserJava.reattachColorChooser (N -> J) 2 : a. [without color selected]CustomeColorDialog "Cancel" / "Use" / Close (Top left) (J -> N) b. [color selected] CustomColorDialog "Save" (J -> N) c. Page reload (During dialog is shown and page is reloaded. (N -> J) Tested on Windows, Ubuntu and Mac (All 64 bit version) Limitation : 1. HTML5 input_type=color doesn't support opacity / alpha. Need to remove the UI option provided in CustomColorDialog. 2. "ColorPicker" is better than "CustomColorDialog". a. ColorPicker is based on "ComboBoxBase" which shows a drop down button with color pallets and an hyper link to "Custom Color". b. Problem with Closed notification from ColorPicker as selecting "Custom Color" hides the ColorPicker.
14-07-2016
Minimized content to re-produce WebEngine.loadContent("<input type='color' name='backgroundColor' value='#ff00ff'>"); Webkit feature "INPUT_TYPE_COLOR", This is currently not implemented in our port.
07-07-2016