Last Comment Bug 636792 - Coalesce keyboard pan with zoom to element pan
: Coalesce keyboard pan with zoom to element pan
Status: VERIFIED FIXED
:
Product: Fennec Graveyard
Classification: Graveyard
Component: Panning/Zooming (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
:
:
Mentors:
: 640754 (view as bug list)
Depends on: 661997
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-25 11:20 PST by Benjamin Stover (:stechz)
Modified: 2011-09-06 07:24 PDT (History)
7 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (3.01 KB, patch)
2011-05-19 09:57 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
mark.finkle: review+
Details | Diff | Splinter Review
Patch - followup (1.79 KB, patch)
2011-05-24 09:24 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
mark.finkle: review+
Details | Diff | Splinter Review

Description Benjamin Stover (:stechz) 2011-02-25 11:20:49 PST
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) 2011-02-25 11:24:42 PST
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?) 2011-02-25 11:33:35 PST
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) 2011-02-25 11:35:37 PST
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) 2011-02-25 14:38:58 PST
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 5 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-05-18 03:35:38 PDT
*** Bug 640754 has been marked as a duplicate of this bug. ***
Comment 6 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-05-19 09:57:09 PDT
Created attachment 533683 [details] [diff] [review]
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?) 2011-05-19 11:42:58 PDT
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) 2011-05-19 14:03:37 PDT
(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?) 2011-05-19 16:17:48 PDT
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) 2011-05-20 03:20:08 PDT
http://hg.mozilla.org/mozilla-central/rev/e8d3246d2f2f
Comment 11 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-05-24 08:37:41 PDT
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) 2011-05-24 09:24:52 PDT
Created attachment 534795 [details] [diff] [review]
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?) 2011-05-24 10:23:43 PDT
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) 2011-05-25 03:20:43 PDT
(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) 2011-05-29 09:34:42 PDT
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) 2011-05-30 04:53:02 PDT
(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) 2011-05-30 08:14:15 PDT
gasp, still not work like what I want on device...
Comment 18 Phil Ringnalda (:philor) 2011-05-30 10:34:31 PDT
Or off, since now you've got two TEST-UNEXPECTED-PASS errors on desktop ;)
Comment 19 Andreea Pod 2011-07-20 05:50:54 PDT
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 2011-07-20 05:54:27 PDT
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 2011-07-20 05:56:10 PDT
 Mozilla /5.0 (Android;Linux armv7l;rv:6.0) Gecko/20110713 Firefox/6.0 Fennec/6.0

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