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

[John Hendrikx]

On Fri, 8 Nov 2024 16:28:17 GMT, Kevin Rushforth <kcr at 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