8191995: Regression: DatePicker must commit on focusLost

[Maran23]

This PR fixes an issue where the DatePicker is not committing his text as value when the focus is lost.
As the ticket also mentions, this is a regression which last worked on JavaFX 8 and got broken by this refactoring: JDK-8150946

The fix is to provide the same api to the DatePicker which was introduced by JDK-8150946 for ComboBox and Spinner.

Note: While fixing this I found a possible bug which I tracked here: JDK-8277756
-> When creating a DatePicker with the second constructor (with LocalDate as parameter) two listener won't be added since they are only added at the first constructor (That's also why I added the focusProperty listener in the second constructor).

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed
• Change requires a CSR request to be approved

Issues

• JDK-8191995: Regression: DatePicker must commit on focusLost
• JDK-8277782: Regression: DatePicker must commit on focusLost (CSR)

Reviewers

• Jeanette Winzenburg (@kleopatra - Reviewer)
• Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/679/head:pull/679
$ git checkout pull/679

Update a local copy of the PR:
$ git checkout pull/679
$ git pull https://git.openjdk.java.net/jfx pull/679/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 679

View PR using the GUI difftool:
$ git pr show -t 679

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/679.diff

[bridgekeeper]

👋 Welcome back mhanl! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[mlbridge]

Webrevs

• 00: Full (3fe5ae3f)

[kevinrushforth]

/reviewers 2
/csr

[openjdk]

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

[openjdk]

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@Maran23 please create a CSR request for issue JDK-8191995. This pull request cannot be integrated until the CSR request is approved.

[kleopatra]

will review thoroughly soon - for now just a comment to get rid of that annoying yellow bar at the top ;)

On first skim, it looks straightforward - adding the api same as for combo/spinner. I think you might get going and flesh out the csr.

[Maran23]

I just finished the CSR. :)

[kleopatra]

fix looks good - verified that the new api and its implementation is the exact same as in ComboBox.

Tests look good as well - verified the focused-related test fails/passes before/after the fix, all other tests pass after the fix.

[kevinrushforth]

Looks good. I've also "Reviewed" the CSR, so it can be finalized.

[openjdk]

@Maran23 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8191995: Regression: DatePicker must commit on focusLost

Reviewed-by: fastegal, kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 11 new commits pushed to the master branch:

• 5805bf8: 8276313: ScrollPane scroll delta incorrectly depends on content height
• d3fbb51: 8276553: ListView scrollTo() is broken after fix for JDK-8089589
• aa045c5: 8272118: ListViewSkin et al: must not cancel edit on scrolling
• 5bd72a7: 8277457: AccessControlException: access denied ("java.net.NetPermission" "getCookieHandler")
• 3d57213: 8277475: Update JDK_DOCS property to point to JDK 17 docs
• b257647: 8276847: JSException: ReferenceError: Can't find variable: IntersectionObserver
• fc3792d: 8276206: Rename TextBinding class to better express its purpose
• 0dbdec4: 8276490: Incorrect path for duplicate x and y values, when path falls outside axis bound
• d14be6a: 8274061: Tree-/TableRowSkin: misbehavior on switching skin
• 423e1be: 8277133: Dragboard contents retrieved all over again during a DND process on WebView
• ... and 1 more: https://git.openjdk.java.net/jfx/compare/e694fb55692d3d438a947e2b0c307083e033ab30...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

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 (@kleopatra, @kevinrushforth) 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).

[Maran23]

/integrate

[openjdk]

@Maran23
Your change (at version 3fe5ae3) is now ready to be sponsored by a Committer.

[kevinrushforth]

/sponsor

[openjdk]

Going to push as commit 6fd4ab6.
Since your change was applied there have been 11 commits pushed to the master branch:

• 5805bf8: 8276313: ScrollPane scroll delta incorrectly depends on content height
• d3fbb51: 8276553: ListView scrollTo() is broken after fix for JDK-8089589
• aa045c5: 8272118: ListViewSkin et al: must not cancel edit on scrolling
• 5bd72a7: 8277457: AccessControlException: access denied ("java.net.NetPermission" "getCookieHandler")
• 3d57213: 8277475: Update JDK_DOCS property to point to JDK 17 docs
• b257647: 8276847: JSException: ReferenceError: Can't find variable: IntersectionObserver
• fc3792d: 8276206: Rename TextBinding class to better express its purpose
• 0dbdec4: 8276490: Incorrect path for duplicate x and y values, when path falls outside axis bound
• d14be6a: 8274061: Tree-/TableRowSkin: misbehavior on switching skin
• 423e1be: 8277133: Dragboard contents retrieved all over again during a DND process on WebView
• ... and 1 more: https://git.openjdk.java.net/jfx/compare/e694fb55692d3d438a947e2b0c307083e033ab30...master

Your commit was automatically rebased without conflicts.

[openjdk]

@kevinrushforth @Maran23 Pushed as commit 6fd4ab6.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[mlbridge]

Mailing list message from Dirk Lemmermann on openjfx-dev:

Just submitted a bug report related to this issue. The DatePicker could now commit back to the old / previous value when the user selection of a new date via the dropdown also triggers the closing of the dialog that is showing the date picker. In that case the newly added commitValue() method will use the date expressed by the text inside the editor (TextField), which is actually the old date.

Dirk

On 7 Dec 2021, at 14:11, Kevin Rushforth <kcr at openjdk.java.net> wrote:

On Wed, 24 Nov 2021 09:09:53 GMT, Marius Hanl <mhanl at openjdk.org> wrote:

This PR fixes an issue where the `DatePicker` is not committing his text as value when the focus is lost.
As the ticket also mentions, this is a regression which last worked on JavaFX 8 and got broken by this refactoring: [JDK-8150946](https://bugs.openjdk.java.net/browse/JDK-8150946)

The fix is to provide the same api to the `DatePicker` which was introduced by [JDK-8150946](https://bugs.openjdk.java.net/browse/JDK-8150946) for `ComboBox` and `Spinner`.

Note: While fixing this I found a possible bug which I tracked here: [JDK-8277756](https://bugs.openjdk.java.net/browse/JDK-8277756)
-> When creating a `DatePicker` with the second constructor (with `LocalDate` as parameter) two listener won't be added since they are only added at the first constructor (That's also why I added the focusProperty listener in the second constructor).

Looks good. I've also "Reviewed" the CSR, so it can be finalized.

-------------

Marked as reviewed by kcr (Lead).

PR: https://git.openjdk.java.net/jfx/pull/679

[mlbridge]

Mailing list message from Kevin Rushforth on openjfx-dev:

Thanks for reporting this.

-- Kevin

On 10/10/2022 10:15 AM, Dirk Lemmermann wrote:

Just submitted a bug report related to this issue. The DatePicker could now commit back to the old / previous value when the user selection of a new date via the dropdown also triggers the closing of the dialog that is showing the date picker. In that case the newly added commitValue() method will use the date expressed by the text inside the editor (TextField), which is actually the old date.

Dirk

On 7 Dec 2021, at 14:11, Kevin Rushforth <kcr at openjdk.java.net> wrote:

On Wed, 24 Nov 2021 09:09:53 GMT, Marius Hanl <mhanl at openjdk.org> wrote:

This PR fixes an issue where the `DatePicker` is not committing his text as value when the focus is lost.
As the ticket also mentions, this is a regression which last worked on JavaFX 8 and got broken by this refactoring: [JDK-8150946](https://bugs.openjdk.java.net/browse/JDK-8150946)

The fix is to provide the same api to the `DatePicker` which was introduced by [JDK-8150946](https://bugs.openjdk.java.net/browse/JDK-8150946) for `ComboBox` and `Spinner`.

Note: While fixing this I found a possible bug which I tracked here: [JDK-8277756](https://bugs.openjdk.java.net/browse/JDK-8277756)
-> When creating a `DatePicker` with the second constructor (with `LocalDate` as parameter) two listener won't be added since they are only added at the first constructor (That's also why I added the focusProperty listener in the second constructor).
Looks good. I've also "Reviewed" the CSR, so it can be finalized.

-------------

Marked as reviewed by kcr (Lead).

PR: https://git.openjdk.java.net/jfx/pull/679