906499 - Caret visibility persists after form submission

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Description

[marcsances]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:23.0) Gecko/20100101 Firefox/23.0 (Beta/Release)
Build ID: 20130814063812

Steps to reproduce:

1. Enter Twitter app, for example.
2. Focus a text field (like the tweet field), type, then tap in some position of the text field so the cursor arrow shows.
3. Press submit [tweet] button.


Actual results:

The cursor arrow keeps on screen after the form has been sent.

Only happens in applications, normal websites work as expected.


Expected results:

The cursor arrow should have gone by the time the textbox lost focus.

Comment 1

[Aaron Train [:aaronmt]]

Which version of Firefox are you using? Which device and Android version? Do you see any difference when testing out Nightly for Android (http://nightly.mozilla.org)?

Comment 2

[marcsances]

Actually, the problem is happening on Nightly.

I have Android 2.3.6, 320x240 resolution, ARMv6 processor.

Comment 3

[marcsances]

This happened also in Marketplace:

1. Go to Marketplace
2. Make a search
3. Focus searchbox so caret shows
4. Tap the back button
5. Caret keeps in Marketplace homescreen

Comment 4

[marcsances]

Happened in Facebook website also, it's not only application-wide, but sometimes it does disappear as expected.

Comment 5

[Aaron Train [:aaronmt]]

Oh this is a duplicate of bug 903316.

Comment 6

[Mark Capella [:capella]]

mmm ... not sure this is the same exact thing ... I don't use twitter and can't repro the STR on marketplace

Comment 7

[Aaron Train [:aaronmt]]

Might be a mixup here between the caret and text handles

Comment 8

[marcsances]

It's a similar bug but not exactly the same, I didn't delete anything. It may have the same source though.

Comment 9

[Mark Capella [:capella]]

I pushed a test build to TRY for what I believe solves bug 903316 ... can you see if it fixes this bus? Or maybe detail STR in Marketplace if different than comment 3 ?

When I try I:
0) Tap Menu -> Tools -> Apps
1) I have no apps installed, so I click "Get some from the Firefox Marketplace"
1) The Marketplace page opens
2) I tap in the search field
3) Keyboard opens, caret appears, cursor blinks in field
4) I tap the hardware back key
5) Keyboard hides, caret remains, cursor still blinks
6) If I tap back again, caret hides, and I return to the App page

This appears normal to me  (?)

Comment 10

[marcsances]

I didn't mean the hardware back key, you open the keyboard, then tap the back icon in the webpage, at the left of the text field.

Comment 11

[Mark Capella [:capella]]

Hmmm ... I'm using a phone / not tablet ... do you mean like the back arrow that shows in the tablet to the left of the awesomebar search field?

To the left of the marketplace search field I have a marketplace icon ...

If I tap that, the KEB hides, the caret stays visible and the blinking cursor hides ...

If that's what you're thinking is wrong, yes I think it's an issue also.

Comment 12

[Mark Capella [:capella]]

Attachment: bug906499

A tiny fix ...

Comment 13

[:Margaret Leibovic]

Comment on attachment 792148 [details] [diff] [review]
bug906499

Review of attachment 792148 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/SelectionHandler.js
@@ +74,5 @@
>              this.copySelection();
>            else
>              this._closeSelection();
> +        } else if (this._activeType == this.TYPE_CURSOR) {
> +          this._closeSelection();

Can you give me a bit of an explanation about this change? What happens if you tap on a different part of the text in the input box? Will the caret/cursor move to that new position?

The caret is attached in a Gesture:SingleTap handler in browser.js:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#4254

So if the caret still works with this patch here, it means we're relying on the fact that this observer gets called first, and that seems fragile to me.

Comment 14

[Mark Capella [:capella]]

You are awesome, this is exactly what happens. Since our observer here is added last, it gets called first when we tap into the same editable field.

So we close it, and hide the handles. The browser.js grabs the same message and re-positions / starts a new selection.

Comment 15

[Mark Capella [:capella]]

Forgot to add, I'm not sure this is fragile, as much as predictable. Say there's a page with two input fields, we need to close the first caret and jump to the second if user taps one to the other.

Comment 16

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #15)
> Forgot to add, I'm not sure this is fragile, as much as predictable. Say
> there's a page with two input fields, we need to close the first caret and
> jump to the second if user taps one to the other.

Yes, you actually just discovered another bug there, which is that we don't remove listeners in that case. We call _closeSelection at the top of startSelection, but not at the top of attachCaret.

Comment 17

[:Margaret Leibovic]

Comment on attachment 792148 [details] [diff] [review]
bug906499

Review of attachment 792148 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/SelectionHandler.js
@@ +73,5 @@
>            if (this._pointInSelection(data.x, data.y))
>              this.copySelection();
>            else
>              this._closeSelection();
> +        } else if (this._activeType == this.TYPE_CURSOR) {

Please add a comment here about how attachCaret is called in the the "Gesture:SingleTap" handler in BrowserEventHandler, but that we're guaranteed to call this first because this observer was added last.

Comment 18

[Mark Capella [:capella]]

hehe :-) There's a lot of little things I'd like to do in this whole area, but it might be better to sneak up on it patch by patch to be sure I / we understand it all.

I don't like how we use the closeSelection() in a catch-all fashion, rather than having it do a closeSelectionDownToCaret() and closeSelectionDownToNone() routine for example which could also then be called individually in some cases in a more precise fashion.

Comment 19

[Mark Capella [:capella]]

Good point re: comments (added)
Another small one down and on to the bigger ones
https://hg.mozilla.org/integration/fx-team/rev/996bff87f926

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/996bff87f926