Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8268877

TextInputControlSkin: incorrect inputMethod event handler after switching skin

    XMLWordPrintable

    Details

      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)



        Attachments

          Issue Links

            Activity

              People

              Assignee:
              Unassigned Unassigned
              Reporter:
              fastegal Jeanette Winzenburg
              Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

                Dates

                Created:
                Updated: