609940 - Android only : caret (cursor) can hide underneath the formfill helper

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: Screenshot

Mozilla/5.0 (Android; Linux armv71; rv2.0b8pre) Gecko/20101105 Firefox/4.0b8pre Fennec/4.0b3pre

1. have the hardware keyboard out
2. go to http://people.mozilla.com/~nhirata/html_tp/formsninput.html
3. scroll down to the text area box
4. click in the text area box and keep hitting return fast until the caret scrolls out of view

Expected: the caret will still be in focus
Actual: the caret hides under the formfill helper.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Vivien - another "pan into view" bug? Feel free to dupe this bug

Comment 2

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

> 1. have the hardware keyboard out

uh? do you have a G1 or something like this?
If yes, does this bug only happens when the hardware keyboard is out? (Asking that to find the better way to reproduce with the devices I have currently)

Comment 3

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

I need help to reproduce this bug

Comment 4

[Aakash Desai [:aakashd]]

We can get someone from qa to help. In the future, try adding the "qawanted" keyword :). This way we can use our Monday triage sessions to find out which bugs need help.

Comment 5

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

(In reply to comment #4)
> We can get someone from qa to help. In the future, try adding the "qawanted"
> keyword :). This way we can use our Monday triage sessions to find out which
> bugs need help.

I will use the flag next time, thanks for the tip :)

Comment 6

[Martijn Wargers (dead)]

Attachment: testcase

I can reproduce this, just by tapping on the textarea in this testcase.

I've seen this bug occuring too, where I'm working with virtual keyboards.

Comment 7

[Martijn Wargers (dead)]

Hmm, on the Android phone, the bug doesn't show that easily, although I can see it directly on Windows, that way.

Comment 8

[Martijn Wargers (dead)]

Attachment: testcase2

Ok, this testcase reproduces it for me on windows, the N90 and the Droid.
Steps to reproduce:
- Visit testcase2, scroll down to textarea 4
- Tap on textarea 4

Expected result:
- The caret should be completely in view

Actual result:
- The caret is partly or completely buried under the form helper

Comment 9

[Martijn Wargers (dead)]

Let me know, if you can reproduce it with this testcase. If not, I'll ad back the 'qawanted' keyword (and will try come up with a better testcase).

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Viven - This bug seems similar to some of other formhelper bugs you have worked one.

Comment 11

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

(In reply to comment #10)
> Viven - This bug seems similar to some of other formhelper bugs you have worked
> one.

I have already a plan that I'm working on, basically the path to show the form helper, dealing with multiple resizes, size changed of the current view, etc... is wrong. I'm redoing it, this is also the first step to fix bug 622342 in my opinion.

Comment 13

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

Attachment: wip-1

First wip, with some paltform bug inside :(
I'll file bug for them

Comment 14

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

Attachment: Patch

The patch add some tests but half of them are disabled because of bug 625341 (sick).

Comment 15

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

Attachment: Patch

Since bug 625341 is fixed the patch is the same but with more tests enabled.

Comment 16

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

ARGH! The situation is worse than ever on the N900 with this patch, only a squared part of the screen is enabled for the webpage, the rest is greyed...

Comment 17

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

Attachment: Patch

The patch resolve the odd issue I as seeing on Maemo and fix bug 622342

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 503850 [details] [diff] [review]
Patch


>diff --git a/chrome/content/forms.js b/chrome/content/forms.js
>       gFocusManager.setFocus(element, Ci.nsIFocusManager.FLAG_NOSCROLL);
>-      sendAsyncMessage("FormAssist:Show", this._getJSON());
>+      // To ensure we get the current caret positionning of the focused
>+      // element we need to delayed a bit the event
>+      this._executeDelayed(function(self) {
>+        sendAsyncMessage("FormAssist:Show", self._getJSON());

nit: Add a line break before the comment

>   _getCaretRect: function _formHelperGetCaretRect() {
>     let element = this.currentElement;
>+    let focusedElement = gFocusManager.getFocusedElementForWindow(content, true, {});
>     if ((element instanceof HTMLTextAreaElement ||
>         (element instanceof HTMLInputElement && element.type == "text")) &&
>-        gFocusManager.focusedElement == element) {
>+        focusedElement == element) {

These multi-line if tests kill me. I guess I am the only one though.

>diff --git a/chrome/tests/head.js b/chrome/tests/head.js

> let chromeRoot = getRootDirectory(gTestPath);
>-messageManager.loadFrameScript(chromeRoot + "remote_head.js", true);
>+let baseURI = "http://mochi.test:8888/browser/mobile/chrome/";
>+messageManager.loadFrameScript(baseURI + "remote_head.js", true);
> messageManager.loadFrameScript("chrome://mochikit/content/tests/SimpleTest/EventUtils.js", true);

This confuses me. I know we changed how to access scripts in the tests. I thought chromeRoot was the right way?

We still use chromeRoot to load JS in browser_vkb.js and browser_forms.js - is this OK?
http://mxr.mozilla.org/mobile-browser/source/chrome/tests/browser_vkb.js#5
http://mxr.mozilla.org/mobile-browser/source/chrome/tests/browser_forms.js#2

r+, but what's the answer for the chromeRoot stuff? and fix the nit

Comment 19

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

(In reply to comment #18)
> >diff --git a/chrome/tests/head.js b/chrome/tests/head.js
> 
> > let chromeRoot = getRootDirectory(gTestPath);
> >-messageManager.loadFrameScript(chromeRoot + "remote_head.js", true);
> >+let baseURI = "http://mochi.test:8888/browser/mobile/chrome/";
> >+messageManager.loadFrameScript(baseURI + "remote_head.js", true);
> > messageManager.loadFrameScript("chrome://mochikit/content/tests/SimpleTest/EventUtils.js", true);
> 
> This confuses me. I know we changed how to access scripts in the tests. I
> thought chromeRoot was the right way?
> 
> We still use chromeRoot to load JS in browser_vkb.js and browser_forms.js - is
> this OK?
> http://mxr.mozilla.org/mobile-browser/source/chrome/tests/browser_vkb.js#5
> http://mxr.mozilla.org/mobile-browser/source/chrome/tests/browser_forms.js#2
> 
> r+, but what's the answer for the chromeRoot stuff? and fix the nit

I don't know the gorry details but for there is a security reason that prevent this test to run for me if I use chromeRoot to load remote_head.js.
I think I can move this call internally to browser_formsZoom.js if you think it's better?

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

How about just adding a comment so we remember that you did it on purpose.

Comment 21

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

(In reply to comment #20)
> How about just adding a comment so we remember that you did it on purpose.

Will do. I'm a little bit of a scooge with comments :)

Comment 22

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

http://hg.mozilla.org/mobile-browser/rev/18be310a82a2

Comment 23

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: screenshot2 build : build 20110119

Comment 24

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

This bug still occurs in :
Mozilla/5.0 (Android; Linux armv71; rv2.0b10pre) Gecko/20110119 Firefox/4.0b10pre Fennec/4.0b4pre

Comment 25

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I found that I got an error when hitting return in the text area box:
Error: browser.getPosition is not a function
Source File: chrome://browser/content/common-ui.js

Comment 26

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Line 874

Comment 27

[Mark Finkle (:mfinkle) (use needinfo?)]

We should make sure the error is not happening, but bug 622578 will make the form helper not move content around.

I feel like WONTFIX'ing this bug and moving ahead with bug 622578