969230 - [FTE] Email address field is covered by keyboard in about B2G OS page

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::First Time Experience, defect)

Description

[Al Tsai [:atsai]]

E-mail field in FTE, about B2G OS page, is covered by keyboard if you attempt to enter e-mail.

STR:
1. Launch you phone with FTE
2. Go to "about B2G OS" page, and try to type e-mail

Expected Result:
*. Page scroll up a little bit to make you able to type e-mail address and see the e-mail you type in

Actual Result:
*. E-mail field is covered by keyboard panel

Comment 1

[Jason Smith [:jsmith]]

Can we a screenshot of this bug on Buri?

Comment 2

[Al Tsai [:atsai]]

Attachment: 2014-02-07-03-05-27.png

Sure, here it is.

Buri v1.3
Gaia      f9a37c77efb4621a1f57e4695b497d18601fe134
Gecko     http://hg.mozilla.org/releases/mozilla-aurora/rev/3d9d920ca43b
BuildID   20140203004001
Version   28.0a2
ro.build.version.incremental=eng.archermind.20131114.105818
ro.build.date=Thu Nov 14 10:58:33 CST 2013

Comment 3

[Jason Smith [:jsmith]]

This has to be a regression.

Can someone check what happens on 1.1 to confirm?

Comment 4

[Joe Cheng [:jcheng] (please needinfo)]

does not seem to be Tarako specific, remove tarako

Comment 5

[Matthew Vaughan]

This issue does not reproduce on the 02/06/14 1.1 build on a Buri device.

Gaia:    c5cb252e5d1aa45d14f3a2ea182e73e2271e4496
Gecko:   adaf8883268e
BuildID: 20140206041207
Version: 18.0

Comment 6

[Preeti Raghunath(:Preeti)]

If this doesn't reproduce on 1.1 then something within the 1.3 cycle broke it.

Can we narrow it down to see what commit broke it and ask for a backout?

Comment 7

[Preeti Raghunath(:Preeti)]

Dylan ni'ed for email

Comment 8

[Dylan Oliver [:doliver]]

This is FTU, not email. Redirecting to Francisco to get it on his radar but from comment #22 I think you were looking for a regression range so I'll tag for that too.

Comment 9

[Matthew Vaughan]

This issue started reproducing on the 01/16/14 1.3 build.

- Works -
Device: Buri v1.3 MOZ RIL
BuildID: 20140115004003
Gaia: 14e199d6a9ad917eacad883820a9f7619dbf42c8
Gecko: d7260b206e91
Version: 28.0a2
Firmware Version: V1.2-device.cfg

- Broken -
Device: Buri v1.3 MOZ RIL
BuildID: 20140116004002
Gaia: 423326d524d3807b3a7ef9cc10f34baf26a34b8c
Gecko: 1dd80f8faa2f
Version: 28.0a2
Firmware Version: V1.2-device.cfg

Comment 10

[Francisco Jordano [:arcturus] [:francisco]]

Hi Dylan,

thanks for the heads up, as well, I think this is a problem related to the keyboard.

Pinging Jan to confirm.

Comment 11

[Preeti Raghunath(:Preeti)]

Tim

Please help reassign for keyboard

Comment 12

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

Al,

Were you able to scroll the page manually when the keyboard is focused?

The whole auto-scrolling thing took place in Gecko in forms.js, so my team is probably not the best engineers to work on it (in a timely matter). Vivien, do you know who we should loop?

Comment 13

[Al Tsai [:atsai]]

Yes, I can scroll the page manually while keyboard is focused. I believe it's auto-scrolling doesn't work on FTE pages.

Comment 14

[Jan Jongboom [:janjongboom] (inactive)]

forms.js is owned by Keyboard team so taking the bug.

Comment 15

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

Sounds like Jan volunteered :)

Comment 16

[Jan Jongboom [:janjongboom] (inactive)]

* Does not reproduce on mc and gaia 1.4
* Does reproduce on mc and gaia 1.3

So here's what happens I believe: we focus, in Gecko we see that the textbox is already in the viewport, and we don't do anything, but after a while FTU realizes that the window was resized and mrepositions the bottom bar (Back/Done buttons), and we end up underneath that bar. The question is if we should fix this in Gecko or Gaia. Apparently it has been fixed in master, so let's see how that was done.

Comment 17

[Jan Jongboom [:janjongboom] (inactive)]

Oh hmm, now it does repro on gaia 1.4, perhaps a timing issue.

Comment 18

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: 969230.patch

Comment 19

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: 969230.patch

The bug is that isVisible() does not check if another element overlaps the focusedElement, which is the case on FTU. I rewrote the visibility check to be a lot simpler which also works on this.

Let me know if you think of a case where the simplified check is not enough...

Comment 20

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 8376215 [details] [diff] [review]
969230.patch

I'm not the best to review this patch.
Redirect to djf as he wrote the original code. If he doesn't have time to review this, I'll try:-)

Comment 22

[Kai-Chih Hu [:khu]]

David, can you help to review the patch? Thank you. :)
(I think US is in holidays next Monday.)

Comment 23

[David Flanagan [:djf]]

Comment on attachment 8376215 [details] [diff] [review]
969230.patch

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

This looks like a nice simplification to form.js, but I can't give r+ because:

1) I didn't write that code in forms.js. It is from bug 819598 and was written over a year ago by James Lal. I did review it, apparently, but have no memory of it and don't understand it.  Maybe James would be willing to override my r- here.

2) Today is a US holiday, so I'm not actually working today and don't have the time to dig deep enough to understand the code. (I assume that James is also OOO today as well.)

3) Bug 819598 did not have tests attached to it, so as far as I know we don't have any way to tell whether this proposed patch will cause other regressions.

4) We have a regression window identified on January 16, but no one has looked to see what changed that day.  If Jan is right about the FTU app moving its buttons in response to a resize event, I'd guess that a change in FTU caused the regression.

5) The hypothesis in comment #16 is that the FTU app is repositioning its buttons in response to a resize event and covering up the input field. If so, then there is a timing issue here, and the attached patch will only work if the isVisible() check happens after the FTU app (or any content) moves things.  So I worry that even though the patch works today, it will not be robust.

Basically: this is a major change to forms.js and it feels to risky to me to land in a last minute blocker bug. It does seem like a nice simplification, and bugfix for our isVisible code though, and we should ask James to review it for landing in 1.4.

Meanwhile, I suggest that we identify what changed on 1/16 to cause the regression in the first place, and let's see if we can fix this bug there.

Comment 24

[David Flanagan [:djf]]

Francisco,

Jan's hypothesis in comment #16 is that FTU is actively moving its bottom button bar when the keyboard pops up.  Does it do something like that?  Comment #9 says that this regression occurred on 1/16. FTU was under active development then. Could you check whether anything changed in FTU that could have caused this?

Comment 25

[David Flanagan [:djf]]

Tim,

Do you know if we landed anything in the keyboard on January 16 that could have affected the timing of the app resize in relation to the isVisible() call in forms.js?

Comment 26

[David Flanagan [:djf]]

I wonder if bug 951375 could have caused this regression. Seems like a stretch, but that patch looks to be at the right date, and makes changes that could affect document size and element positioning.

Comment 27

[Jan Jongboom [:janjongboom] (inactive)]

FYI, even when this is introduced last month in FTU, it's still a platform bug. Gecko gets a resize event, and should scroll into view because the bottom bar is on top of the element, but Gecko thinks the element is already visible. This is behavior we will also see in content.

What do you think?

Comment 28

[David Flanagan [:djf]]

Jan,

I don't fully understand the bug, and don't get why the bug did not manifest before.  I'd be a lot more comfortable if we really knew what was going on.

I agree that your visibility patch is an important one and recommend that we pursue it in 1.4.

If we do have to land a forms.js change in 1.3, I'd be much happier about it if you could add your overlap test to the existing code, rather than simplifying the code so much. It is the amount of changed code that I worry about since, as far as I know, we do not have tests for this.

Comment 29

[David Flanagan [:djf]]

Jan,

If you can create sample content (like a new panel in the UI Tests app, or just a web page somewhere) that manifests this same bug (without your patch) that might help us understand what is going on.

It would also help us assess the risk involved. Obviously, we have to block on fixing the FTU app. But knowing how common the bug will be in the wild will help us decide whether we need to fix the general problem in gekco or if we can just work around it in Gaia for 1.3.

Comment 30

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

I suspect there might be some timing issue introduced by keyboard manager. The below log show changes in relevant date:

commit 21166231ed393fec82c337eaba5c98fd300ef5cb
Merge: c99867f cf7cc51
Author: Alive.Kuo <[e-mail]>
Date:   Wed Jan 15 19:25:01 2014 -0800

    Merge pull request #15346 from alivedise/bugzilla/959043/software-home-button-malfunction
    
    Bug 959043 - Fix Software Home Button on Nexus4, r=timdream, r=gary

commit cf7cc51c24990dda3f28d14d856169fa52c8af5e
Author: Alive Kuo <[e-mail]>
Date:   Wed Jan 15 17:22:23 2014 +0800

    Bug 959043 - Fix Software Home Button on Nexus4

 apps/system/js/keyboard_manager.js | 4 ++++
 1 file changed, 4 insertions(+)

commit e52c26afac7a88a48a81715301315431413e0fce
Author: Fabrice Desre <[e-mail]>
Date:   Tue Jan 14 21:34:43 2014 -0800

    Bug 957880 - Expose memory pressure events to gaia's system app r=timdream

 apps/system/js/keyboard_manager.js | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)


That said, I believe Gecko should still be able to handling difference in timing like this.

Jan, you could identify the actually regression bug by backing out the possible commit and test it out?

Comment 31

[Jan Jongboom [:janjongboom] (inactive)]

This is the culprit:

commit 73055fa4a8612ddb189f37530ca12da41ff5565d
Author: Vivien Nicolas <[e-mail]>
Date:   Tue Jan 14 12:28:47 2014 +0100

    Bug 951375 - [FTE] Cannot pan the First time Experience page. r=borja

If I back it out of master it works again. Let's see after lunch what I can do about it.

Comment 32

[Jan Jongboom [:janjongboom] (inactive)]

Well, backing it out is not really an option because then scrolling in FTU lists like WiFi doesn't work anymore...

Vivien, any insights?

Comment 33

[Francisco Jordano [:arcturus] [:francisco]]

Fernando can you take a look to this?

Comment 34

[Fernando Campo (:fcampo)]

Sure, will do.

Comment 35

[Fernando Campo (:fcampo)]

I found a hack added long time ago about this same problem, but it wasn't working, so I'm fixing it. As the workaround was there before, and gecko doesn't seem likely to be fixed any time soon, I guess that this is the way of closing the bug. I'll add some tests to pass the Rick-wall :p

Jan, as this is not keyboard related, I'm stealing the bug from you (unless you REALLY want it :D)

Comment 36

[Fernando Campo (:fcampo)]

Attachment: link to PR - https://github.com/mozilla-b2g/gaia/pull/16626

Fixed an already-existing hack to manually scroll the window when a resize happens after focusing on the input.

I understand that the tests are so-so, but it was the only way I found for a scroll. If someone gives me a better approach to test it, will be more than welcome

Comment 37

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8381521 [details] [review]
link to PR - https://github.com/mozilla-b2g/gaia/pull/16626

Nice work Fernando,

please take a look to the comment I left on github related to the definition of the 'scrollToElement' function.

Also please check in travis:
- Linting problems: probably you started to work in a branch that doesn't include the hooks for latest jshint checks. Rebase just to check if the problem is solved.
- Travis is red with the new tests :( probably a problem of lack of unit test isolation.

Thanks again!

Comment 38

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

(In reply to Jan Jongboom [:janjongboom] from comment #32)
> Well, backing it out is not really an option because then scrolling in FTU
> lists like WiFi doesn't work anymore...
> 
> Vivien, any insights?

Reviving the old hacks sounds good. For the future I have a POC keyboard that does not resize the window (bug 970093) and I hope to rework a bit the focusing code at that point. I suspect the core issue to be some sort of race between the focusing of the field and the resize of the window. The field is positionned into view before the window is resized.

Comment 39

[Fernando Campo (:fcampo)]

Comment on attachment 8381521 [details] [review]
link to PR - https://github.com/mozilla-b2g/gaia/pull/16626

Updated PR, sorry for the changes to other classes, but some other unrelated tests were leaving things open

Comment 40

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8381521 [details] [review]
link to PR - https://github.com/mozilla-b2g/gaia/pull/16626

Excellent!

Thanks for taking the time to fix the rest of bugs.

Please merge once travis is green!

Comment 41

[Fernando Campo (:fcampo)]

green travis, merged on master - b557ecc3f460e49e34edeefcb73dbc0908f32a01

Comment 42

[Fernando Campo (:fcampo)]

Attachment: v1.3 uplift request

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): apparently bug 951375 (see comment 31)
[User impact] if declined: user needs to scroll manually after clicking on input cause keyboard is gonna hide it
[Testing completed]: added unit tests, checked manually on device
[Risk to taking this patch] (and alternatives if risky): low risk, as we only do a page-specific hack to scroll the container
[String changes made]:

Comment 43

[Ross Kuhlman ([e-mail])]

Verified this issue does not occur in v1.4.
Environmental Variables:
Device: Buri v1.4 Moz RIL
BuildID: 20140304040200
Gaia: 6df4533f14ec2645bb13d8a803a5151583ca13a8
Gecko: 529b86b92b1d
Version: 30.0a1
Firmware Version: v1.2-device.cfg

Comment 44

[Ryan VanderMeulen [:RyanVM]]

v1.3: 8f82493d3b4f6868d57f49af003cc6cfc035dd07 & f692ee0e21ef4e25ec501d60226aa7da4b06ae60

When resolving some conflicts, I accidentally added a few lines that I shouldn't have. The second commit reverts those changes. If things still look wrong, please needinfo? me and I'll revert everything.