8245053: Keyboard doesn't show when TextInputControl has focus #219

abhinayagarwal · 2020-05-14T19:22:54Z

In Android, TextInputControls (TextField and TextArea) are responsible for showing and hiding software keyboard. Currently, a focus listener is attached to these controls and is used to toggle the visibility of keyboard. This condition fails in cases where the control already has focus but the keyboard is not visible.

Ideally, the keyboard should be shown again when the user taps on the TextInputControl.

This PR adds an event handler for MouseEvent.MOUSE_CLICKED event and shows the keyboard if the TextInput control is both editable and focused.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8245053: Keyboard doesn't show when TextInputControl has focus

Reviewers

Johan Vos (jvos - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/219/head:pull/219
$ git checkout pull/219

bridgekeeper · 2020-05-14T19:26:11Z

👋 Welcome back abhinayagarwal! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

mlbridge · 2020-05-14T19:30:19Z

Webrevs

kleopatra · 2020-05-15T10:58:14Z

out off my home-zone here, just fyi and in case it might be relevant to android skins as well: there's an umbrella issue to cleanup skin implementations JDK-8241364 - both the android text skins look like leaking in the manually registered focus listener (not introduced here) and the eventHandler added here. Probably should be removed manually in dispose (or in the case of the focusListener, registered via skin api).

abhinayagarwal · 2020-05-19T08:08:44Z

@kleopatra Done!

modules/javafx.controls/src/android/java/javafx/scene/control/skin/TextAreaSkinAndroid.java

kleopatra · 2020-05-19T09:23:38Z

wondering if there is any way to include android specific skins in unit testing? For all (nearly, htmlEditor excluded ;) others we have a test to guard against contract violations (and will soon have tests for memory leaks).

abhinayagarwal · 2020-05-19T13:58:00Z

Indeed. It would be good if we could add a few Android specific tests which are automatically activated when -PCOMPILE_TARGETS=android is passed to Gradle.

johanvos

Looks good.
A test strategy for Android is a good idea for a follow-up issue.

openjdk · 2020-05-29T12:48:56Z

@abhinayagarwal This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8245053: Keyboard doesn't show when TextInputControl has focus

Reviewed-by: jvos

If you would like to add a summary, use the /summary command.
To credit additional contributors, use the /contributor command.
To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 83 commits pushed to the master branch:

a46b250: Merge
926b5b6: 8249839: Cherry pick GTK WebKit 2.28.3 changes
6b00892: 8250238: Media fails to load libav 58 library when using modules from maven central
aae8b6b: Merge
3cc29e3: 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175%
5f60ea5: 8248381: Create a daemon thread for MonocleTimer
9260fd1: Merge
f056d24: 8248908: Printer.createPageLayout() returns 0.75" margins instead of hardware margins
5e7e452: 8245284: Update to 610.1 version of WebKit
2f4666a: 8248490: [macOS] Undecorated stage does not minimize
... and 73 more: https://git.openjdk.java.net/jfx/compare/1cae6a87a8322abf96e56853fc8d205723439e42...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate a46b2507c565e6a79aad79db589e82b3c9ccc0a9.

As you are not a known OpenJDK Author, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@johanvos) 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).

mipastgt · 2020-05-29T13:07:26Z

Have you considered the case where a TextField is focused by default when its view is shown? As soon as the view is shown the keybord will pop up, even if the user does not plan to edit anything. I just experienced this when I exchanged the Label in the HelloGluon example against a TextField. A lot of strange things then happen. The keyboard pops up, it hides the Gluon nag screen, if you close the keyboard and the nag screen you still see these edit markers and all this just because this text field was focused by default. To my opinion the keyboard and the edit markers should only be shown when the user actually clicks into the text field and thus indicates that he really wants to edit something. I also noticed that after the keyboard was shown I could not normally close the application anymore. (This was on Android with a Galaxy Tab 4)

johanvos

worth discussing the case were a TextInput is the only control.

abhinayagarwal · 2020-06-12T13:38:10Z

@mipastgt The issue happens because of the focus listener. With this PR, we introduce mouseEventListener to show the Keyboard on MOUSE_CLICKED event. Therefore, the issue brought up by your previous comment should be resolved by updating the focusChangeListener to:

private final ChangeListener<Boolean> focusChangeListener = (observable, wasFocused, isFocused) -> {
    if (wasFocused && !isFocused) {
        hideSoftwareKeyboard();
    }
};

We should most probably also introduce an API for showing the keyboard as available on both iOS and Android platforms :)

Thoughts?

johanvos · 2020-06-25T06:39:51Z

It is probably good to add this suggestion in this PR. I'll review again once it is in.

…ible for hiding the keyboard only.

abhinayagarwal · 2020-07-30T08:31:50Z

/integrate

openjdk · 2020-07-30T08:32:18Z

@abhinayagarwal
Your change (at version b13c44b) is now ready to be sponsored by a Committer.

johanvos · 2020-07-30T08:33:45Z

/sponsor

openjdk · 2020-07-30T08:34:47Z

@johanvos @abhinayagarwal The following commits have been pushed to master since your change was applied:

a46b250: Merge
926b5b6: 8249839: Cherry pick GTK WebKit 2.28.3 changes
6b00892: 8250238: Media fails to load libav 58 library when using modules from maven central
aae8b6b: Merge
3cc29e3: 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175%
5f60ea5: 8248381: Create a daemon thread for MonocleTimer
9260fd1: Merge
f056d24: 8248908: Printer.createPageLayout() returns 0.75" margins instead of hardware margins
5e7e452: 8245284: Update to 610.1 version of WebKit
2f4666a: 8248490: [macOS] Undecorated stage does not minimize
281f5c2: Merge
5de99be: Merge
946590e: 8248365: Debug build crashes on Windows when playing media file
e2d1c02: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException
82f66ca: Merge
d67c47f: 8201567: QuantumRenderer modifies buffer in use by JavaFX Application Thread
cd57bb5: Merge
a4f3150: 8248317: Change JavaFX release version to 16
126637f: 8201570: Get two bytes for the Linux input event type, not four
f3a0446: 8247963: Update SQLite to version 3.32.3
32584db: 8238954: Improve performance of tiled snapshot rendering
869ea40: 8244212: Optionally download media and webkit libraries from latest openjfx EA build
62f8cee: 8247947: Build DirectShow Samples (Base Classes) from source checked into repo
527cc2b: 8248551: [TestBug] Ignore two failing FXML unit tests which use Nashorn script engine
45c9854: 8238080: FXMLLoader: if script engines implement javax.script.Compilable compile scripts
15f97ed: 8240264: iOS: Unnecessary logging on every pulse when GL context changes
2ca509a: 8193800: TreeTableView selection changes on sorting
a5878e0: 8214699: Node.getPseudoClassStates must return the same instance on every call
8440b64: 8208169: can not print selected pages of web page
1727dfa: 8247163: JFXPanel throws exception on click when no Scene is set
54e2507: 8247360: Add missing license file for Microsoft DirectShow Samples
fb962ac: 8244418: MenuBar: IOOB exception on requestFocus on empty bar
bf2e972: 8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars
b200891: 8244824: TableView : Incorrect German translation
f6ec5f1: Merge
b2b46eb: 8242892: SpinnerValueFactory has an implicit no-arg constructor
afa805f: 8245575: Show the ContextMenu of input controls with long press gesture on iOS
5304266: 8245635: GlassPasteboard::getUTFs fails on iOS
ba501ef: 8246357: Allow static build of webkit library on linux
a02e09d: 8246195: ListViewSkin/Behavior: misbehavior on switching skin
9749982: 8246204: No 3D support for newer Intel graphics drivers on Linux
6bd0e22: 8239095: Upgrade libFFI to the latest 3.3 version
853ac78: 8245282: Button/Combo Behavior: memory leak on dispose
a78b3fb: 8242523: Update the animation and clip envelope classes
1ab653c: 8244657: ChoiceBox/ToolBarSkin: misbehavior on switching skin
804ccce: 8244195: [TEST_BUG] Convert the system tests TabPanePermuteGetTabsTest to unit test
1663624: Merge
13f42e1: Merge
9edba9c: 8243110: SVGTest.testSVGRenderingWithPattern fails intermittently
168b7f7: 8246099: Intermittent test failures in SandboxAppTest
78bf7b7: 8245422: Better Pisces rasterizing
c41777e: 8245634: [TestBug] Enable and fix tests ignored with message "impl_cssSet API removed"
3ceee69: 8245499: Text input controls should show handles on iOS
8914bd2: 8234540: javafx.web LeakTest.testGarbageCollectability fails intermittently
16f446a: 8234876: Unit test classes should not extend Application
2d98fe6: 8245601: TESTBUG] Disable TabPaneDragPolicyTest on Mac until JDK-8213136 is fixed and fix ISE
f3190db: 8244531: Tests: add support to identify recurring issues with controls et al
1971c70: 8245457: TestBug] Enable and fix ignored tests in ButtonBaseTest & ButtonTest
6e0b45a: 8245183: Two fxml unit tests log warnings about deprecated escape sequences
a13a642: 8244579: Windows "User Objects" leakage with WebView
37b5edc: 8245456: MacPasteboard throws ClassCastException on static builds
6e03930: 8237602: TabPane doesn't respect order of TabPane.getTabs() list
f8cc090: Merge
bb24322: 8244112: Skin implementations: must not violate contract of dispose
dbb6437: 8244647: Wrong first layout pass of Scrollbar controls on touch supported devices
7b06190: 8242548: Wrapped labeled controls using -fx-line-spacing cut text off
435671e: 8202296: Monocle MouseInput doesn't send keyboard modifiers in events.
c14cc44: 8244417: support static build for Windows
b14e085: 8244735: Error on iOS passing keys with unicode values greater than 255
b0d66d0: 8242508: Upgrade to Visual Studio 2019 version 16.5.3
0f87d20: 8244487: One Windows 10 SDK file missing from FX build
4597437: Merge
4ec163d: 8242001: ChoiceBox: must update value on setting SelectionModel, part2
236e2d6: 8244421: Wrong scrollbar position on touch enabled devices
0385563: 8243255: Font size is large in JavaFX app with enabled Monocle on Raspberry Pi
2e90076: 8242507: Add support for Visual Studio 2019
39d9c3b: 8244110: NPE in MenuButtonSkinBase change listener
99f7747: 8241999: ChoiceBox: incorrect toggle selected for uncontained
f1ac39b: Merge
bb5ce2c: Merge
d299465: Merge
d2346b8: Merge
397587d: 8241108: Glib improvements

Your commit was automatically rebased without conflicts.

Pushed as commit 5d34d72.

Show keyboard when user taps on TextInput controls

6f56d48

openjdk bot added the rfr Ready for review label May 14, 2020

kevinrushforth requested a review from johanvos May 15, 2020 17:20

kevinrushforth assigned abhinayagarwal May 15, 2020

Remove listeners in dispose method

9b7d54f

kleopatra reviewed May 19, 2020

View reviewed changes

modules/javafx.controls/src/android/java/javafx/scene/control/skin/TextAreaSkinAndroid.java Show resolved Hide resolved

Skin implementation must maintain the contract of dispose

0b22747

johanvos approved these changes May 29, 2020

View reviewed changes

openjdk bot added the ready Ready to be integrated label May 29, 2020

Fix typo: dispose should remove event handler

3024ddc

openjdk bot removed the ready Ready to be integrated label May 29, 2020

johanvos reviewed May 29, 2020

View reviewed changes

openjdk bot removed the rfr Ready for review label Jun 25, 2020

abhinayagarwal force-pushed the JDK-8245053 branch from 0aa7902 to 44fc875 Compare June 25, 2020 17:46

openjdk bot added the rfr Ready for review label Jun 25, 2020

Show keyboard on mouse event. Focus change listener should be respons…

b13c44b

…ible for hiding the keyboard only.

abhinayagarwal force-pushed the JDK-8245053 branch from 44fc875 to b13c44b Compare June 25, 2020 18:05

johanvos approved these changes Jul 29, 2020

View reviewed changes

openjdk bot added the ready Ready to be integrated label Jul 29, 2020

openjdk bot added the sponsor Ready to sponsor label Jul 30, 2020

openjdk bot closed this Jul 30, 2020

openjdk bot added integrated Pull request has been integrated and removed sponsor Ready to sponsor ready Ready to be integrated rfr Ready for review labels Jul 30, 2020

8245053: Keyboard doesn't show when TextInputControl has focus #219

8245053: Keyboard doesn't show when TextInputControl has focus #219

Uh oh!

Conversation

abhinayagarwal commented May 14, 2020 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Download

Uh oh!

bridgekeeper bot commented May 14, 2020

Uh oh!

mlbridge bot commented May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

kleopatra commented May 15, 2020

Uh oh!

abhinayagarwal commented May 19, 2020

Uh oh!

Uh oh!

kleopatra commented May 19, 2020

Uh oh!

abhinayagarwal commented May 19, 2020

Uh oh!

johanvos left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mipastgt commented May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johanvos left a comment

Choose a reason for hiding this comment

Uh oh!

abhinayagarwal commented Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johanvos commented Jun 25, 2020

Uh oh!

abhinayagarwal commented Jul 30, 2020

Uh oh!

openjdk bot commented Jul 30, 2020

Uh oh!

johanvos commented Jul 30, 2020

Uh oh!

openjdk bot commented Jul 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

abhinayagarwal commented May 14, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented May 14, 2020 •

edited

Loading

openjdk bot commented May 29, 2020 •

edited

Loading

mipastgt commented May 29, 2020 •

edited

Loading

abhinayagarwal commented Jun 12, 2020 •

edited

Loading