8207759: VK_ENTER not consumed by TextField when default Button exists #15
Conversation
|
👋 Welcome back fastegal! A progress list of the required criteria for merging this PR into |
|
@aghaisas can you review this as well? |
|
This will need two reviewers. |
...x.controls/src/main/java/com/sun/javafx/scene/control/behavior/TextInputControlBehavior.java
Show resolved
Hide resolved
|
@kleopatra This change can now be integrated. The commit message will be:
Since the source branch of this PR was last updated there have been 53 commits pushed to the
Since there are no conflicts, your changes will automatically be rebased on top of the above commits when integrating. If you prefer to do this manually, please merge As you do not have Committer status in this project, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@aghaisas, @kevinrushforth) but any other Committer may sponsor as well.
|
|
Zitat von "openjdk[bot]" <[email protected]>:
@kleopatra This change can now be integrated. The commit message will be:
```
Ajit, Kevin,
hmm .. wondering if this message is a bot-error? Kevin stated that it
needs two reviewers (approving, I assume?) but currently have one only
- so question is: should I post the /integrate comment or not yet?
…-- Jeanette
8207759: VK_ENTER not consumed by TextField when default Button exists
Reviewed-by: aghaisas
```
- If you would like to add a summary, use the `/summary` command.
- To list additional contributors, use the `/contributor` command.
Since the source branch of this PR was last updated there have been
53 commits pushed to the `master` branch:
* fc539b5: 8223296:
NullPointerException in GlassScene.java at line 325
* 71ca899: 8220722:
ProgressBarSkin: adds strong listener to control's width property
* 07af89a: 8221334: TableViewSkin:
must initialize flow's cellCount in constructor
* 4ddf554: 8234916: [macos 10.15]
Garbled text running with native-image
* 46338d0: 8232524:
SynchronizedObservableMap cannot be be protected for copying/iterating
* a68347c: 8235150: IosApplication
does not pass the required object in _leaveNestedEventLoopImpl
* 1c27fbd: 8210955:
DOMTest::testEventListenerCascade fails
* 2d4096a: 8235151: Nonexistent
notifyQuit method referred from iOS GlassHelper.m
* 98035cb: 8211308: Support HTTP/2
in WebView
* 6892fa1: 8232064: Switch FX
build to use JDK 13.0.1 as boot JDK
* 1d670f1: 8200224: Multiple press
event when JFXPanel gains focus
* 83eb0a7: 8193445: JavaFX CSS is
applied redundantly leading to significant performance degradation
* 798afbc: 8230610: Upgrade
GStreamer to version 1.16.1
* 126896d: 8234704: Fix
attribution in libxslt.md
* 4d3c723: 8234593: Mark
LeakTest.testGarbageCollectability as unstable
* 5a39824: 8234056: Upgrade to
libxslt 1.1.34
* 8bea7b7: 8229472: Deprecate for
removal JavaBeanXxxPropertyBuilders constructors
* aad1720: 8233420: Upgrade to gcc
8.3 on Linux
* 42040c4: 8232063: Upgrade gradle
to version 6.0
* aab07a4: 8234239: [TEST_BUG]
Reenable few ignored web tests
* 95ad601: 8233421: Upgrade to
Visual Studio 2017 version 15.9.16
* 3e0557a: 8234303: [TEST_BUG]
Correct ignore tag in graphics unit tests
* e37cb37: 8234150: Address
ignored tests in ComboBoxTest, LabeledTest, HyperLinkTest and
TextInputControlTest
* 4f496d4: 8234194: [TEST_BUG]
Reenable few graphics unit tests
* 927fc8a: 8234174: Change IDEA
VCS mapping to Git
* 3d0cb49: 8234189: [TEST_BUG]
Remove ignored and invalid graphics unit tests
* dc01309: 8234110:
SwingFXUtilsTest is unsuitable for unit test framework
* 5b96ee4: 8231188: Update SQLite
to version 3.30.1
* d46dcae: 8233338: FX javadoc
headings are out of sequence
* 94bcf3f: 8231692: Test
Infrastructure: enhance KeyEventFirer to inject keyEvents into scene
* 286d1b5: 8230492: font-family
not set in HTMLEditor if font name has a number in it
* f74f3af: 8233040:
ComboBoxPopupControl: remove eventFilter for F4
* a1cc4ab: 8232210: Update Mesa
3-D Headers to version 19.2.1
* dca8df4: 8232943: Gesture
support is not initialized on iOS
* 5a70b0c: 8189092:
ArrayIndexOutOfBoundsException on Linux in getCachedGlyph
* ac71396: 8232929: Duplicate
symbols when building static libraries
* ab6ea3b: 8232158: [macOS]
Fallback to command line tools if xcode is missing
* 2ae171a: 8232687: No static JNI
loader for libprism-sw
* a09a0fa: 8232522: FX: Update
copyright year in docs, readme files to 2020
* 582d999: Merge
* b6e53f4: 8218640: Update ICU4C
to version 64.2
* bada612: Merge
* 64aaeb8: 8226754: FX build fails
using gradle 5.6+ or 6
* a4bc22d: Merge
* 2593dea: Merge
* 9df20c5: Merge
* a1aa38a: Merge
* 0ff02bb: Merge
* a433bf2: Merge
* 561153d: Merge
* 75d439e: Merge
* 758252f: Merge
* 35e0cae: 8227402: Improve XSLT processing
Since there are no conflicts, your changes will automatically be
rebased on top of the above commits when integrating. If you prefer
to do this manually, please merge `master` into your branch first.
As you do not have Committer status in this project, an existing
[Committer](https://openjdk.java.net/bylaws#committer) must agree to
[sponsor](https://openjdk.java.net/sponsor/) your change. Possible
candidates are the reviewers of this PR ***@***.***) but any other
Committer may sponsor as well.
- To flag this PR as ready for integration with the above commit
message, type `/integrate` in a new comment. (Afterwards, your
sponsor types `/sponsor` in a new comment to perform the integration).
<!-- PullRequestBot merge is ready comment -->
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#15 (comment)
|
|
Right, this still needs one more review (I hope to get to it today). The bot doesn't know whether all criteria have been met. It can't, for example, know whether there are outstanding comments from some reviewers that need to be addressed. Nor does it know which PRs need two reviewers (or sometimes a third if there is a specific person we would like to review it), which ones need a CSR, etc. So having it state authoritatively that the PR is ready to integrate is a bit misleading in this case. This is documented in the Code Review section of the CONTRIBUTING doc:
I wonder if there is a way to improve this? |
|
/integrate |
|
@kleopatra |
|
/sponsor |
openjdk
bot
added
integrated
|
@aghaisas @kleopatra The following commits have been pushed to master since your change was applied:
Your commit was automatically rebased without conflicts. Pushed as commit d2d44b4. |
Update from jfx
| if (onAction != null || actionEvent.isConsumed()) { | ||
| event.consume(); | ||
| } |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
correct, it's a bug.
#1523
There was a problem hiding this comment.
@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.commitValue();
textField.fireEvent(actionEvent);
// JDK-8207759
// mapping must not auto-consume
// instead consume here if handled by action or handlers
if (actionEvent.isConsumed()) {
event.consume();
}
}
What do you think? My code brain is a bit rusty, so might have missed something.
There was a problem hiding this comment.
@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();
}
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
FWIW, checking the isConsumed flag will only work if the action is consumed by a handler registered directly with the TextField. If there's a filter or handler attached to some other part of the scene graph a copy of the event will be made and consumed and actionEvent.isConsumed() will return false. A more complete fix is to replace textField.fireEvent with a call to EventUtil.fireEvent and check for a null return value.
If I understand this thread correctly if a KeyEvent generates an ActionEvent the KeyEvent should only be consumed if the ActionEvent is. If so the behavior should be consistent across TextFields and the controls that manage TextFields internally (Spinner, ComboBox, and DatePicker). I'm already working on a test for Escape key handling for this set of controls (for Escape the issue is whether a formatter is attached or not). I'm willing to write up a similar test for Enter handling for the same set of controls. I just need to make sure I understand exactly what behavior is expected.
There was a problem hiding this comment.
FWIW, checking the isConsumed flag will only work if the action is consumed by a handler registered directly with the TextField. If there's a filter or handler attached to some other part of the scene graph a copy of the event will be made and consumed and
actionEvent.isConsumed()will return false. A more complete fix is to replacetextField.fireEventwith a call toEventUtil.fireEventand check for anullreturn value.
It seems that an earlier check I did to see if events are copied wasn't done correctly. Indeed events are copied at every filter/handler. That does seem excessive. It also makes the public Node#fireEvent a bit useless for proper event propagation for normal users that don't have access to EventUtil.
I checked the events that are copied, and it is basically making the copy to adjust the source (target remains constant). That's really odd; I'd expect the source to remain the same as well (ie. the Scene or Window, not the "previous" filter/handler Node level), especially as it is documented as:
/**
* The object on which the Event initially occurred.
*
* @return the object on which the Event initially occurred
*/
public Object getSource() {
return source;
}
There was a problem hiding this comment.
I checked the events that are copied, and it is basically making the copy to adjust the source (target remains constant). That's really odd; I'd expect the source to remain the same as well (ie. the Scene or Window, not the "previous" filter/handler Node level), especially as it is documented as:
I haven't walked through this in AWT (there are only so many hours in the day) but I believe the AWT source corresponds to the JavaFX target i.e. the object the event was fired against. JavaFX repurposes source as the object the handler or filter was registered on. JavaFX shouldn't be pointing at the AWT documentation since it doesn't match the JavaFX implementation.
This is a fix for https://bugs.openjdk.java.net/browse/JDK-8207759
The issue is that default/cancel button on a scene are triggered even though a onKeyPressed handler for ENTER/CANCEL consumes the keyEvent. See the bug for details on both cause and fix.
There are additional/changed tests to verify the fix. All old tests are passing.
Progress
Issue
JDK-8207759: VK_ENTER not consumed by TextField when default Button exists
Approvers