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

TextFormatter: filter notified twice on forward/backward

    Details

      Description

      At the end is a test case (the fixtures can be added to core fx TextFieldTest more or less without change) that installs a textFormatter with a filter which tracks notifications, for this bug it simply counts its invocations. The fixtures invoke navigation methods backward/forward and some others for comparison. They test that for each navigation method, the filter is called exactly once. That's the case for all except forward/backward.

      The culprit seems to be the forward/backward methods themselves:

          public void forward() {
              // user has moved caret to the right
              final int textLength = getLength();
              final int dot = getCaretPosition();
              final int mark = getAnchor();
              if (dot != mark) {
                  int pos = Math.max(dot, mark);
                  selectRange(pos, pos);
              } else if (dot < textLength && textLength > 0) {
                  if (charIterator == null) {
                      charIterator = BreakIterator.getCharacterInstance();
                  }
                  charIterator.setText(getText());
                  int pos = charIterator.following(dot);
                  selectRange(pos, pos);
              }
              // following line is basically calling selectRange(pos, pos) again
              deselect();
          }

      don't know when the last line might be needed - but suspect that if there use-cases where it is needed it should be called only if the range has not been set in the if/else blocks, so probably having to change to logic to

      if (dot != mark) {
         doStuff();
      } else if (not-at-end-of-text) {
        soOtherStuff();
      } else { // at end of text, must deselect
        deselect();
      }

      A quick workaround for application code is to subclass TextField and override selectRange(...) to do nothing if anchor/caretPosition are the same as the newly requested. Doing so will also workaround JDK-8210297.

      public class XTextField extends TextField {
       
          public XTextField() {
              super();
          }
          
          /**
           * @param text
           */
          public XTextField(String text) {
              super(text);
          }

          /**
           * Overridden to return if anchor and caret are already at the given
           * positions.
           *
           * Removes both double notification on backward/forward and
           * notification on unrelated focusOwner changes.
           */
          @Override
          public void selectRange(int anchor, int caretPosition) {
              if (getAnchor() == anchor && getCaretPosition() == caretPosition) return;
              super.selectRange(anchor, caretPosition);
          }
          
      }
       
      This class passes all tests of core TextInputControlTest, TextFieldTest and the fixtures below.

      @RunWith(JUnit4.class)
      public class TextFieldTest {

          // replace with core variant to kick off the fx thread
          @ClassRule
          public static TestRule classRule = new JavaFXThreadingRule();

          protected TextField txtField;//Empty string
          protected TextField dummyTxtField;//With string value

          @Before
          public void setup() {
              txtField = createTextField();
              dummyTxtField = createTextField("dummy");
          }

          // to test XTextField, subclass and override these factory methods
          // to return XTextField
          protected TextField createTextField() {
              return new TextField();
          }
          
          protected TextField createTextField(String text) {
              return new TextField(text);
          }
          
          // TextFormatter notification on focus traversal
          
          /**
           * Test notification behavior on focus traversal.
           * https://bugs.openjdk.java.net/browse/JDK-8210297
           */
          @Test
          public void testFormatterNotificationOnTab() {
              TextField first = createTextField("first");
              TextField second = createTextField("second");
              TextField third = createTextField("third");
              
              List<String> notifications = new ArrayList<>();
              VBox box = new VBox(10, first, second, third);
              Stage stage = new StageLoader(box).getStage();
              first.requestFocus();
              assertEquals(first, stage.getScene().getFocusOwner());
              box.getChildren().forEach(node -> {
                  TextField field = (TextField) node;
                  UnaryOperator<TextFormatter.Change> filter = c -> {
                      notifications.add(c.getControlText());
                      return c;
                  };
                  field.setTextFormatter(new TextFormatter<>(filter));
              });
              
              second.requestFocus();
              assertEquals("sanity: seond is focuOwner", second, stage.getScene().getFocusOwner());
              assertEquals(notifications.toString(), 2, notifications.size());
          }
          
          // Textformatter notification on navigation.
          
          /**
           * Test notification on navigation: backward.
           * this bug: fails
           */
          @Test
          public void testFormatterNotificationOnBackward() {
              int pos = 3;
              assertFormatterNotificationOnNavigation(
                      t -> t.selectRange(pos, pos),
                      t -> t.backward());
          }
          
          /**
           * Test notification on navigation: forward.
           * this bug: fails
           */
          @Test
          public void testFormatterNotificationOnForward() {
              assertFormatterNotificationOnNavigation(t -> t.forward());
          }
          
          /**
           * Test notification on navigation: enOffNextWord.
            * for comparison - passes
          */
          @Test
          public void testFormatterNotificationOnForwardWord() {
              assertFormatterNotificationOnNavigation(t -> t.endOfNextWord());
          }
       
          /**
           * Test notification on navigation: home.
           * for comparison - passes
           */
          @Test
          public void testFormatterNotificationOnHome() {
              int pos = 3;
              assertFormatterNotificationOnNavigation(
                      t -> t.selectRange(pos, pos),
                      t -> t.home());
          }

          /**
           * Test that navigation produces a single notification of Textformatter.
           *
           * @param preFormatter the initial state transition before
           * setting the formatter, may be null if initial state is default
           * @param navigation the navigation
           */
          protected void assertFormatterNotificationOnNavigation(
                  Consumer<TextField> preFormatter,
                  Consumer<TextField> navigation) {
              assertFormatterNotificationOnNavigation(preFormatter, navigation, null, null);
          }
          
          /**
           * Test that navigation produces a single notification of Textformatter.
           *
           * @param navigation the navigation
           */
          protected void assertFormatterNotificationOnNavigation(Consumer<TextField> navigation) {
              assertFormatterNotificationOnNavigation(null, navigation, null, null);
          }
          
          /**
           * Test that navigation produces a single notification of textFormatter.
           * A textField to navigate on is created with text "The quick brown fox"
           * and a TextFormatter with a filter implemented to count the notifications.
           *
           * @param preFormatter the initial state transition before
           * setting the formatter, may be null if initial state is default
           * @param navigation the navigation
           * @param anchorProvider the expected anchor after navigation - sanity check, may be null.
           * @param caretProvider the expected caret after navigation - sanity check, may be null.
           */
          protected void assertFormatterNotificationOnNavigation(
                  Consumer<TextField> preFormatter,
                  Consumer <TextField> navigation,
                  Function<TextField, Integer> anchorProvider,
                  Function<TextField, Integer> caretProvider) {
              // textfield with text "The quick brown fox"
              TextField txtField = createTextField("The quick brown fox");
              // initial state - do before setting the formatter to not include in counting
              if (preFormatter != null) {
                  preFormatter.accept(txtField);
              }
              IntegerProperty count = new SimpleIntegerProperty(0);
              UnaryOperator<TextFormatter.Change> filter = c -> {
                  count.set(count.get() + 1);
                  return c;
              };
              TextFormatter<String> formatter = new TextFormatter<>(filter);
              txtField.setTextFormatter(formatter);
              // navigate
              navigation.accept(txtField);
              // sanity tests of anchor/caret pos
              // here we are not really interested in those (they are tested extensively
              // in TextInputControlTest) - just for digging if things go wrong unexpectedly
              if (anchorProvider != null) {
                  int anchor = anchorProvider.apply(txtField);
                  assertEquals("sanity: anchor at end", anchor, txtField.getAnchor());
              }
              if (caretProvider != null) {
                  int caret = caretProvider.apply(txtField);
                  assertEquals("sanity: caret at end", caret, txtField.getCaretPosition());
              }
              assertEquals("must have received a single notification", 1, count.get());
          }
      }



        Attachments

          Issue Links

            Activity

              People

              • Assignee:
                aghaisas Ajit Ghaisas
                Reporter:
                fastegal Jeanette Winzenburg
              • Votes:
                0 Vote for this issue
                Watchers:
                1 Start watching this issue

                Dates

                • Created:
                  Updated: