Closed Bug 1464096 Opened Last year Closed Last year

SessionTextInput.Delegate showKeyboard and hideKeyboard appear to be fired inconsistently.

Categories

(GeckoView :: General, enhancement, P1)

enhancement

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: rbarker, Assigned: jchen)

Details

(Whiteboard: [geckoview:fxr])

Attachments

(6 files)

While attempting to utilized the SessionTextInput.Delegate class in FxR, we are seeing showKeyboard and hideKeyboard be called inconsistently. When loading a page with a text box, showKeyboard was expected to fire when the text box gains focus and then hideKeyboard was expected to fire when clicking outside of the text box.

Instead, showKeyboard does not seem to fire until the text box is clicked a second time and hideKeyboard seems to almost never fire. Logging showed it would fire on occasion but it was not clear what action was actually triggering the callback. The branch that implements the Delegate can be found here:

https://github.com/MozillaReality/FirefoxReality/tree/text-input-delegate
After further testing, the only time I see hideKeyboard fire is when the View attached to the session gains focus. Since the View attached to the session is mostly stubbed out is it possible we are missing some functionality?
Whiteboard: [geckoview:fxr]
Assignee: nobody → nchen
Priority: -- → P1
(In reply to Randall Barker [:rbarker] from comment #1)
> After further testing, the only time I see hideKeyboard fire is when the
> View attached to the session gains focus. Since the View attached to the
> session is mostly stubbed out is it possible we are missing some
> functionality?

Ah we have a focus check at [1]. Maybe worth trying commenting out.

[1] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoInputConnection.java#874.
Comment on attachment 8982014 [details]
Bug 1464096 - 1. Add session parameter to all SessionTextInput.Delegate methods;

https://reviewboard.mozilla.org/r/248060/#review254224
Attachment #8982014 - Flags: review?(esawin) → review+
Comment on attachment 8982015 [details]
Bug 1464096 - 2. Move restart/show/hide input functionality to GeckoEditable;

https://reviewboard.mozilla.org/r/248062/#review254226

r+ with nits.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java:1229
(Diff revision 1)
> -                    mFocusedChild = null;
> +                mFocusedChild = null;
> +                break;
> +
> +            case SessionTextInput.EditableListener.NOTIFY_IME_OPEN_VKB:
> +                toggleSoftInput(/* force */ true);
> +                return;

We call break for some cases but return for others (hence never calling notifyIME). Is this intended?

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoEditable.java:1370
(Diff revision 1)
> +        }
> +
> +        outAttrs.inputType = InputType.TYPE_CLASS_TEXT;
> +        if (state == SessionTextInput.EditableListener.IME_STATE_PASSWORD ||
> +                "password".equalsIgnoreCase(typeHint))
> +            outAttrs.inputType |= InputType.TYPE_TEXT_VARIATION_PASSWORD;

Please always use curly braces.
Attachment #8982015 - Flags: review?(esawin) → review+
Comment on attachment 8982016 [details]
Bug 1464096 - 3. Remove SessionTextInput.isInputActive;

https://reviewboard.mozilla.org/r/248064/#review254234
Attachment #8982016 - Flags: review?(esawin) → review+
Comment on attachment 8983509 [details]
Bug 1464096 - 4. Move SessionTextInput.Delegate to GeckoSession.TextInputDelegate;

https://reviewboard.mozilla.org/r/249366/#review255540
Attachment #8983509 - Flags: review+
Comment on attachment 8983510 [details]
Bug 1464096 - 5. Allow key events when there is no View;

https://reviewboard.mozilla.org/r/249368/#review255542
Attachment #8983510 - Flags: review+
Comment on attachment 8982015 [details]
Bug 1464096 - 2. Move restart/show/hide input functionality to GeckoEditable;

https://reviewboard.mozilla.org/r/248062/#review254226

> We call break for some cases but return for others (hence never calling notifyIME). Is this intended?

Yep intended but I added some comments to clarify.
Comment on attachment 8983576 [details]
Bug 1464096 - 6. Add TextInputDelegate support in GeckoSessionTestRule;

https://reviewboard.mozilla.org/r/249436/#review255618
Attachment #8983576 - Flags: review+
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/91c731643ad9
1. Add session parameter to all SessionTextInput.Delegate methods; r=esawin
https://hg.mozilla.org/integration/autoland/rev/43e114b91a02
2. Move restart/show/hide input functionality to GeckoEditable; r=esawin
https://hg.mozilla.org/integration/autoland/rev/20c2e0dcc01a
3. Remove SessionTextInput.isInputActive; r=esawin
https://hg.mozilla.org/integration/autoland/rev/f4a4552cb530
4. Move SessionTextInput.Delegate to GeckoSession.TextInputDelegate; r=jchen
https://hg.mozilla.org/integration/autoland/rev/5fb83a8f09f5
5. Allow key events when there is no View; r=jchen
https://hg.mozilla.org/integration/autoland/rev/a7e33fa24cdd
6. Add TextInputDelegate support in GeckoSessionTestRule; r=jchen
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 62 → mozilla62
You need to log in before you can comment on or make changes to this bug.