RFR: 8207759: VK_ENTER not consumed by TextField when default Button exists [v3]

[Jeanette Winzenburg]

On Fri, 8 Nov 2024 15:31:04 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/TextFieldBehavior.java line 190:
>> 
>>> 188:         if (onAction != null || actionEvent.isConsumed()) {
>>> 189:             event.consume();
>>> 190:         }
>> 
>> @kleopatra @kevinrushforth @aghaisas 
>> 
>> Shouldn't this be an `&&` ?  Now having an empty `setOnAction` that doesn't consume anything (but just logs for example) will affect the operation of this control.
>
> correct, it's a bug.
> #1523

hmm .. it's been a while but you might be on to something. 

Let's recap:

- the first issue to get fixed was JDK-8207774: the problem was that the isConsumed always was false (due to creating the actionEvent for passing around was done by the constructor that led to immediate copying, setting consumed to false)
- the next issue was this: the problem was that actively refiring the keyEvent on parent led to a nested eventDispatch, confusing every collaborator. That was fixed by removing the refire altogether and consuming the keyEvent if handled by the textField's action/Listeners (that's the plainly reverted logic above)
- both before and after the fixes the state "handled" can be reached by the mere existence of an onAction handler on the textField, consuming or not

And that's still wrong, as you correctly - IMO - noted. The expectation formulated as a test (c&p and adjusted from TextFieldTest)

    /**
     * ENTER must be forwarded if action
     * not consumed the action.
     *
     * Here we test that key handlers on parent are notified.
     */
    @Test
    public void testEnterWithNotConsumingActionParentHandler() {
        initStage();
        root.getChildren().add(txtField);
        txtField.setOnAction(e -> {}); // do nothing handler
        List<KeyEvent> keys = new ArrayList<>();

        root.addEventHandler(KeyEvent.KEY_PRESSED, e -> {
            keys.add(e);
        });
        stage.show();
        KeyEventFirer keyboard = new KeyEventFirer(txtField);
        keyboard.doKeyPress(ENTER);
        assertEquals("key handler must be notified", 1, keys.size());
    }

We can't reach that by replacing the || by && (would introduce a regression, as failing tests indicate) but .. maybe by entirely removing the not/null check of onAction: the fired actionEvent will be passed to the singleton handler (along with any added via addXX) anyway, so it doesn't really make sense (with the first issue fixed)

Now the method looks like and all tests including the new one above (and hopefully the analogous twins of XXConsuming -> XXNotConsuming, didn't try, being lazy ;):

    @Override protected void fire(KeyEvent event) {
        TextField textField = getNode();
        EventHandler<ActionEvent> onAction = textField.getOnAction();
        // use textField as target to prevent immediate copy in dispatch
        ActionEvent actionEvent = new ActionEvent(textField, textField);

        textField.commitValue();
        textField.fireEvent(actionEvent);
        if (actionEvent.isConsumed()) {
            event.consume();
        }
    }

What do you think? My code brain is a bit rusty, so might have missed something.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/15#discussion_r1834601110