Closed Bug 912033 Opened 6 years ago Closed 6 years ago

[Keyboard] [New IME] Pop down animation is broken

Categories

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

x86
macOS
defect
Not set

Tracking

(blocking-b2g:koi+, b2g-v1.2 fixed)

RESOLVED FIXED
1.2 C3(Oct25)
blocking-b2g koi+
Tracking Status
b2g-v1.2 --- fixed

People

(Reporter: janjongboom, Assigned: janjongboom)

References

Details

(Whiteboard: [ft:system-platform], [3rd-party-keyboard])

Attachments

(1 file)

46 bytes, text/x-github-pull-request
GaryChen
: review+
Details | Review
Based on the CSS there should be a pop down animation just like the pop up anim for keyboard, but it does not work at the moment.
blocking-b2g: --- → koi?
Whiteboard: [ft:system-platform]
Need to fix
Assignee: nobody → rlu
blocking-b2g: koi? → koi+
Assignee: rlu → janjongboom
Attached file Patch
It's dependent on bug 912028 because of the more sane flow introduced there, plus the tests.

What it does:

1. Don't hide the iframe where the 3rd party keyboard lives until pop down anim is played
2. Change animation to ease-in for pop down because you want slow->fast instead of other way around when disappearing

I tested this on GP Keon with mc and it looks nice and smooth.
Attachment #811093 - Flags: review?(rlu)
Attachment #811093 - Flags: review?(rlu) → review?(dflanagan)
Attachment mime type: text/plain → text/x-github-pull-request
Comment on attachment 811093 [details] [review]
Patch

Basically, this works and the animation looks good when hiding the keyboard.
Jan, thanks for the great work (again).

However, I noticed that there is a case that when you press [Home] button to make an app enter background, the keyboard would be hidden as well.
In this case, we did not handle the keyboard hiding good enough so the homescreen would be seen half-sized before the app is hidden. 
I am not sure when this is broken.

Do you think we can address that in this bug or we could open another bug to fix that?
Attachment #811093 - Flags: review?(dflanagan) → review+
Comment on attachment 811093 [details] [review]
Patch

I added an immediate hiding function to the PR. It's called hideKeyboardImmediately, please re-r? :-)
Attachment #811093 - Flags: review+ → review?(rlu)
Hi Jan,

Would you please assign one of the target milestones (1.2 C3 ~ 1.2 C6) for this bug depending on time you need for resolving this one? We need the flag to check when this bug will be resolved. Thank you!

Ivan
Flags: needinfo?(janjongboom)
Flags: needinfo?(janjongboom)
Target Milestone: --- → 1.2 C3(Oct25)
Comment on attachment 811093 [details] [review]
Patch

Why is adding "notransition" necessary here?
I don't remember we have that in v1-train for immediately hiding the keyboard.

Could you help me understand it in more detail?
Thanks.
Attachment #811093 - Flags: review?(rlu)
Comment on attachment 811093 [details] [review]
Patch

Because the pop down transition has always been broken, also on v1-train. Ergo: the problem of the transition playing when switching apps is a new one that appears due to fixing this bug.
Attachment #811093 - Flags: review?(rlu)
Status: NEW → ASSIGNED
Whiteboard: [ft:system-platform] → [ft:system-platform], [3rd-party-keyboard]
Comment on attachment 811093 [details] [review]
Patch

It seems with this patch, the keyboard won't hide after it lose the focus?

Jan,

Could you please help check what went wrong?
Thank you.

STR
===
1. Open contact app
2. Click "+" to add a contact.
3. Click on the input field.
4. Click on the header or the [x] on the upper left => The keyboard won't be dismissed.
Attachment #811093 - Flags: review?(rlu)
Hm, it's any issue that shows sometimes. It's because we get mozbrowserresize very often, and sometimes even after we hide the keyboard due to blur. However: when that event comes in:

    if (this.keyboardFrameContainer.classList.contains('hide') ||
        this.keyboardFrameContainer.dataset.transitionIn === 'true') {
      this.showKeyboard(updateHeight);

we cancel the blur and show it. So i think we should never open the keyboard if mozbrowserresize tells us to, but always rely on focus/blur events. I'll see if it has any nasty side effects.
You get mozbrowserresize because the keyboard app requested it. Maybe it's a issue that the keyboard requests too much. However I think this event should not prevent the keyboard manager to show/hide the keyboard so I agree that we always rely on focus/blur events.
Depends on: 931005
Comment on attachment 811093 [details] [review]
Patch

Alright, all should be well now. There is an issue where (sometimes) the keyboard used to flicker, but because of changed behavior it now starts to slide down and then slides up immediately after switching elements. That's addressed in bug 931005.

Changes in window_manager were required so we hide the keyboard before closing the app, and the app will resize first so we don't hide a half app (cause keyboard already closed and the app isn't thus leaving half the screen open).
Attachment #811093 - Flags: review?(rlu)
Comment on attachment 811093 [details] [review]
Patch

Ask for Gary's help to evaluate this patch.
Attachment #811093 - Flags: review?(rlu) → review?(gchen)
Hi Jan,
   I've comment on github, looking forward to discuss with you on github.

   nice work!
Flags: needinfo?(janjongboom)
Responded on GH (and rebased against master as well).
Flags: needinfo?(janjongboom)
Comment on attachment 811093 [details] [review]
Patch

r=me,
thanks for your great work!
Attachment #811093 - Flags: review?(gchen) → review+
Landed in https://github.com/mozilla-b2g/gaia/commit/a1c1ac4c03367291b29d2b5bc62a80196cfbb71a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
I was not able to uplift this bug to v1.2.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1.2, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1.2
  git cherry-pick -x -m1 a1c1ac4c03367291b29d2b5bc62a80196cfbb71a
  <RESOLVE MERGE CONFLICTS>
  git commit
Flags: needinfo?(janjongboom)
Hi Jan,
   I've fixed the conflict on this pull request and I am waiting for travis. (https://github.com/mozilla-b2g/gaia/pull/13465)
   Do you want to do it by yourself or just use my pr?
Merge it!
Flags: needinfo?(janjongboom)
You need to log in before you can comment on or make changes to this bug.