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

RESOLVED FIXED

Status

Firefox OS
General
P1
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: cpeterson)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox17 wontfix, firefox18 fixed, firefox19 fixed)

Details

(Whiteboard: [LOE:S][WebAPI:P1][qa+])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Whiteboard: [lang=js][mentor=jlebar]
(Reporter)

Updated

6 years ago
blocking-basecamp: --- → ?
This was fixed by me at bug 759238. Not sure what happen since.
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?
This is a big usability issue IMO so blocking.
blocking-basecamp: ? → +
Rudy, if you're the proper owner here, feel free to re-assign.
Assignee: nobody → rlu
(Reporter)

Comment 5

6 years ago
I have cycles; I might as well take a stab at this.
Assignee: rlu → justin.lebar+bug
Whiteboard: [lang=js][mentor=jlebar]
(Reporter)

Updated

6 years ago
Whiteboard: [LOE:S]
(Reporter)

Updated

6 years ago
Depends on: 786780
(Reporter)

Comment 6

6 years ago
I think this is simple once bug 780395 (née bug 786780) is fixed.
Depends on: 780395
No longer depends on: 786780
Whiteboard: [LOE:S] → [LOE:S][WebAPI:P2]

Comment 7

5 years ago
This seems like a P1 blocker given the usability impact.  Updating whiteboard.
Whiteboard: [LOE:S][WebAPI:P2] → [LOE:S][WebAPI:P1]
Keywords: feature
Keywords: feature
(Reporter)

Comment 8

5 years ago
Created attachment 661457 [details] [diff] [review]
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.
(Reporter)

Comment 9

5 years ago
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.
Attachment #661457 - Attachment description: PoC, v1 → Patch, v1
Attachment #661457 - Flags: review?(21)
(Reporter)

Comment 10

5 years ago
Vivien, we started talking about this bug on IRC, but then we got distracted...  What's up?
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 :/
Attachment #661457 - Flags: review?(21)
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.
Attachment #661457 - Flags: review-
(Reporter)

Comment 13

5 years ago
> 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.
(Reporter)

Updated

5 years ago
Attachment #661457 - Flags: review- → review?(21)
(Reporter)

Comment 14

5 years ago
<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.
(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?
(Reporter)

Comment 16

5 years ago
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.  :(
(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 :)
(Reporter)

Comment 18

5 years ago
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.
(Reporter)

Comment 19

5 years ago
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.
We're trying to create a "nightly" branch with tested commits, but it's having a rough time in review.
(Reporter)

Comment 21

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

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

Comment 22

5 years ago
I still do not have a functioning device build, but I'm working on it.
(Reporter)

Comment 23

5 years ago
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?
Attachment #661457 - Flags: review?(21) → review-
(Reporter)

Comment 24

5 years ago
Tim, do you think you're the right person to look at this?
Assignee: justin.lebar+bug → nobody
Assigning to Chris Peterson.
Assignee: nobody → cpeterson
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Priority: -- → P1

Updated

5 years ago
Blocks: 798695
(Assignee)

Updated

5 years ago
Blocks: 796046
Summary: When IME appears, focused input element should not be hidden by keyboard → [keyboard]When IME appears, focused input element should not be hidden by keyboard
Whiteboard: [LOE:S][WebAPI:P1] → [LOE:S][WebAPI:P1][qa+]
How are things going on a fix here, Chris?  Are we waiting on the non-blocking-basecamp part of bug 780395?
Flags: needinfo?(cpeterson)
(Assignee)

Comment 27

5 years ago
Andrew, I was waiting for djf to land a big refactoring. Now that he has landed it, I can continue my investigation.
Flags: needinfo?(cpeterson)
Duplicate of this bug: 806602
(Assignee)

Comment 29

5 years ago
Created attachment 680277 [details] [diff] [review]
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.
Attachment #661457 - Attachment is obsolete: true
Attachment #680277 - Flags: review?(dflanagan)
Attachment #680277 - Flags: feedback?(ttaubert)
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.
Attachment #680277 - Flags: review?(dflanagan) → review+
(Assignee)

Comment 32

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/a498c73be0fa
status-firefox17: --- → wontfix
status-firefox18: --- → fixed
(Assignee)

Updated

5 years ago
Duplicate of this bug: 798695
https://hg.mozilla.org/mozilla-central/rev/3de447b1c396
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Attachment #680277 - Flags: feedback?(ttaubert)
(Assignee)

Updated

5 years ago
Depends on: 819598
You need to log in before you can comment on or make changes to this bug.