On Tue, 24 Mar 2026 19:18:57 GMT, Marius Hanl <[email protected]> wrote:
>> This is an implementation for the long standing issue of allowing to commit >> a cell value when the focus is lost or the editing index (cell) changed. >> This also contains >> [JDK-8089311](https://bugs.openjdk.org/browse/JDK-8089311) (for better >> understanding the usecase with `TextField` cells, but we may split this >> later on). >> >> API >> - >> - Instead of calling `cancelEdit`, every cell now calls `stopEdit` when the >> focus is lost or the editing index changed. The default behavior is >> cancelling the edit, but developers can now override the behavior and allow >> a `commitEdit` instead >> - There are multiple 'events' that can lead to a editing change. Every >> change will now call `stopEdit`. >> It is therefore the responsibility of the developer to decide, when it makes >> sense to actually commit the value instead of cancelling it. >> This decision was made as the behavior is manipulating the editing index, >> but you as a developer can as well. Therefore, we do not always know what >> intention led to a change of the editing index. >> - Every `MOUSE_PRESSED` shifts the focus to the cell container, which is >> undesired in case of editing the cell. So this event is now consumed. The >> cell container will request focus before (if it is not yet set) >> Note: So basically, we request focus a tiny bit earlier (for commit reasons) >> instead of after, which interruppts the commit >> - All `TextField` cells now commit their value (instead of cancel) on focus >> loss >> - `TextField` Escape handling was badly implemented (it was never really >> called, as the cell container handled Escape before) >> >> Considerations >> - >> - I tried to make the API minimal, and without breaking changes (other than >> the `TextField` cells committing their values, but we may split this up) >> - Clicking the `ScrollBar` now commits/cancels the edit. I checked other >> applications and this is very common. But something I need to note here. >> This probably can be fixed in the same way mentioned above (`focusWithin`) >> - It might be hard for a developer to exactly know the cause why `stopEdit` >> is called. This does not seem like a problem, as e.g. for a `TextField`, you >> normally register listeners for e.g. focus loss for commit or pressing the >> Escape key for cancel, so you keep full control. >> >> Possible next PRs >> - >> - The Cell Container focus behavior is, well, weird right now. That is why >> consuming the event is needed to better support this PR. One thing we may >> can consider is using the `focusWithin` property instead for all 4 C... > > Marius Hanl has updated the pull request incrementally with one additional > commit since the last revision: > > Improve behavior select signature, improve javadoc Still a preliminary review - I am trying to understand the details at the high level. So I see one new API (that should be listed in the **API** section), and a bunch of behavioral changes that are listed in the **API** and **Considerations** sections. Maybe move the behavioral changes into its own **Behavioral Changes** section? Is it possible/feasible to show the changes in a "Before/After" table, or at least provide some examples to start with? Specifically, > All TextField cells now commit their value (instead of cancel) on focus loss what happens when there is a validation error coming from the value converter? > TextField Escape handling was badly implemented (it was never really called, > as the cell container handled Escape before) what was the problem before exactly? how does it work now? modules/javafx.controls/src/main/java/javafx/scene/control/Cell.java line 374: > 372: /** > 373: * Stops the edit operation of the cell. > 374: * This method is called when another cell is edited or the focus > changed. called by whom? is it meant to be called by the application? can it be made `protected`? modules/javafx.controls/src/main/java/javafx/scene/control/Cell.java line 379: > 377: * This method is meant to be subclassed in case the default > behavior is not enough. > 378: * For example, subclasses decide to rather commit the edit > operation instead of cancelling. > 379: */ needs a `@since 27` ------------- PR Review: https://git.openjdk.org/jfx/pull/1935#pullrequestreview-4001871475 PR Review Comment: https://git.openjdk.org/jfx/pull/1935#discussion_r2983906535 PR Review Comment: https://git.openjdk.org/jfx/pull/1935#discussion_r2983904663
