785663 - [keyboard]When IME appears, focused input element should not be hidden by keyboard

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Justin Lebar (not reading bugmail)]

This was originally filed as https://github.com/mozilla-b2g/gaia/issues/3765, but we established that it's a platform bug.

(Ben Francis asked)
> [Could we] modify gecko so that iframes automatically scroll to make focussed 
> inputs visible?

(I replied)
> I like the idea of doing this automagically, but I'm not sure we could do it
> quite right from a Gecko perspective. In Gecko, it might be sane to ensure
> that we don't move a focused input field off-screen when we resize an iframe,
> but it's probably not sane to say that whenever an iframe is resized, we
> ensure that the iframe is scrolled so that the focused input field is
> onscreen. That is, if the focused input field is offscreen before you resize,
> the general automagic Gecko case should probably leave it offscreen after you
> resize.
> 
> Anyway, we have b2g-specific code in Gecko which detects when an input element
> is focused (to trigger showing the IME), so it shouldn't be too hard to hack
> that code to also ensure that the input element is on-screen.

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

This was fixed by me at bug 759238. Not sure what happen since.

Comment 2

[Ben Francis [:benfrancis]]

Could this be an OOP-related regression?

Tim's code works in the test app, but not inside browser tabs.

Rudy, would you mind taking a look?

Comment 3

[Andrew Overholt [:overholt]]

This is a big usability issue IMO so blocking.

Comment 4

[Andrew Overholt [:overholt]]

Rudy, if you're the proper owner here, feel free to re-assign.

Comment 5

[Justin Lebar (not reading bugmail)]

I have cycles; I might as well take a stab at this.

Comment 6

[Justin Lebar (not reading bugmail)]

I think this is simple once bug 780395 (née bug 786780) is fixed.

Comment 7

[Chris Lee [:clee]]

This seems like a P1 blocker given the usability impact.  Updating whiteboard.

Comment 8

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1

This doesn't work quite right because if the element is in view, it's
scrolled to the bottom of the screen!  But it's the right idea, I think.

Comment 9

[Justin Lebar (not reading bugmail)]

Comment on attachment 661457 [details] [diff] [review]
Patch, v1

Actually, I don't think it's so bad that we scroll the textbox to the bottom of the screen when we show the IME.  That way, the input area is always in a predictable place.

Anyway I can't seem to figure out how to tell if the element is on-screen after the resize, so this is the best I can do right now without some help.

Comment 10

[Justin Lebar (not reading bugmail)]

Vivien, we started talking about this bug on IRC, but then we got distracted...  What's up?

Comment 11

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

Comment on attachment 661457 [details] [diff] [review]
Patch, v1

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

I think we may need to fix what happen when there is a 'resize' event. It seems like 'resize' is called a lof of times when the browser size change because of the keyboard beeing displayed. I wonder if that the main cause of the bug and if we need to coalesce the calls to element.scrollIntoView there. Also I wonder if one of the issue is not the fact that Gecko think that the field is viewable but the zoom level has moved it out of view. In this case I don't think this patch would help :/

Comment 12

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

Comment on attachment 661457 [details] [diff] [review]
Patch, v1

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

I think we may need to fix what happen when there is a 'resize' event. It seems like 'resize' is called a lof of times when the browser size change because of the keyboard beeing displayed. I wonder if that the main cause of the bug and if we need to coalesce the calls to element.scrollIntoView there. Also I wonder if one of the issue is not the fact that Gecko think that the field is viewable but the zoom level has moved it out of view. In this case I don't think this patch would help :/

r- because it seems like a workaround that won't work at some point.

Comment 13

[Justin Lebar (not reading bugmail)]

> In this case I don't think this patch would help :/

I've tested this patch and it works fine.

I thought we established on IRC that the problem is that we resize the window before we show the IME.  Therefore the resize handler does nothing.

We could remove the current ineffective resize handler, which would make this less of a hack, except I recall you said on IRC that we still wanted it, to cover the case when we rotate the screen and resize the window.

Comment 14

[Justin Lebar (not reading bugmail)]

<vingtetun> jlebar: hey! can you try to go to http://vingtetun.org/tmp/forms.html, put one of the text input field next to the bottom of the screen, zoom as much as you can and hit the field? (with your patch)
<jlebar> vingtetun, I don't have a working build at the moment, but I can try in an hour or two.

Comment 15

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

(In reply to Justin Lebar [:jlebar] from comment #14)
> <vingtetun> jlebar: hey! can you try to go to
> http://vingtetun.org/tmp/forms.html, put one of the text input field next to
> the bottom of the screen, zoom as much as you can and hit the field? (with
> your patch)
> <jlebar> vingtetun, I don't have a working build at the moment, but I can
> try in an hour or two.

Did it worked?

Comment 16

[Justin Lebar (not reading bugmail)]

Sorry, I got distracted by push notifications.  And that will continue to distract me for much of tomorrow, I'm afraid.

I'll get to this as soon as I can, either Monday or Tuesday.  Sorry again.  :(

Comment 17

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

(In reply to Justin Lebar [:jlebar] from comment #16)
> Sorry, I got distracted by push notifications.  And that will continue to
> distract me for much of tomorrow, I'm afraid.
> 
> I'll get to this as soon as I can, either Monday or Tuesday.  Sorry again. 
> :(

No worries. I'm laggy too and I think this can wait for monday :)

Comment 18

[Justin Lebar (not reading bugmail)]

Vivien, my device's homescreen is empty save for the search icon, and I can't connect to wifi.

If you have the capability of making a functional build, you'd probably have a much faster time testing this than me.

Comment 19

[Justin Lebar (not reading bugmail)]

Intriguingly, everything worked as it was supposed to on my desktop build.  Sorry I can't test on device, but I think it's probably not worth not spend hours getting my device to work to do a five-second test.  Too bad there's no way to check out a known-good rev of all 90 of our subrepositories.

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We're trying to create a "nightly" branch with tested commits, but it's having a rough time in review.

Comment 21

[Justin Lebar (not reading bugmail)]

> Intriguingly, everything worked as it was supposed to on my desktop build.

Sorry, I should say, everything worked fine /without the patch here/.

Comment 22

[Justin Lebar (not reading bugmail)]

I still do not have a functioning device build, but I'm working on it.

Comment 23

[Justin Lebar (not reading bugmail)]

Comment on attachment 661457 [details] [diff] [review]
Patch, v1

I finally got a device build working, and this patch no longer works.  It appears that we resize the content iframe a number of times when we show the IME, and the scroll doesn't happen at the right time.

Can we kick this off to someone who actually understands our IME?  Tim, maybe?

Comment 24

[Justin Lebar (not reading bugmail)]

Tim, do you think you're the right person to look at this?

Comment 25

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

Assigning to Chris Peterson.

Comment 26

[Andrew Overholt [:overholt]]

How are things going on a fix here, Chris?  Are we waiting on the non-blocking-basecamp part of bug 780395?

Comment 27

[Chris Peterson [:cpeterson]]

Andrew, I was waiting for djf to land a big refactoring. Now that he has landed it, I can continue my investigation.

Comment 29

[Chris Peterson [:cpeterson]]

Attachment: scrollIntoView-after-resize.patch

Wait for keyboard to stop resizing before scrolling text input into view. We may receive multiple resize events in quick succession, so wait a bit before scrolling the input element into view.

I don't know why forms.js receive 2-3 resize events, but if I scrollIntoView() when handling any or all of the resize events, the text input does not actually move.

The 20 ms delay is a magic number. Even delaying the scrollIntoView() 1 ms makes the bug "go away".

This patch also fixes email textarea bug 798695.

Comment 30

[David Flanagan [:djf]]

Comment on attachment 680277 [details] [diff] [review]
scrollIntoView-after-resize.patch

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

Looks good. Note that I have not tested myself, though.

::: b2g/chrome/content/forms.js
@@ +50,5 @@
>  
>    isKeyboardOpened: false,
>    selectionStart: 0,
>    selectionEnd: 0,
> +  scrollIntoViewTimeout: null,

to be parallel, it would be nice to have the blurTimeout here, too. But not required to land.

Comment 31

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3de447b1c396

Comment 32

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a498c73be0fa

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3de447b1c396