On Fri, 8 Nov 2024 15:39:57 GMT, Jeanette Winzenburg <faste...@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.
>
> @hjohn @kevinrushforth @aghaisas
>
> 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();
> // JDK-8207774
> // use textField as target to prevent immediate copy in dispatch
> ActionEvent actionEvent = new ActionEvent(textField, textField);
>
> textField.comm...
@kleopatra Your explanation makes sense to me. Thank you for taking the time to
write it up.
If so, the proposed fix, including adjusting the comment, would be:
---
a/modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/TextFieldBehavior.java
+++
b/modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/TextFieldBehavior.java
@@ -162,9 +162,8 @@ public class TextFieldBehavior extends
TextInputControlBehavior<TextField> {
textField.commitValue();
textField.fireEvent(actionEvent);
- // fix of JDK-8207759: reverted logic
- // mapping not auto-consume and consume if handled by action
- if (onAction != null || actionEvent.isConsumed()) {
+ // Consume the original event if the copied actionEvent was consumed.
+ if (actionEvent.isConsumed()) {
event.consume();
}
}
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/15#discussion_r1834693101
