Closed Bug 969230 Opened 7 years ago Closed 7 years ago
[FTE] Email address field is covered by keyboard in about B2G OS page
33.96 KB, image/png
6.73 KB, patch
|Details | Diff | Splinter Review|
46 bytes, text/x-github-pull-request
|Details | Review|
7 bytes, text/plain
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
Can we a screenshot of this bug on Buri?
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
This has to be a regression. Can someone check what happens on 1.1 to confirm?
does not seem to be Tarako specific, remove tarako
Summary: [Tarako][FTE] Email address field is covered by keyboard in about B2G OS page → [FTE] Email address field is covered by keyboard in about B2G OS page
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
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?
Dylan ni'ed for email
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.
Flags: needinfo?(doliver) → needinfo?(francisco.jordano)
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
Hi Dylan, thanks for the heads up, as well, I think this is a problem related to the keyboard. Pinging Jan to confirm.
Flags: needinfo?(francisco.jordano) → needinfo?(janjongboom)
Tim Please help reassign for keyboard
blocking-b2g: 1.3? → 1.3+
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?
Component: Gaia::Keyboard → General
Yes, I can scroll the page manually while keyboard is focused. I believe it's auto-scrolling doesn't work on FTE pages.
forms.js is owned by Keyboard team so taking the bug.
Assignee: nobody → janjongboom
Sounds like Jan volunteered :)
* 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.
Oh hmm, now it does repro on gaia 1.4, perhaps a timing issue.
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 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:-)
Attachment #8376215 - Flags: review?(xyuan) → review?(dflanagan)
David, can you help to review the patch? Thank you. :) (I think US is in holidays next Monday.)
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.
Attachment #8376215 - Flags: review?(dflanagan) → review-
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?
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?
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.
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?
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.
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.
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 <email@example.com> 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 <firstname.lastname@example.org> 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 <email@example.com> 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?
Flags: needinfo?(timdream) → needinfo?(janjongboom)
This is the culprit: commit 73055fa4a8612ddb189f37530ca12da41ff5565d Author: Vivien Nicolas <firstname.lastname@example.org> 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.
Well, backing it out is not really an option because then scrolling in FTU lists like WiFi doesn't work anymore... Vivien, any insights?
Fernando can you take a look to this?
Flags: needinfo?(francisco.jordano) → needinfo?(fernando.campo)
Sure, will do.
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)
Assignee: janjongboom → fernando.campo
Component: General → Gaia::First Time Experience
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
Attachment #8381521 - Flags: review?(francisco.jordano)
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!
Attachment #8381521 - Flags: review?(francisco.jordano) → review-
(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 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
Attachment #8381521 - Flags: review- → review?(francisco.jordano)
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!
Attachment #8381521 - Flags: review?(francisco.jordano) → review+
green travis, merged on master - b557ecc3f460e49e34edeefcb73dbc0908f32a01
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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]:
Attachment #8384629 - Flags: approval-gaia-v1.3?
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
Attachment #8384629 - Flags: approval-gaia-v1.3? → approval-gaia-v1.3+
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.
You need to log in before you can comment on or make changes to this bug.