On Fri, 8 Nov 2024 15:31:04 GMT, Andy Goryachev <ango...@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
