JDK-8268877 : TextInputControlSkin: incorrect inputMethod event handler after switching skin
  • Type: Bug
  • Component: javafx
  • Sub-Component: controls
  • Affected Version: openjfx16
  • Priority: P4
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2021-06-16
  • Updated: 2022-11-29
  • Resolved: 2022-11-14
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.
Other
openjfx20 b08Fixed
Related Reports
Blocks :  
Blocks :  
Relates :  
Relates :  
Relates :  
Relates :  
Description
turned up while fixing JDK-8240506, failing test:

    @Test
    public void testOnInputMethodTextChangedNPE() {
        String initialText = "some text";
        String prefix = "from input event";
        TextField field = new TextField(initialText);
        installDefaultSkin(field);
        InputMethodEvent event = new InputMethodEvent(InputMethodEvent.INPUT_METHOD_TEXT_CHANGED, 
                List.of(), prefix, 0);
        Event.fireEvent(field, event);
        assertEquals("sanity: prefix must be committed", prefix + initialText, field.getText());
        replaceSkin(field);
        Event.fireEvent(field, event);
        assertEquals(" prefix must be committed again", prefix + prefix + initialText, field.getText());
    }  
    
This throws an NPE before the fix because the handler installed by the skin _is not_ removed, and fails the assertion because the handler installed by the skin _is_ removed ;)

The problem is the installation pattern which sets its own singleton handler if there is none on the control:

        if (control.getOnInputMethodTextChanged() == null) {
            control.setOnInputMethodTextChanged(event -> {
                handleInputMethodEvent(event);
            });
        }

When replacing the skin, the new skin's constructor is run while the control is still attached to the old skin - at that time, the field's handler is not null (because set by the old skin), so the new skin doesn't replace it. In dispose, the handler is either removed (after the fix) leaving the control without handler or not removed (before the fix) leading to the NPE.

This pattern is copied to ComboBoxPopupControl to fix JDK-8096136 - in its discussion Jonathan proposed to add a handler (vs. setting the singleton), but with this here in place it was agreed to use the same everywhere. Don't know when/why it was done as it is (skins moved into public scope disrupted history)




Comments
Changeset: a13630fb Author: Andy Goryachev <angorya@openjdk.org> Committer: Kevin Rushforth <kcr@openjdk.org> Date: 2022-11-14 18:23:05 +0000 URL: https://git.openjdk.org/jfx/commit/a13630fba8854b84516a089ff67e4c0c4c01ff79
14-11-2022

A pull request was submitted for review. URL: https://git.openjdk.org/jfx/pull/903 Date: 2022-09-27 19:36:46 +0000
03-11-2022

the problem with the code you provided is 3. user-set value must be preserved after the skin is uninstalled the constructor overwrites the value set by the user. and it will fail SkinCleanupTest.testTextInputOnInputMethodTextChangedHandler : 1289 the problem is - the constructor does not know if the current value is from the user and must be kept, or from the old skin and must be overwritten.
18-08-2022

Bullet 1: _new skin must install its property, unless the value was initially set by the user_ agreed - but that's not the story of inputMethodRequests: they are set unconditionally. So it's fine to follow the general pattern, which is: a skin must remove everthing it installed and not touch anything it didn't - so in the case of inputMethodRequests // constructor myInputMethodRequests = ... control.setInputMethodRequests(myInputMethodRequests) // dispose: if it's ours, remove otherwise do nothing if (control.getInputMethodRequests() == myInputMethodRequests) { control.setInputMethodRequests(null); } I don't see any problem in this particular case.
18-08-2022

So, [~fastegal], we cannot have the cake and eat it too. We do have singleton properties (TextInputControlskin.inputMethodRequests for example). We cannot satisfy all these requirements without Skin.install: 1. new skin must install its property, unless the value was initially set by the user 2. skin must uninstall own value, if set in step 1 3. user-set value must be preserved after the skin is uninstalled Specifically, the constructor cannot decide whether to keep or overwrite the existing value for #1 because at this point, the value might come either from the user or from the old skin that is attached.
17-08-2022

event handlers: yes listeners: yes properties: NO (or at least not trivially) TextInputControlSkin:334 control.setInputMethodRequests(...) - new skin sets this property, which is then gets destroyed (set to null) when Control.setSkin() invokes dispose() on the old skin. we could try checking if the current property corresponds to our value in dispose(), I suppose, though it will leave the Skinnable in an inconsistent state if the customer creates a new skin and *discards* it for whatever reason, and result in a memory leak. I suppose that's the price we have to pay for backward compatibility.
17-08-2022

You are right, it looks like we can add listeners. The remaining question is - how do we enforce that rule in user-developed skins? Apart from adding a rule to javadoc, there is no way to block this pattern programmatically, right?
17-08-2022

hmm .. I honestly don't see the problem: actually, we _do_ forbid (== it's a bug doing so) skins to set singleton event handlers on its skinnable, that's decided in JDK-8093590 (which I unfortunately didn't see when stumbling across this). Most of those that did were fixed by replacing the singleton handler with an added handler, f.i. a conditional variant in MenuButtonSkinBase: if (control.getOnMousePressed() == null) { - control.setOnMousePressed(e -> { + control.addEventHandler(MouseEvent.MOUSE_PRESSED, e -> { getBehavior().mousePressed(e, behaveLikeButton); }); } Doing the same to the onAction singleton in MenuButtonSkin: myHandler = e -> control.show(); if (control.getOnAction() == null) { - control.setOnAction(e -> control.show()); + control.addEventHandler(ActionEvent.ACTION, myHandler); } // in dispose getSkinnable().removeEventHandler(ActionEvent.ACTION, myHandler); Why do you think we can't use this pattern (the same as applied in fixing the old JDK-8093590)? What do I miss?
17-08-2022

You do bring a good point, [~fastegal]. We won't need a new install() method if, and only if we can replace setting of singletons with add/remove. The problem is, I don't always know how - perhaps there is a solution I do not see. For example, in MenuButtonSkin:108, the code conditionally sets a singleton handler on onAction (onAction is a publicly writeable property, see ButtonBase:123). How can we replace this code with commutative operations (add/remove)? One can argue that instead of adding the install() method, we ought to forbid setting singletons - but how to enforce that? So I disagree with you - we can't fix these issues without the API changes. We have a "bad" design and we must fix it, or face the endless series of bugs.
16-08-2022

Hmm .. I would say C is the same as A: it's a singleton event handler which should not be set by a skin - doesn't matter whether it's set un/conditionally, see the fixes in JDK-8093590 (MenuButtonSkin had such a conditionally set handler, has been replaced by a conditionally added handler). The only difference I see is the event type: Action is a semantic event, all others are low-level input type events. Personally, I don't think that makes a difference, though - but naturally open to good reasons :)
16-08-2022

There is one more case: C: setting a singleton based on some condition (for instance, != null). we cannot distinguish between the user having set it from the previous skin having set it (since the previous skin is still attached)
15-08-2022

honestly don't understand what "makes no sense" ;) Application code rules, if it explicitly sets whatever - either before or during showing the node - the skin should keep its hands from that custom setting. Actually, we have two separate problems with text skins: A - setting a singleton event handler: doing so by a skin turns out to be plain wrong (see JDK-8093590). The fix to that seems to have overlooked the inputMethodTextChanged ;). Also see a code comment to ContextMenuContent which explains why setting the singleton handler (for keyPressed) on the content (no public api) The fix for this part is to follow the lead of JDK-8093590 and replace setting the singleton handler with adding one (and remove that in dispose) B - the on-the-fly setting of the inputMethodsRequests (well spotted - I overlooked it when fixing side-effects of replacing text skins ;) The fix to that part is apply the the same pattern as always in dispose: don't-replace-what-we-didn't-install. So .. I disagree with the evaluation of JDK-8290844 as blocking this: both parts can be solved without api changes
15-08-2022

[~fastegal]: Could you please refer me to the requirements that "skin must not replace handler", and also "skin dispose must not remove handler that was installed by control" in SkinCleanupTest.testTextInputOnInputMethodTextChangedWithHandler()? Perhaps if I know the source of these requirements, or the problem we are trying to solve, I could address this scenario - right now it makes no sense. I understand the general sentiment not to add new APIs, and try to work around the problem in the implementation (basically, option 3 in your last comment in JDK-8241364). In this case, I am afraid the requirements SkinCleanupTest.testTextInputOnInputMethodTextChangedWithHandler() tests for make it impossible. Specifically, when the new skin constructor tries to set its handler on the control, it does not know whether existing handler is from an old skin (in which case it's ok to overwrite the handler), or from the user's custom code, in which case it should leave it as is, at least according to my interpretation of the above mentioned test case.
18-07-2022

In TextInputControlSkin, the constructor mutates two properties in the control: onInputMethodTextChangedProperty inputMethodRequestsProperty
15-07-2022

I think we might have a design problem here. The issue is caused by the fact that the control is being mutated in the skin's constructor, at the moment when the old skin is still attached. A better design is to have explicit methods, similar to Swing's installUI(JComponent) and uninstallUI(JComponent). In javafx, we seem to have dispose() but not a corresponding install(). https://github.com/openjdk/jdk/blob/0184f46bdfe4441ea6ef28c658c6677c4c736ee9/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTextUI.java#L793 https://github.com/openjdk/jdk/blob/0184f46bdfe4441ea6ef28c658c6677c4c736ee9/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTextUI.java#L858
15-07-2022

notes, just to not forget: - TextInputControlSkin and ComboBoxPopupControl seem to be the only skins that set a singleton eventHandler on its control - adding (and removing) a handler might be an option - in swing there's in interface UIResource denoting something as installed by the LookAndFeel, when sticking to the singleton handler, might do something similar here
16-06-2021