Re: RFR: 8324939: Editable TableView loses focus after commit [v2]
On Wed, 3 Apr 2024 19:54:20 GMT, Jose Pereda wrote: >> This PR fixes the issue that after committing an edit on a >> ListView/TreeView/TableView/TreeTableView control, the control might lose >> the focus unexpectedly. >> >> For that, it refactors the >> `ControlUtils::requestFocusOnControlOnlyIfCurrentFocusOwnerIsChild` method, >> in order to check if the control (`ListView`, `TreeView`, `TableView`, >> `TreeTableView`) should request the focus _before_ the actual focus owner >> (which could be the control added to the cell to edit its content, like a >> `TextField`) is removed from the cell, so the `Control::requestFocus` call, >> if needed, can be still invoked after the edit commit is done (as it was >> done before). >> >> By adding >> `ControlUtils::controlShouldRequestFocusIfCurrentFocusOwnerIsChild` the >> `Cell::commitEdit` implementations can now query if the control should have >> the focus, after `super.commitEdit(newValue);` but before firing the >> `CellEditEvent` and calling `updateItem()`, and if the result is true, then >> request focus after the edit commit ends (like it was done before). >> >> Two new tests per control have been included, to verify that the focus >> remains at the control, one for edit cancel (this passes before and after >> the proposed changes), one for edit commit (this fails before and passes >> after including the proposed fix). > > Jose Pereda has updated the pull request incrementally with one additional > commit since the last revision: > > Address feedback from reviewer Looks good to me too. Fix and the rationale behind makes sense. Something I also noticed on virtual containers (cells). - Marked as reviewed by mhanl (Committer). PR Review: https://git.openjdk.org/jfx/pull/1411#pullrequestreview-1978930892
Re: RFR: 8324939: Editable TableView loses focus after commit [v2]
On Wed, 3 Apr 2024 19:51:08 GMT, Jose Pereda wrote: >> modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeViewTest.java >> line 2362: >> >>> 2360: assertTrue(treeView.isFocused()); >>> 2361: >>> 2362: VirtualFlowTestUtils.BLOCK_STAGE_LOADER_DISPOSE = true; >> >> This should not be needed since you are creating a `stageLoader` above. This >> seems to be only used when no `stageLoader` was created before > > Thanks @Maran23, I've removed it now from the added tests. > I wish there was a more reliable way of implementing getCell() without > resorting to undocumented flags like BLOCK_STAGE_LOADER_DISPOSE. There is, just add the virtual container to a Stage. Since the cells are only created inside the skin, which is only created when the component is inside the scene tree. - PR Review Comment: https://git.openjdk.org/jfx/pull/1411#discussion_r1551054299
Re: RFR: 8324939: Editable TableView loses focus after commit [v2]
On Wed, 3 Apr 2024 19:54:20 GMT, Jose Pereda wrote: >> This PR fixes the issue that after committing an edit on a >> ListView/TreeView/TableView/TreeTableView control, the control might lose >> the focus unexpectedly. >> >> For that, it refactors the >> `ControlUtils::requestFocusOnControlOnlyIfCurrentFocusOwnerIsChild` method, >> in order to check if the control (`ListView`, `TreeView`, `TableView`, >> `TreeTableView`) should request the focus _before_ the actual focus owner >> (which could be the control added to the cell to edit its content, like a >> `TextField`) is removed from the cell, so the `Control::requestFocus` call, >> if needed, can be still invoked after the edit commit is done (as it was >> done before). >> >> By adding >> `ControlUtils::controlShouldRequestFocusIfCurrentFocusOwnerIsChild` the >> `Cell::commitEdit` implementations can now query if the control should have >> the focus, after `super.commitEdit(newValue);` but before firing the >> `CellEditEvent` and calling `updateItem()`, and if the result is true, then >> request focus after the edit commit ends (like it was done before). >> >> Two new tests per control have been included, to verify that the focus >> remains at the control, one for edit cancel (this passes before and after >> the proposed changes), one for edit commit (this fails before and passes >> after including the proposed fix). > > Jose Pereda has updated the pull request incrementally with one additional > commit since the last revision: > > Address feedback from reviewer Marked as reviewed by angorya (Reviewer). - PR Review: https://git.openjdk.org/jfx/pull/1411#pullrequestreview-1978103130
Re: RFR: 8324939: Editable TableView loses focus after commit [v2]
> This PR fixes the issue that after committing an edit on a > ListView/TreeView/TableView/TreeTableView control, the control might lose the > focus unexpectedly. > > For that, it refactors the > `ControlUtils::requestFocusOnControlOnlyIfCurrentFocusOwnerIsChild` method, > in order to check if the control (`ListView`, `TreeView`, `TableView`, > `TreeTableView`) should request the focus _before_ the actual focus owner > (which could be the control added to the cell to edit its content, like a > `TextField`) is removed from the cell, so the `Control::requestFocus` call, > if needed, can be still invoked after the edit commit is done (as it was done > before). > > By adding `ControlUtils::controlShouldRequestFocusIfCurrentFocusOwnerIsChild` > the `Cell::commitEdit` implementations can now query if the control should > have the focus, after `super.commitEdit(newValue);` but before firing the > `CellEditEvent` and calling `updateItem()`, and if the result is true, then > request focus after the edit commit ends (like it was done before). > > Two new tests per control have been included, to verify that the focus > remains at the control, one for edit cancel (this passes before and after the > proposed changes), one for edit commit (this fails before and passes after > including the proposed fix). Jose Pereda has updated the pull request incrementally with one additional commit since the last revision: Address feedback from reviewer - Changes: - all: https://git.openjdk.org/jfx/pull/1411/files - new: https://git.openjdk.org/jfx/pull/1411/files/8f312781..744562f0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1411&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1411&range=00-01 Stats: 16 lines in 4 files changed: 0 ins; 16 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1411.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1411/head:pull/1411 PR: https://git.openjdk.org/jfx/pull/1411
Re: RFR: 8324939: Editable TableView loses focus after commit [v2]
On Thu, 28 Mar 2024 11:20:23 GMT, Marius Hanl wrote: >> Jose Pereda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Address feedback from reviewer > > modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeViewTest.java > line 2362: > >> 2360: assertTrue(treeView.isFocused()); >> 2361: >> 2362: VirtualFlowTestUtils.BLOCK_STAGE_LOADER_DISPOSE = true; > > This should not be needed since you are creating a `stageLoader` above. This > seems to be only used when no `stageLoader` was created before Thanks @Maran23, I've removed it now from the added tests. - PR Review Comment: https://git.openjdk.org/jfx/pull/1411#discussion_r1550359262
Re: RFR: 8324939: Editable TableView loses focus after commit
On Wed, 20 Mar 2024 10:55:56 GMT, Jose Pereda wrote:
> This PR fixes the issue that after committing an edit on a
> ListView/TreeView/TableView/TreeTableView control, the control might lose the
> focus unexpectedly.
>
> For that, it refactors the
> `ControlUtils::requestFocusOnControlOnlyIfCurrentFocusOwnerIsChild` method,
> in order to check if the control (`ListView`, `TreeView`, `TableView`,
> `TreeTableView`) should request the focus _before_ the actual focus owner
> (which could be the control added to the cell to edit its content, like a
> `TextField`) is removed from the cell, so the `Control::requestFocus` call,
> if needed, can be still invoked after the edit commit is done (as it was done
> before).
>
> By adding `ControlUtils::controlShouldRequestFocusIfCurrentFocusOwnerIsChild`
> the `Cell::commitEdit` implementations can now query if the control should
> have the focus, after `super.commitEdit(newValue);` but before firing the
> `CellEditEvent` and calling `updateItem()`, and if the result is true, then
> request focus after the edit commit ends (like it was done before).
>
> Two new tests per control have been included, to verify that the focus
> remains at the control, one for edit cancel (this passes before and after the
> proposed changes), one for edit commit (this fails before and passes after
> including the proposed fix).
The problem listed in the ticket is fixed, tested on macOS 14.4.1 M1 with the
monkey tester.
Thank you for adding tests and fixing the comments!
(there was a comment from @Maran23 , if you would like to address it I'll
re-approve).
modules/javafx.controls/src/main/java/javafx/scene/control/TreeCell.java line
429:
> 427:
> 428: if (tree != null) {
> 429: // reset the editing item in the TreeView
+1
-
Marked as reviewed by angorya (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/1411#pullrequestreview-1977641091
PR Review Comment: https://git.openjdk.org/jfx/pull/1411#discussion_r1550251709
Re: RFR: 8324939: Editable TableView loses focus after commit
On Thu, 28 Mar 2024 11:20:23 GMT, Marius Hanl wrote: >> This PR fixes the issue that after committing an edit on a >> ListView/TreeView/TableView/TreeTableView control, the control might lose >> the focus unexpectedly. >> >> For that, it refactors the >> `ControlUtils::requestFocusOnControlOnlyIfCurrentFocusOwnerIsChild` method, >> in order to check if the control (`ListView`, `TreeView`, `TableView`, >> `TreeTableView`) should request the focus _before_ the actual focus owner >> (which could be the control added to the cell to edit its content, like a >> `TextField`) is removed from the cell, so the `Control::requestFocus` call, >> if needed, can be still invoked after the edit commit is done (as it was >> done before). >> >> By adding >> `ControlUtils::controlShouldRequestFocusIfCurrentFocusOwnerIsChild` the >> `Cell::commitEdit` implementations can now query if the control should have >> the focus, after `super.commitEdit(newValue);` but before firing the >> `CellEditEvent` and calling `updateItem()`, and if the result is true, then >> request focus after the edit commit ends (like it was done before). >> >> Two new tests per control have been included, to verify that the focus >> remains at the control, one for edit cancel (this passes before and after >> the proposed changes), one for edit commit (this fails before and passes >> after including the proposed fix). > > modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeViewTest.java > line 2362: > >> 2360: assertTrue(treeView.isFocused()); >> 2361: >> 2362: VirtualFlowTestUtils.BLOCK_STAGE_LOADER_DISPOSE = true; > > This should not be needed since you are creating a `stageLoader` above. This > seems to be only used when no `stageLoader` was created before I wish there was a more reliable way of implementing getCell() without resorting to undocumented flags like BLOCK_STAGE_LOADER_DISPOSE. - PR Review Comment: https://git.openjdk.org/jfx/pull/1411#discussion_r1550266702
Re: RFR: 8324939: Editable TableView loses focus after commit
On Wed, 20 Mar 2024 10:55:56 GMT, Jose Pereda wrote: > This PR fixes the issue that after committing an edit on a > ListView/TreeView/TableView/TreeTableView control, the control might lose the > focus unexpectedly. > > For that, it refactors the > `ControlUtils::requestFocusOnControlOnlyIfCurrentFocusOwnerIsChild` method, > in order to check if the control (`ListView`, `TreeView`, `TableView`, > `TreeTableView`) should request the focus _before_ the actual focus owner > (which could be the control added to the cell to edit its content, like a > `TextField`) is removed from the cell, so the `Control::requestFocus` call, > if needed, can be still invoked after the edit commit is done (as it was done > before). > > By adding `ControlUtils::controlShouldRequestFocusIfCurrentFocusOwnerIsChild` > the `Cell::commitEdit` implementations can now query if the control should > have the focus, after `super.commitEdit(newValue);` but before firing the > `CellEditEvent` and calling `updateItem()`, and if the result is true, then > request focus after the edit commit ends (like it was done before). > > Two new tests per control have been included, to verify that the focus > remains at the control, one for edit cancel (this passes before and after the > proposed changes), one for edit commit (this fails before and passes after > including the proposed fix). modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeViewTest.java line 2362: > 2360: assertTrue(treeView.isFocused()); > 2361: > 2362: VirtualFlowTestUtils.BLOCK_STAGE_LOADER_DISPOSE = true; This should not be needed since you are creating a `stageLoader` above. This seems to be only used when no `stageLoader` was created before - PR Review Comment: https://git.openjdk.org/jfx/pull/1411#discussion_r1542737994
Re: RFR: 8324939: Editable TableView loses focus after commit
On Mon, 25 Mar 2024 20:11:21 GMT, Kevin Rushforth wrote: > Can you review this? in the queue; yes. - PR Comment: https://git.openjdk.org/jfx/pull/1411#issuecomment-2018832489
Re: RFR: 8324939: Editable TableView loses focus after commit
On Wed, 20 Mar 2024 10:55:56 GMT, Jose Pereda wrote: > This PR fixes the issue that after committing an edit on a > ListView/TreeView/TableView/TreeTableView control, the control might lose the > focus unexpectedly. > > For that, it refactors the > `ControlUtils::requestFocusOnControlOnlyIfCurrentFocusOwnerIsChild` method, > in order to check if the control (`ListView`, `TreeView`, `TableView`, > `TreeTableView`) should request the focus _before_ the actual focus owner > (which could be the control added to the cell to edit its content, like a > `TextField`) is removed from the cell, so the `Control::requestFocus` call, > if needed, can be still invoked after the edit commit is done (as it was done > before). > > By adding `ControlUtils::controlShouldRequestFocusIfCurrentFocusOwnerIsChild` > the `Cell::commitEdit` implementations can now query if the control should > have the focus, after `super.commitEdit(newValue);` but before firing the > `CellEditEvent` and calling `updateItem()`, and if the result is true, then > request focus after the edit commit ends (like it was done before). > > Two new tests per control have been included, to verify that the focus > remains at the control, one for edit cancel (this passes before and after the > proposed changes), one for edit commit (this fails before and passes after > including the proposed fix). @andy-goryachev-oracle Can you review this? - PR Comment: https://git.openjdk.org/jfx/pull/1411#issuecomment-2018826207
RFR: 8324939: Editable TableView loses focus after commit
This PR fixes the issue that after committing an edit on a ListView/TreeView/TableView/TreeTableView control, the control might lose the focus unexpectedly. For that, it refactors the `ControlUtils::requestFocusOnControlOnlyIfCurrentFocusOwnerIsChild` method, in order to check if the control (`ListView`, `TreeView`, `TableView`, `TreeTableView`) should request the focus _before_ the actual focus owner (which could be the control added to the cell to edit its content, like a `TextField`) is removed from the cell, so the `Control::requestFocus` call, if needed, can be still invoked after the edit commit is done (as it was done before). By adding `ControlUtils::controlShouldRequestFocusIfCurrentFocusOwnerIsChild` the `Cell::commitEdit` implementations can now query if the control should have the focus, after `super.commitEdit(newValue);` but before firing the `CellEditEvent` and calling `updateItem()`, and if the result is true, then request focus after the edit commit ends (like it was done before). Two new tests per control have been included, to verify that the focus remains at the control, one for edit cancel (this passes before and after the proposed changes), one for edit commit (this fails before and passes after including the proposed fix). - Commit messages: - Refactor ControlUtils, check focus before owner is gone, and include tests Changes: https://git.openjdk.org/jfx/pull/1411/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1411&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8324939 Stats: 459 lines in 9 files changed: 438 ins; 5 del; 16 mod Patch: https://git.openjdk.org/jfx/pull/1411.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1411/head:pull/1411 PR: https://git.openjdk.org/jfx/pull/1411
