Closed Bug 1464096 Opened 6 years ago Closed 6 years ago

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

Tracking

(firefox62 fixed)

Status:

RESOLVED FIXED

Milestone:

mozilla62

Tracking Flags:

Tracking

Status

firefox62

---

fixed

People

(Reporter: rbarker, Assigned: jchen)

Details

(Whiteboard: [geckoview:fxr])

Attachments

(6 files)

Bug 1464096 - 1. Add session parameter to all SessionTextInput.Delegate methods; 6 years ago (inactive) Jim Chen [:jchen] [:darchons] 59 bytes, text/x-review-board-request	esawin : review+	Details
Bug 1464096 - 2. Move restart/show/hide input functionality to GeckoEditable; 6 years ago (inactive) Jim Chen [:jchen] [:darchons] 59 bytes, text/x-review-board-request	esawin : review+	Details
Bug 1464096 - 3. Remove SessionTextInput.isInputActive; 6 years ago (inactive) Jim Chen [:jchen] [:darchons] 59 bytes, text/x-review-board-request	esawin : review+	Details
Bug 1464096 - 4. Move SessionTextInput.Delegate to GeckoSession.TextInputDelegate; 6 years ago (inactive) Jim Chen [:jchen] [:darchons] 59 bytes, text/x-review-board-request		Details
Bug 1464096 - 5. Allow key events when there is no View; 6 years ago (inactive) Jim Chen [:jchen] [:darchons] 59 bytes, text/x-review-board-request		Details
Bug 1464096 - 6. Add TextInputDelegate support in GeckoSessionTestRule; 6 years ago (inactive) Jim Chen [:jchen] [:darchons] 59 bytes, text/x-review-board-request	jchen : review+	Details

Randall Barker [:rbarker]

Reporter

Description

•

6 years ago

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

Randall Barker [:rbarker]

Reporter

Comment 1

•

6 years ago

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?

Randall Barker [:rbarker]

Reporter

Updated

•

6 years ago

Whiteboard: [geckoview:fxr]

David Bolter [:davidb] (NeedInfo me for attention)

Updated

•

6 years ago

Assignee: nobody → nchen

Priority: -- → P1

(inactive) Jim Chen [:jchen] [:darchons]

Assignee

Comment 2

•

6 years ago

(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 hidden (mozreview-request)

Follow the GV convention and add a session parameter to all methods in
SessionTextInput.Delegate.

Review commit: https://reviewboard.mozilla.org/r/248060/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248060/

Comment hidden (mozreview-request)

Right now all the code to restart/show/hide the input method is in
GeckoInputConnection. However, that means the corresponding
SessionTextInput.Delegate methods are not called when
GeckoInputConnection is not available, typically due to us not having a
View. This patch moves all that code to GeckoEditable or
SessionTextInput, so that those delegate methods are always called even
if we don't have a View / GeckoInputConnection.

Review commit: https://reviewboard.mozilla.org/r/248062/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248062/

Comment hidden (mozreview-request)

The SessionTextInput.isInputActive API is prone to races. Its
functionality can be replicated through the
SessionTextInput.Delegate.restartInput callback, so it's okay to remove
it.

Review commit: https://reviewboard.mozilla.org/r/248064/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248064/

Eugen Sawin [:esawin]

Comment 6

•

6 years ago

mozreview-review

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+

Eugen Sawin [:esawin]

Comment 7

•

6 years ago

mozreview-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+

Eugen Sawin [:esawin]

Comment 8

•

6 years ago

mozreview-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 hidden (mozreview-request)

I think it's more consumer-friendly to put the text input delegate in
GeckoSession alongside all other delegates.

Review commit: https://reviewboard.mozilla.org/r/249366/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/249366/

Comment hidden (mozreview-request)

Allow key events to pass through to Gecko when there is no text input
View.

Review commit: https://reviewboard.mozilla.org/r/249368/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/249368/

Comment hidden (mozreview-request)

Comment on attachment 8982014 [details]
Bug 1464096 - 1. Add session parameter to all SessionTextInput.Delegate methods;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248060/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8982015 [details]
Bug 1464096 - 2. Move restart/show/hide input functionality to GeckoEditable;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248062/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8982016 [details]
Bug 1464096 - 3. Remove SessionTextInput.isInputActive;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248064/diff/1-2/

(inactive) Jim Chen [:jchen] [:darchons]

Assignee

Comment 14

•

6 years ago

mozreview-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+

(inactive) Jim Chen [:jchen] [:darchons]

Assignee

Comment 15

•

6 years ago

mozreview-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 hidden (mozreview-request)

Comment on attachment 8982014 [details]
Bug 1464096 - 1. Add session parameter to all SessionTextInput.Delegate methods;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248060/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8982015 [details]
Bug 1464096 - 2. Move restart/show/hide input functionality to GeckoEditable;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248062/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8982016 [details]
Bug 1464096 - 3. Remove SessionTextInput.isInputActive;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248064/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8983509 [details]
Bug 1464096 - 4. Move SessionTextInput.Delegate to GeckoSession.TextInputDelegate;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249366/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8983510 [details]
Bug 1464096 - 5. Allow key events when there is no View;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249368/diff/1-2/

(inactive) Jim Chen [:jchen] [:darchons]

Assignee

Comment 21

•

6 years ago

mozreview-review-reply

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 hidden (mozreview-request)

Support TextInputDelegate for use in tests.

Review commit: https://reviewboard.mozilla.org/r/249436/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/249436/

Comment hidden (mozreview-request)

Comment on attachment 8982014 [details]
Bug 1464096 - 1. Add session parameter to all SessionTextInput.Delegate methods;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248060/diff/3-4/

Comment hidden (mozreview-request)

Comment on attachment 8982015 [details]
Bug 1464096 - 2. Move restart/show/hide input functionality to GeckoEditable;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248062/diff/3-4/

Comment hidden (mozreview-request)

Comment on attachment 8982016 [details]
Bug 1464096 - 3. Remove SessionTextInput.isInputActive;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248064/diff/3-4/

Comment hidden (mozreview-request)

Comment on attachment 8983509 [details]
Bug 1464096 - 4. Move SessionTextInput.Delegate to GeckoSession.TextInputDelegate;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249366/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8983510 [details]
Bug 1464096 - 5. Allow key events when there is no View;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249368/diff/2-3/

(inactive) Jim Chen [:jchen] [:darchons]

Assignee

Comment 28

•

6 years ago

mozreview-review

Comment on attachment 8983576 [details]
Bug 1464096 - 6. Add TextInputDelegate support in GeckoSessionTestRule;

https://reviewboard.mozilla.org/r/249436/#review255618

Attachment #8983576 - Flags: review+

Pulsebot

Comment 29

•

6 years ago

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

Narcis Beleuzu [:NarcisB]

Comment 30

•

6 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/91c731643ad9
https://hg.mozilla.org/mozilla-central/rev/43e114b91a02
https://hg.mozilla.org/mozilla-central/rev/20c2e0dcc01a
https://hg.mozilla.org/mozilla-central/rev/f4a4552cb530
https://hg.mozilla.org/mozilla-central/rev/5fb83a8f09f5
https://hg.mozilla.org/mozilla-central/rev/a7e33fa24cdd

Status: NEW → RESOLVED

Closed: 6 years ago

status-firefox62: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → Firefox 62

BMO Automation

Updated

•

5 years ago

Product: Firefox for Android → GeckoView

Dylan Hardison [:dylan] (he/him)

Updated

•

5 years ago

Target Milestone: Firefox 62 → mozilla62

You need to log in before you can comment on or make changes to this bug.