On Fri, 8 Nov 2024 16:28:17 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> @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
>> ...
>
> @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();
> }
> }
You are right, `&&` would be insufficient as there is another way to add action
handlers. I didn't really see that, although the fix I had in mind was to
eliminate the `onAction` variable completely (it is unused in your fix as
well), so probably would have ended up at the correct result :)
I can roll this into a small PR if you wish.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/15#discussion_r1835039351
