636792 - Coalesce keyboard pan with zoom to element pan

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Panning/Zooming, defect)

Description

[Benjamin Stover (:stechz)]

The technicalities of this might be annoying to get right, but form assistant would feel soooo much better if it anticipated the keyboard coming up before transitioning to the next for element and vise versa. I promise.

Comment 1

[Benjamin Stover (:stechz)]

Also, we need to bring the keyboard up/down after the animation has finished. Our resize takes too long.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

I don't think this blocks 2.0, but certainly is something we should add. This plays into something Vivien once mentioned - about keeping the keyboard visible while navigating a form. So, once the keyboard becomes visible, we keep it visible until the users taps focus off a widget or uses the back button to close the keyboard.

Comment 3

[Benjamin Stover (:stechz)]

This is not what I meant, though that would work too (might be better?). But it is key that the keyboard is not changing during our animation.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Hey I'm planning to fix bug 636339 before 2.0. The plan is to have the keyboard _always_ open while handling forms, this will allow you to navigate through list and to use the "next" key on all elements (in such a case the navigation buttons will be hidden on Android)

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

The patch use the new Util.isKeyboardOpened method and add an event generated when the state of the keyboard change.

This prevent doing extra zooming operations when the keyboard is shown.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 533683 [details] [diff] [review]
Patch

Make sure you test this on a few devices. I am worried about race conditions with the KeyboardChanged event. Your current code checks to make sure the keyboard is not open before listening for the event, but I still worry.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #7)
> Comment on attachment 533683 [details] [diff] [review] [review]
> Patch
> 
> Make sure you test this on a few devices. I am worried about race conditions
> with the KeyboardChanged event. Your current code checks to make sure the
> keyboard is not open before listening for the event, but I still worry.

I understand why you are worried. 
I can still add a timeout, so if the event never fires or we miss the form assistant will still react, what do you think?

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Let's land it as is for now. No need to complicate the code unless we see some problems during testing.

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

http://hg.mozilla.org/mozilla-central/rev/e8d3246d2f2f

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I have broke the browser_autocomplete.js test with this change, I'll add a followup.

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch - followup

The followup ensure the form assistant has received and process the opening message before sending an autocomplete message to the form assistant :)

This also prevent doing the test on desktop where there is no such thing, otherwise the show method wait for a KeyboardChanged event that is never fired...

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 534795 [details] [diff] [review]
Patch - followup

>+  timer.interval(100, function() {

This makes me cry

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #13)
> Comment on attachment 534795 [details] [diff] [review] [review]
> Patch - followup
> 
> >+  timer.interval(100, function() {
> 
> This makes me cry

This is basically a waitFor! Maybe I should write such a method for the content side?

Comment 15

[Phil Ringnalda (:philor)]

I pushed that to try (along with some other stuff), and it seems to make Android time out even earlier, after the initial "Click on a datalist element and show suggestions" step rather than after the current "Check arrows visibility" step.

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #15)
> I pushed that to try (along with some other stuff), and it seems to make
> Android time out even earlier, after the initial "Click on a datalist
> element and show suggestions" step rather than after the current "Check
> arrows visibility" step.

I have simplified the test because of bug 659651, should works now.
http://hg.mozilla.org/mozilla-central/rev/74ec824bffe6

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

gasp, still not work like what I want on device...

Comment 18

[Phil Ringnalda (:philor)]

Or off, since now you've got two TEST-UNEXPECTED-PASS errors on desktop ;)

Comment 19

[Andreea Pod]

I tried to verify this bug using steps from bug 640754 which is duplicate of this bug. 

When I tap on the search field in google.com everything it's ok but when I hit back or tap outside the vkb to close it I still can see the black flicker. I see Bug 627557 is new. 
Should I close this bug or Bug 627557 as dupe of this and reopen this one?

Comment 20

[Andreea Pod]

Verified fixed on Firefox 6 Beta 2: Mozilla /5.0 (Android;Linux armv7l;rv:6.0) Gecko/20110713 Firefox/6.0 Fennec/6.0

Comment 21

[Andreea Pod]

 Mozilla /5.0 (Android;Linux armv7l;rv:6.0) Gecko/20110713 Firefox/6.0 Fennec/6.0