587492 - More consistent zooming into input elements.

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jeremias Bosch (:jbos)]

Zooming and scrolling to input content is most important. The main goal must be that the user can always read what he is typing or selecting. 

In order to achive this we must have a flexible code which allow different platforms to perform correctly. We must achive that the different virtual keyboard implementations on meego, android and other platforms do never hide the content. 

Meego i.e. use a overlay keyboard while android resize the browser.

To ensure this, we need to set a certain screenarea which marked as visible. We ensure through scrolling and zooming that he content is moved to this area.

Comment 1

[Jeremias Bosch (:jbos)]

Attachment: Fix it.

What this patch does:
 - changing the calculation 
 - fixed size for characters on screen to guaranty that the user can read what he is typing
 - scroll and zoom back to caret in case user scrolled / zoomed away and start
typing again.
 - adjustment of scroll and zoom in case of rotation 
 - following the caret to keep it visible
 - adds a workaround for a text render issue (text not painted on second page -
tilemanager problem)
 - position and zooming correctly for non textinput fields and ensure that they
are visible.

Comment 2

[Tony Chung [:tchung]]

Flagging for 2.0 triage.

Comment 4

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

Comment on attachment 466174 [details] [diff] [review]
Fix it.

Want Vivien to take a look at this.

Comment 5

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

Comment on attachment 466174 [details] [diff] [review]
Fix it.

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js
>--- a/chrome/content/browser-ui.js
>+++ b/chrome/content/browser-ui.js
>       case "FormAssist:Update":
>-        this._zoom(null, Rect.fromRect(json.caretRect));
>+        this._currentElementRect = Rect.fromRect(json.elRect);
>+        this._currentCaretRect = Rect.fromRect(json.caretRect);
>+        this._zoom(this._currentElementRect, this._currentCaretRect);
>+

Can't we directly set this._currentElementRect and this._currentCaretRect directly into the _zoom method?


>   /** Zoom and move viewport so that element is legible and touchable. */
>   _zoom: function _formHelperZoom(aElementRect, aCaretRect) {
>     let bv = Browser._browserView;
>-    let zoomRect = bv.getVisibleRect();
>+    this._currentElementRect = aElementRect;
>+    this._currentCaretRect = aCaretRect;
> 

Oh! Remove those into the FormAssist:Update case

>-    // Zoom to a specified Rect
>-    if (aElementRect && bv.allowZoom && Services.prefs.getBoolPref("formhelper.autozoom")) {

You remove this check but you never put it back, we want to keep the bv.allowZoom check and the pref check

>-      // Zoom to an element by keeping the caret into view
>-      let zoomLevel = Browser._getZoomLevelForRect(aElementRect);
>-      zoomLevel = Math.min(Math.max(kBrowserFormZoomLevelMin, zoomLevel), kBrowserFormZoomLevelMax);

kBrowserFormZoomLevelMin && kBrowserFormZoomLevelMax were used to keep a reasonable zoom amount for forms can you ensure clampZoomLevel will do that if you decide to use it instead?
And in this case please remove the constants if there are not used anymore

>+    //:TODO: get values from preferences and/or the system
>+    let viewAreaHeight = 250;
>+    let viewAreaWidth = 480;

I'm not sure to understand from where those values (250, 480) come from? 
Those values can change depending on the current input element, I would like to get them from the current visible rect if possible

>+    let caretLines = 5.0;
>+    let margin = 15;
>+    let harmonizeValue = 10;
>+    let fallbackCaretHeight = 30; //used in cases there is no caret aviable

Nit: "available"

> 
>-      zoomRect = Browser._getZoomRectForPoint(aElementRect.center().x, aElementRect.y, zoomLevel);
>-      Browser.animatedZoomTo(zoomRect);
>+    //we may can get this in the next version from the keyboard
>+    //exact sizes!
>+    if (!Util.isPortrait()) {
>+      caretLines = 3.0;
>+      viewAreaHeight = 80;
>+      viewAreaWidth = 800;
>     }

Same as above for 80/800. I would like to have them from the current available height/width.

> 
>-    // Move the view to show the caret if needed
>-    if (aCaretRect) {
>-      let caretRect = bv.browserToViewportRect(aCaretRect);
>-      if (zoomRect.contains(caretRect))
>-        return;
>+    //
>+    //Start calculation here, the order is important!
>+    //

Nit: // Start calculation here, the order is important


>-      let [deltaX, deltaY] = this._getOffsetForCaret(caretRect, zoomRect);
>-      if (deltaX != 0 || deltaY != 0) {
>-        Browser.contentScrollboxScroller.scrollBy(deltaX, deltaY);
>-        bv.onAfterVisibleMove();
>-      }
>+    //first hide all tool/sidebars, this will adjust the visible rect.
>+    Browser.hideTitlebar();
>+    Browser.hideSidebars();

Why do you want to hide them if the user has decided to show them?
If you get the current available height/width instead of fixed values you probably doesn't have to call these functions

>-      Browser.animatedZoomTo(zoomRect);
>+    //The height and Y of the caret might change during writing - even in cases the 
>+    //fontsize keeps the same. To avoid unneeded zooming and scrolling
>+    let harmonizedCaretHeight = 0;
>+    let harmonizedCaretY = 0;
>+
>+    //for buttons and non input field elements a caretRect with the height of 0 gets reported
>+    //cover this case.
>+    if (!aCaretRect.isEmpty())
>+    {

Nit:  if (!aCaretRect.isEmpty()) {

>+      //the height and y position may vary from letter to letter
>+      //adjust position and zooming only if a bigger step was done.
>+      harmonizedCaretHeight = aCaretRect.height - aCaretRect.height % harmonizeValue;
>+      harmonizedCaretY = aCaretRect.y - aCaretRect.y % harmonizeValue;
>+    } else {
>+      harmonizedCaretHeight = fallbackCaretHeight;

Nit: if fallbackCaretHeight is use only here you can declare right here.
Nit: Add a line break too

>+      //use the element as position
>+      harmonizedCaretY = aElementRect.y;
>+      aCaretRect.x = aElementRect.x;
>     }
>+
>+    let zoomLevel = (viewAreaHeight/caretLines)/harmonizedCaretHeight;

Nit: Please use spaces:
let zoomLevel = (viewAreaHeight / caretLines) / harmonizedCaretHeight;

>+    zoomLevel = bv.clampZoomLevel(zoomLevel);
>+
>+    viewAreaWidth  /= zoomLevel;
>+    viewAreaHeight /= zoomLevel;
>+
>+    let x = (margin + aCaretRect.x - aElementRect.x) < viewAreaWidth ? aElementRect.x-margin : aCaretRect.x-viewAreaWidth+margin;

Nit: use spaces for the end of the expression, also if you use margin only here, declare it next to the expression please

>+    let y = harmonizedCaretY - margin - viewAreaHeight/caretLines;
>+
>+    bv.setZoomLevel(zoomLevel);
>+    let vis = bv.getVisibleRect();
>+    x = bv.browserToViewport(x);
>+    y = bv.browserToViewport(y);
>+
>+    Browser.contentScrollboxScroller.scrollBy(x-vis.x, y-vis.y);
>+
>+
>+    //workaround for tilemanager bug, after one screen height scrolled text gets not painted on typing - 
>+    bv.invalidateEntireView();

Why do we stop using the nice animated zoom method?

>+    //--

Nit: remove this line


>diff --git a/chrome/content/forms.js b/chrome/content/forms.js
>--- a/chrome/content/forms.js
>+++ b/chrome/content/forms.js

>+    //change zoom on resize/rotation
>+    if (aEvent.type == "resize")
>+    {
>+        sendAsyncMessage("FormAssist:Resize");
>+    } else {

Can you change that for:

if (aEvent.type == "resize") {
  sendAsyncMessage("FormAssist:Resize");
}
else {
  ...
}

and then use the current indentation and brackets alignment of the file

Comment 6

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

I like the idea of doing a more consistent zoom/handling of caret and inputs elements.
What we currently does is very basic and the patch can bring some nice benefits in this area!

Comment 7

[Jeremias Bosch (:jbos)]

Alright, i see i need to do some more explanation here, but that is absolutly understandable :) since you can't know the meego platform and so you cant know the ideas behind that patch. I hope to be able to explain it just by anserwing the comments.

The biggest difference between meego and android is, that meego uses a "overlay" keyboard. It does not resize the window. The plus with that is, its really fast and seemles, there is no flowing in the ui needed, but it does also mean that zooming / scrolling for content cant orientate on the window size. Thats why I introduced the concept of a visible area, which is the screenarea minus the area which gets covered by the virtual keyboard. 
I needed to cover here also the rotation case because the size of the keyboard changes, and with that the visible screen area, when you rotate the device.


[..]
> >+        this._currentElementRect = Rect.fromRect(json.elRect);
> >+        this._currentCaretRect = Rect.fromRect(json.caretRect);
> >+        this._zoom(this._currentElementRect, this._currentCaretRect);
> >+
> 
> Can't we directly set this._currentElementRect and this._currentCaretRect
> directly into the _zoom method?

Mhm, we could, but that would set them again to there own value in case of rotation. Thats why i didn't set them.
   
> 
> >   /** Zoom and move viewport so that element is legible and touchable. */
> >   _zoom: function _formHelperZoom(aElementRect, aCaretRect) {
> >     let bv = Browser._browserView;
> >-    let zoomRect = bv.getVisibleRect();
> >+    this._currentElementRect = aElementRect;
> >+    this._currentCaretRect = aCaretRect;
> > 
> 
> Oh! Remove those into the FormAssist:Update case
> 

Ok, i got the point :) Mhm i clearly overlooked that :D

> >-    // Zoom to a specified Rect
> >-    if (aElementRect && bv.allowZoom && Services.prefs.getBoolPref("formhelper.autozoom")) {
> 
> You remove this check but you never put it back, we want to keep the
> bv.allowZoom check and the pref check
> 
Agree.

> >-      // Zoom to an element by keeping the caret into view
> >-      let zoomLevel = Browser._getZoomLevelForRect(aElementRect);
> >-      zoomLevel = Math.min(Math.max(kBrowserFormZoomLevelMin, zoomLevel), kBrowserFormZoomLevelMax);
> 
> kBrowserFormZoomLevelMin && kBrowserFormZoomLevelMax were used to keep a
> reasonable zoom amount for forms can you ensure clampZoomLevel will do that if
> you decide to use it instead?
> And in this case please remove the constants if there are not used anymore
> 
Alright.
> >+    //:TODO: get values from preferences and/or the system
> >+    let viewAreaHeight = 250;
> >+    let viewAreaWidth = 480;
> 
> I'm not sure to understand from where those values (250, 480) come from? 
These numbers describe the "visible screenarea" which is screenarea - virtual keyboard area.



> Those values can change depending on the current input element, I would like to
> get them from the current visible rect if possible
> 
No actually not, they are not depending to any input element. These values are not reported yet from the widget code. 

> >+    let caretLines = 5.0;
> >+    let margin = 15;
> >+    let harmonizeValue = 10;
> >+    let fallbackCaretHeight = 30; //used in cases there is no caret aviable
> 
> Nit: "available"
>

Ups.
 
> > 
> >-      zoomRect = Browser._getZoomRectForPoint(aElementRect.center().x, aElementRect.y, zoomLevel);
> >-      Browser.animatedZoomTo(zoomRect);
> >+    //we may can get this in the next version from the keyboard
> >+    //exact sizes!
> >+    if (!Util.isPortrait()) {
> >+      caretLines = 3.0;
> >+      viewAreaHeight = 80;
> >+      viewAreaWidth = 800;
> >     }
> 
> Same as above for 80/800. I would like to have them from the current available
> height/width.
>
See comment before, 

 
> 
> >-      let [deltaX, deltaY] = this._getOffsetForCaret(caretRect, zoomRect);
> >-      if (deltaX != 0 || deltaY != 0) {
> >-        Browser.contentScrollboxScroller.scrollBy(deltaX, deltaY);
> >-        bv.onAfterVisibleMove();
> >-      }
> >+    //first hide all tool/sidebars, this will adjust the visible rect.
> >+    Browser.hideTitlebar();
> >+    Browser.hideSidebars();
> 
> Why do you want to hide them if the user has decided to show them?
> If you get the current available height/width instead of fixed values you
> probably doesn't have to call these functions
>
They appear in certain cases, like rotation, they also take a lot of space which is needed to display the input element. At least for meego we do not want them visible since the vkb take about 80% of the screen height in landscape (screen height is 480px), when you consider to display the toolbars, the formhelper and the inputfield in the remaining space of less than 100 pixel you get the idea why we disable them.

> Why do we stop using the nice animated zoom method?
> 
It was to slow, to buggy and way to uncontrollable for my taste. The biggest point is, it allows us to use the same method for moving / adjusting when the caret moves out of screen and rotation cases.


I hope this makes it more clear, beside that i will fix it up, thanks for this review.

Comment 8

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

(In reply to comment #7)
> Alright, i see i need to do some more explanation here, but that is absolutly
> understandable :) since you can't know the meego platform and so you cant know
> the ideas behind that patch. I hope to be able to explain it just by anserwing
> the comments.

Obviously! Thanks for your explanation.

> The biggest difference between meego and android is, that meego uses a
> "overlay" keyboard. It does not resize the window. The plus with that is, its
> really fast and seemles, there is no flowing in the ui needed, but it does also
> mean that zooming / scrolling for content cant orientate on the window size.
> Thats why I introduced the concept of a visible area, which is the screenarea
> minus the area which gets covered by the virtual keyboard. 
> I needed to cover here also the rotation case because the size of the keyboard
> changes, and with that the visible screen area, when you rotate the device.
> 

I have nothing against the visible area in that case, but we should probably keep the values from the visible rect for the others platforms.

> > >+    //first hide all tool/sidebars, this will adjust the visible rect.
> > >+    Browser.hideTitlebar();
> > >+    Browser.hideSidebars();
> > 
> > Why do you want to hide them if the user has decided to show them?
> > If you get the current available height/width instead of fixed values you
> > probably doesn't have to call these functions
> >
> They appear in certain cases, like rotation, they also take a lot of space
> which is needed to display the input element. At least for meego we do not want
> them visible since the vkb take about 80% of the screen height in landscape
> (screen height is 480px), when you consider to display the toolbars, the
> formhelper and the inputfield in the remaining space of less than 100 pixel you
> get the idea why we disable them.

Ok makes sense but I just want to avoid hiding those, if there is _only_ a caret change, to prevent web site's javascript to change the caret position in a loop and prevent the user to access the sidebars. (Maybe it was previously the case I'm just thinking of it now :s)

Also if we know the vkb took a certain amount of size could we use a formula to determine the visible area instead of fixed value? This is not that important but I guess Meego will run on many platform and the size will depends on the device? If not you can keep the fixed values.

> 
> > Why do we stop using the nice animated zoom method?
> > 
> It was to slow, to buggy and way to uncontrollable for my taste. The biggest
> point is, it allows us to use the same method for moving / adjusting when the
> caret moves out of screen and rotation cases.

I got your point on that, zooming is still slow for the moment and is one of the key feature (sigh) but I would rather keep the AnimatedZoom method for now to stay unified and see if Layers can help here.

> I hope this makes it more clear, beside that i will fix it up, thanks for this
> review.

Yes, this is much clearer, first I've not realized it was for handling the specific case of Meego for the Keyboard, I'm glad to have learned something new :)

Thanks a lot for the times taken to explain the rationale behind this patch (and for the patch :))

Comment 9

[Jeremias Bosch (:jbos)]

Attachment: Zoom Update 2

I fixed the points, should be fine now.

Comment 10

[Jeremias Bosch (:jbos)]

Did see your comment after pushing :)

> I have nothing against the visible area in that case, but we should probably
> keep the values from the visible rect for the others platforms.

> Also if we know the vkb took a certain amount of size could we use a formula to
> determine the visible area instead of fixed value? This is not that important
> but I guess Meego will run on many platform and the size will depends on the
> device? If not you can keep the fixed values.
> 

We discussed yesterday with Mark Finkle that we would like to introduce a new Event from the Widgetside which informs fennec about the available screenarea. This will make this "worstcase" meego numbers obsolete and can than be used by all platforms. Tuneable values like the "CaretLines" should end up in a pref.

> 
> Ok makes sense but I just want to avoid hiding those, if there is _only_ a
> caret change, to prevent web site's javascript to change the caret position in
> a loop and prevent the user to access the sidebars. (Maybe it was previously
> the case I'm just thinking of it now :s)
> 

Urg, ok i get the point - didn't thought about that. alright i need to think about this a bit. Thanks for mentioning.

> > 
> > > Why do we stop using the nice animated zoom method?
> > > 
> > It was to slow, to buggy and way to uncontrollable for my taste. The biggest
> > point is, it allows us to use the same method for moving / adjusting when the
> > caret moves out of screen and rotation cases.
> 
> I got your point on that, zooming is still slow for the moment and is one of
> the key feature (sigh) but I would rather keep the AnimatedZoom method for now
> to stay unified and see if Layers can help here.

I see, can we agree to use a pref here to have this configurable? So that the platform / user can decide what ever it like to have.

> Thanks a lot for the times taken to explain the rationale behind this patch
> (and for the patch :))
You are welcome, I'm quite happy that you like the approach :)

Comment 11

[Jeremias Bosch (:jbos)]

Attachment: Zoom Update 3

I added the fix for the sidebars, its important to take the actually size in account in order to keep the scrolling when the caret would leave the screen and of course the inital positioning correct.

The animated zooming does behave really strange, i might mix up something but it looks like that it mess with the sidebars around. Can we add a bug about additional animated zooming and get this fixed in another bug?

Comment 12

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

(In reply to comment #11)
> Created attachment 466741 [details] [diff] [review]
> Zoom Update 3

Jeremias do you want me to review this one too or is it a WIP?

Comment 13

[Jeremias Bosch (:jbos)]

i would like to get your idea about that. I'm about to introduce the visible area which will add a xul part here and of course some more changes in fennec.

Just a additional question, is there a way to find out if the awesomebar is visible?

Comment 15

[Jeremias Bosch (:jbos)]

Attachment: Use realnumbers from the system

With this patch we observe the softkb-change. It contains the still visible screen area. The Observer adjust the controls (formhelper, awesomebar-page, find-in-page) by calling "sizeControls" and cause adjustment in zoom/scroll if needed.

We always hide the awesomebar now (not the sidebar), since the vkb + formhelper take already so much space in landscape that there is just enough for one line of a textinput.

Comment 16

[Jeremias Bosch (:jbos)]

Attachment: Promote size changes of the vkb

Promote size changes of the vkb

Comment 17

[Jeremias Bosch (:jbos)]

Attachment: Rebased

I had another patch applied, rebased it. Sorry for the spam.

Comment 18

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

Comment on attachment 467709 [details] [diff] [review]
Rebased

diff --git a/app/mobile.js b/app/mobile.js
 /* form helper */
 pref("formhelper.enabled", true);
 pref("formhelper.autozoom", true);
+#ifdef MOZ_PLATFORM_MAEMO
+pref("formhelper.restore", true);
+#else
 pref("formhelper.restore", false);
+#endif
+pref("formhelper.portait.caretLines",4);

I assume portait -> portrait :)

diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js
--- a/chrome/content/browser-ui.js
+++ b/chrome/content/browser-ui.js
@@ -1535,16 +1540,19 @@ var FormHelperUI = {
 
+    //first hide all tool/sidebars they take to much space, this will adjust the visible rect.
+    Browser.hideSidebars();
+
@@ -1576,18 +1584,26 @@ var FormHelperUI = {
+      case "FormAssist:Resize":
+        //first hide all tool/sidebars they take to much space, this will adjust the visible rect.
+        Browser.hideSidebars();
+        this._zoom(this._currentElementRect, this._currentCaretRect);
+        this._container.contentHasChanged();
+        break;
+
       case "FormAssist:Update":
-        this._zoom(null, Rect.fromRect(json.caretRect));
+        this._zoom(Rect.fromRect(json.elRect), Rect.fromRect(json.caretRect));
+
         break;
     }
   },

You could probably move the hideSidebars call into the _zoom method but don't do it when the caret change ("FormAssist:Update") if you don't use the elementRect on a caret change (I'm unsure about your reason for that?)
 
@@ -1691,42 +1708,87 @@ var FormHelperUI = {
-    let zoomRect = bv.getVisibleRect();
 
-    // Zoom to a specified Rect
-    if (aElementRect && bv.allowZoom && Services.prefs.getBoolPref("formhelper.autozoom")) {
-      // Zoom to an element by keeping the caret into view
-      let zoomLevel = Browser._getZoomLevelForRect(aElementRect);
-      zoomLevel = Math.min(Math.max(kBrowserFormZoomLevelMin, zoomLevel), kBrowserFormZoomLevelMax);
-
-      zoomRect = Browser._getZoomRectForPoint(aElementRect.center().x, aElementRect.y, zoomLevel);
-      Browser.animatedZoomTo(zoomRect);
+    if (!aElementRect || !bv.allowZoom || !Services.prefs.getBoolPref("formhelper.autozoom")) {
+      return;
     }
We still want to move to the caret position if it has changed but zoom is disabled

+    let viewAreaHeight = bv._visibleScreenArea.height - this._container.getBoundingClientRect().height;
+    let viewAreaWidth = bv._visibleScreenArea.width;
+    let caretLines = Services.prefs.getIntPref("formhelper.portait.caretLines");
+    let harmonizeValue = Services.prefs.getIntPref("formhelper.harmonizeValue");
 
-      Browser.animatedZoomTo(zoomRect);
+    //we may can get this in the next version from the keyboard
+    //exact sizes!
+    if (!Util.isPortrait()) {
+      caretLines = Services.prefs.getIntPref("formhelper.landscape.caretLines");
     }
I rather prefer that to fixed values, thanks for the change.

Nit: Don't use bracket for one line if statement (not my favorite but that's why we're doing in the rest of the file)

if (!Util.isPortrait())
  caretLines = Services.prefs.getIntPref("formhelper.landscape.caretLines");

+
+    // always hide, it takes to much space
+    Browser.hideTitlebar();

We probably don't need to hide the titlebar in case of a simple caret change

+    // Start calculation here, the order is important
+    let [leftvis,rightvis,leftW,rightW] = Browser.computeSidebarVisibility(0,0);

Nit: add spaces between the variables:
     let [leftvis, rightvis, leftW, rightW] = Browser.computeSidebarVisibility(0, 0);

+    let zoomLevel = (viewAreaHeight / caretLines) / harmonizedCaretHeight;
+    zoomLevel = Math.min(Math.max(kBrowserFormZoomLevelMin, zoomLevel), kBrowserFormZoomLevelMax);
+
+    viewAreaWidth  /= zoomLevel;
+    viewAreaHeight /= zoomLevel;
+
+    const margin = Services.prefs.getIntPref("formhelper.margin");
+
+    let x = (marginLeft + marginRight + margin + aCaretRect.x - aElementRect.x) < viewAreaWidth ? aElementRect.x - margin - marginLeft : aCaretRect.x - viewAreaWidth + margin + marginRight;
+    let y = harmonizedCaretY - margin;
+
+    bv.setZoomLevel(zoomLevel);
+    let vis = bv.getVisibleRect();
+    x = bv.browserToViewport(x);
+    y = bv.browserToViewport(y);
+
+    Browser.contentScrollboxScroller.scrollBy(x-vis.x, y-vis.y);


I still want to use the AnimatedZoom method here do a followup if we want to change that.
Sorry for the additional work :(

+
+    //workaround for tilemanager bug, after one screen height scrolled text gets not painted on typing - 
+    bv.invalidateEntireView();

good catch (remove the "- " at the end)

diff --git a/chrome/content/browser.js b/chrome/content/browser.js
--- a/chrome/content/browser.js
+++ b/chrome/content/browser.js
+const gVkbObserver = {
You can just name it VkbObserver

+  observe: function vkb_observe(subject, topic, data) {
+    // update statusbar visiblity
+    let rect = Rect.fromRect(JSON.parse(data));
+    rect.height = rect.bottom - rect.top;
+    rect.width  = rect.right - rect.left;

Nit: statusbar ?

+
+    Browser._browserView._visibleScreenArea = rect;
+
+    BrowserUI.sizeControls(rect.width, rect.height);
+    if (FormHelperUI._currentElementRect && FormHelperUI._currentCaretRect)
+      FormHelperUI._zoom(FormHelperUI._currentElementRect, FormHelperUI._currentCaretRect);

Do you want to test if the form helper is opened? In this case you can check FormHelperUI._open

diff --git a/chrome/content/forms.js b/chrome/content/forms.js
-    let currentElement = this.currentElement;
-    switch (aEvent.keyCode) {
-      case aEvent.DOM_VK_DOWN:
-        if (currentElement instanceof HTMLInputElement && !this._isAutocomplete(currentElement)) {
-          if (this._hasKeyListener(currentElement))
-            return;
-        }
-        else if (currentElement instanceof HTMLTextAreaElement) {
-          let existSelection = currentElement.selectionEnd - currentElement.selectionStart;
-          let isEnd = (currentElement.textLength == currentElement.selectionEnd);
-          if (!isEnd || existSelection)
-            return;
-        }
+    //change zoom on resize/rotation
+    if (aEvent.type == "resize")
+    {

Nit: if (aEvent.type == "resize") {

-    let caretRect = this._getCaretRect();
-    if (!caretRect.isEmpty()) {
-      sendAsyncMessage("FormAssist:Update", { caretRect: caretRect });
+      let caretRect = this._getCaretRect();
+      let elRect = this._getRect();
+      if (!caretRect.isEmpty()) {
+        sendAsyncMessage("FormAssist:Update", { caretRect: caretRect, elRect: elRect });
+      }
     }
   },

I have to admit that I'm not sure wht you need to send the caretRect and the elRect?


Sorry for the long review.
This is much closer to final but I think most of my comments are coming from the fact _zoom has 2 behaviors currently (this is a bad lier function! Shame on the author! oh that's me!):
 1. zoom to the element
 2. Move the view to keep the cursor in place

Your patch alter the function in such a way that I don't see these 2 behaviors but only a zoom method (which is closer to the name of the function but not what I expect)

Are you on IRC? This could be easier to discuss this point. (I'm vingtetun on #mobile)
With it disambiguate i should be fast to get something ready to land!

Comment 19

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

Comment on attachment 467709 [details] [diff] [review]
Rebased

>diff --git a/app/mobile.js b/app/mobile.js

> /* form helper */
> pref("formhelper.enabled", true);
> pref("formhelper.autozoom", true);
>+#ifdef MOZ_PLATFORM_MAEMO
>+pref("formhelper.restore", true);
>+#else
> pref("formhelper.restore", false);
>+#endif
>+pref("formhelper.portait.caretLines",4);
>+pref("formhelper.harmonizeValue",10);
>+pref("formhelper.landscape.caretLines",1);
>+pref("formhelper.margin",15);

Spaces between arguments and lets use this naming:
  pref("formhelper.caretLines.portrait", 4);
  pref("formhelper.caretLines.landscape", 1);

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>   _zoom: function _formHelperZoom(aElementRect, aCaretRect) {

>+    //we may can get this in the next version from the keyboard
>+    //exact sizes!

      // we may be able to get exact sizes from the keyboard someday

>+    if (!Util.isPortrait()) {
>+      caretLines = Services.prefs.getIntPref("formhelper.landscape.caretLines");
>     }

No {} needed for a one-liner

>+const gVkbObserver = {

I would go further than Vivien and say "KeyboardObserver"

>+        default:
>+          if (this._isAutocomplete(aEvent.target)) {
>+            sendAsyncMessage("FormAssist:AutoComplete", this._getJSON());
>+          }

No {} needed on a one-liner

Comment 20

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

(In reply to comment #19)
> Comment on attachment 467709 [details] [diff] [review]
> Rebased
> 
> >diff --git a/app/mobile.js b/app/mobile.js
> 
> > /* form helper */
> > pref("formhelper.enabled", true);
> > pref("formhelper.autozoom", true);
> >+#ifdef MOZ_PLATFORM_MAEMO
> >+pref("formhelper.restore", true);
> >+#else
> > pref("formhelper.restore", false);
> >+#endif

Oops. I forgot to mention. We'll need to consider the impact of "formhelper.restore" working differently on Maemo than Android.

Comment 21

[Jeremias Bosch (:jbos)]

Attachment: now with animated zoom path :D

This version is final, when you ask me. It contains the animated zoom path. as well as the things you pointed out. The toolbar gets only hidden in case there is not enough space to show all. This is dynamic. I also cover now the case of moving textfields, they can move as much as they want, in cases the caret jumps out of screen we follow.

I added also a lot of comments to explain this relative complicated piece of code.

Comment 22

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

Comment on attachment 467930 [details] [diff] [review]
now with animated zoom path :D

>diff --git a/app/mobile.js b/app/mobile.js
>--- a/app/mobile.js
>+++ b/app/mobile.js
>@@ -126,17 +126,27 @@ pref("alerts.height", 50);
> /* password manager */
> pref("signon.rememberSignons", true);
> pref("signon.expireMasterPassword", false);
> pref("signon.SignonFileName", "signons.txt");
> 
> /* form helper */
> pref("formhelper.enabled", true);
> pref("formhelper.autozoom", true);
>+#ifdef MOZ_PLATFORM_MAEMO
>+pref("formhelper.animatedZoom", false);

Thanks for adding it, please keep it true by default on all platforms then we could switch it into another bug if needed (Layers are coming and should help a lot)

>+pref("formhelper.caretLines.portrait",4);
>+pref("formhelper.caretLines.landscape",1);
>+pref("formhelper.harmonizeValue",10);
>+pref("formhelper.margin",15);

See Mark's comment about whitespaces.

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>+    //hide all sidebars, this will adjust the visible rect.
>+    Browser.hideSidebars();
>+    //save the element Rect and reuse it to avoid jumps in cases the element moves sligtly on the website.

Nit: sligtly -> slighty

>+      case "FormAssist:Resize":
>+        //first hide all tool/sidebars they take to much space, this will adjust the visible rect.
>+        Browser.hideSidebars();
>+        this._zoom(this._currentElementRect, this._currentCaretRect);
>+        this._container.contentHasChanged();
>+        break;

Nit: can you ensure we need the this._container.contentHasChanged() call?

>   /** Zoom and move viewport so that element is legible and touchable. */
>   _zoom: function _formHelperZoom(aElementRect, aCaretRect) {
>     let bv = Browser._browserView;
>-    let zoomRect = bv.getVisibleRect();
> 
>-    // Zoom to a specified Rect
>-    if (aElementRect && bv.allowZoom && Services.prefs.getBoolPref("formhelper.autozoom")) {
>-      // Zoom to an element by keeping the caret into view
>-      let zoomLevel = Browser._getZoomLevelForRect(aElementRect);
>-      zoomLevel = Math.min(Math.max(kBrowserFormZoomLevelMin, zoomLevel), kBrowserFormZoomLevelMax);
>+    if (aElementRect && aCaretRect) {
>+      this._currentCaretRect = aCaretRect;
> 
>-      zoomRect = Browser._getZoomRectForPoint(aElementRect.center().x, aElementRect.y, zoomLevel);
>-      Browser.animatedZoomTo(zoomRect);
>-    }
>+
>+      // might not always be set, if not - use the windowsize
>+      let visibleScreenArea = !bv._visibleScreenArea.isEmpty() ? bv._visibleScreenArea : new Rect(0, 0, window.innerWidth, window.innerHeight);

Nit: add a line break (this is cheap!)

>+      // respect the helper container in setting the correct viewAreaHeight
>+      let viewAreaHeight = visibleScreenArea.height - this._container.getBoundingClientRect().height;

Since you define visibleScreenArea to Rect(0, 0, 0, 0) does that mean viewAreaHeight will be negative if there is not virtual keyboard (because visibleScreenArea was not updated and so visibleScreenArea.height will be equal to 0)

>+      let viewAreaWidth = visibleScreenArea.width;
>+      let caretLines = Services.prefs.getIntPref("formhelper.caretLines.portrait");
>+      let harmonizeValue = Services.prefs.getIntPref("formhelper.harmonizeValue");
> 
>-    // Move the view to show the caret if needed
>-    if (aCaretRect) {
>-      let caretRect = bv.browserToViewportRect(aCaretRect);
>-      if (zoomRect.contains(caretRect))
>-        return;
>+      if (!Util.isPortrait())
>+        caretLines = Services.prefs.getIntPref("formhelper.caretLines.landscape");
> 
>-      let [deltaX, deltaY] = this._getOffsetForCaret(caretRect, zoomRect);
>-      if (deltaX != 0 || deltaY != 0) {
>-        Browser.contentScrollboxScroller.scrollBy(deltaX, deltaY);
>+      // hide titlebar if the remaining space would be smaller than the height of the titlebar itself
>+      // if there is enough space left than adjust the height. Since this adjust the the visible rects
>+      // there is no need to adjust the y later
>+      let toolbar = document.getElementById("toolbar-main");
>+      if (viewAreaHeight - toolbar.boxObject.height <= toolbar.boxObject.height * 2)
>+        Browser.hideTitlebar();
>+      else
>+        viewAreaHeight -= toolbar.boxObject.height;

You can use BrowserUI.toolbarH (I should have point you to that sooner) instead of toolbar.boxObject

>+
>+      // To ensure the correct calculation when the sidebars are visible - get the sidebar size and 
>+      // use them as margin
>+      let [leftvis, rightvis, leftW, rightW] = Browser.computeSidebarVisibility(0, 0);
>+      let marginLeft = leftvis ? leftW : 0;
>+      let marginRight = rightvis ? rightW : 0;
>+
>+      //The height and Y of the caret might change during writing - even in cases the 
>+      //fontsize keeps the same. To avoid unneeded zooming and scrolling
>+      let harmonizedCaretHeight = 0;
>+      let harmonizedCaretY = 0;
>+
>+      // Start calculation here, the order is important
>+      // All calculations are done in non_zoomed_coordinates => 1:1 to "screen-pixels"
>+
>+      //for buttons and non input field elements a caretRect with the height of 0 gets reported
>+      //cover this case.
>+      if (!aCaretRect.isEmpty()) {
>+        //the height and y position may vary from letter to letter
>+        //adjust position and zooming only if a bigger step was done.
>+        harmonizedCaretHeight = aCaretRect.height - aCaretRect.height % harmonizeValue;
>+        harmonizedCaretY = aCaretRect.y - aCaretRect.y % harmonizeValue;
>+      } 
>+      else {
>+        harmonizedCaretHeight = 30; //fallback height
>+        //use the element as position
>+        harmonizedCaretY = aElementRect.y;
>+        aCaretRect.x = aElementRect.x;
>+      }
>+
>+      let zoomLevel = bv.getZoomLevel();
>+      if (bv.allowZoom && Services.prefs.getBoolPref("formhelper.autozoom")) {
>+        zoomLevel = (viewAreaHeight / caretLines) / harmonizedCaretHeight;
>+        zoomLevel = Math.min(Math.max(kBrowserFormZoomLevelMin, zoomLevel), kBrowserFormZoomLevelMax);
>+      }

Nit: You use bv.allowZoom && Services.prefs.getBoolPref("formhelper.autozoom") a few times after that, let's store the result into a variable and use that

>+
>+      viewAreaWidth  /= zoomLevel;
>+
>+      const margin = Services.prefs.getIntPref("formhelper.margin");
>+
>+      // if the viewAreaWidth is smaller than the neutralized position + margins.
>+      // [YES] use the x position of the element minus margins as x position for our visible rect.
>+      // [NO] use the x position of the caret minus margins as the x position for our visible rect.
>+      let x = (marginLeft + marginRight + margin + aCaretRect.x - aElementRect.x) < viewAreaWidth ? aElementRect.x - margin - marginLeft : aCaretRect.x - viewAreaWidth + margin + marginRight;

Nit: Add a line break

>+      // use the adjustet Caret Y minus a margin four our visible rect
Nit: adjustet -> adjusted?

>+      let y = harmonizedCaretY - margin;
>+
>+      // from here on play with zoomed values
>+      // if we want to have it animated, build up zoom rect and animate.
>+      if (Services.prefs.getBoolPref("formhelper.animatedZoom") && 
>+          Services.prefs.getBoolPref("formhelper.autozoom") &&
>+          bv.allowZoom) {
>+
>+        let vis = bv.getVisibleRect();
>+        x = bv.browserToViewport(x);
>+        y = bv.browserToViewport(y);
>+
>+        //dont use browser functions they are bogus for this case
>+        let zoomRatio = zoomLevel / bv.getZoomLevel();
>+        let newVisW = vis.width / zoomRatio, newVisH = vis.height / zoomRatio;
>+        let zoomRect = new Rect(x, y, newVisW, newVisH);
>+
>+        Browser.animatedZoomTo(zoomRect);
>+      }
>+      else { // no animated zoom or no zooming at all
>+        //in case zooming is allowed, do it before anything else
>+        if (bv.allowZoom && Services.prefs.getBoolPref("formhelper.autozoom"))
>+          bv.setZoomLevel(zoomLevel);
>+
>+        let vis = bv.getVisibleRect();
>+        // get our x and y in viewport "zoom" coordinates
>+        x = bv.browserToViewport(x);
>+        y = bv.browserToViewport(y);
>+
>+        Browser.contentScrollboxScroller.scrollBy(x-vis.x, y-vis.y);

Nit: add spaces around "-"

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js
>+const gKeyboardObserver = {

As Mark says, KeyboardObserver is enough we don't need the "g".

>+  observe: function vkb_observe(subject, topic, data) {
>+    let rect = Rect.fromRect(JSON.parse(data));
>+    rect.height = rect.bottom - rect.top;
>+    rect.width  = rect.right - rect.left;
>+
>+    Browser._browserView._visibleScreenArea = rect;
>+
>+    BrowserUI.sizeControls(rect.width, rect.height);
>+    FormHelperUI._zoom(FormHelperUI._currentElementRect, FormHelperUI._currentCaretRect);

Nit: I suppose we want to check if the FormHelper is opened at one point before zooming (here or directly inside the function)

>diff --git a/chrome/content/forms.js b/chrome/content/forms.js
>--- a/chrome/content/forms.js
>+++ b/chrome/content/forms.js
>@@ -60,16 +60,18 @@ function FormAssistant() {
>   addMessageListener("FormAssist:Closed", this);
>   addMessageListener("FormAssist:Previous", this);
>   addMessageListener("FormAssist:Next", this);
>   addMessageListener("FormAssist:ChoiceSelect", this);
>   addMessageListener("FormAssist:ChoiceChange", this);
>   addMessageListener("FormAssist:AutoComplete", this);
> 
>   addEventListener("keyup", this, false);

Nit: add a line break


Mostly nits!

I want to have the negativeScreenArea question answered and after that I will test it to ensure everything works as expected but things looks quite good for now.
If you could provide some tests for testing the caret update (manually set, set with letter, "caret attack" with loop, ...) it will be really appreciated (could be done into another patch)

Comment 23

[Jeremias Bosch (:jbos)]

(In reply to comment #22)
> Comment on attachment 467930 [details] [diff] [review]
> now with animated zoom path :D
> 
> >diff --git a/app/mobile.js b/app/mobile.js
> >--- a/app/mobile.js
> >+++ b/app/mobile.js
> >@@ -126,17 +126,27 @@ pref("alerts.height", 50);
> > /* password manager */
> > pref("signon.rememberSignons", true);
> > pref("signon.expireMasterPassword", false);
> > pref("signon.SignonFileName", "signons.txt");
> > 
> > /* form helper */
> > pref("formhelper.enabled", true);
> > pref("formhelper.autozoom", true);
> >+#ifdef MOZ_PLATFORM_MAEMO
> >+pref("formhelper.animatedZoom", false);
> 
> Thanks for adding it, please keep it true by default on all platforms then we
> could switch it into another bug if needed (Layers are coming and should help a
> lot)

On Meego we do not want the animated zoom and scroll yet. There are still to many issues with it (black blinking screen, reset scroll effects when to many keyevents are send at once...) Since we do have a closing timeframe and it does not hurt any delivered platform we really want to have it for now that way.
> >+      // respect the helper container in setting the correct viewAreaHeight
> >+      let viewAreaHeight = visibleScreenArea.height - this._container.getBoundingClientRect().height;
> 
> Since you define visibleScreenArea to Rect(0, 0, 0, 0) does that mean
> viewAreaHeight will be negative if there is not virtual keyboard (because
> visibleScreenArea was not updated and so visibleScreenArea.height will be equal
> to 0)
>
No, Rect(0,0,0,0) is empty = true, which means its the innerWindow Size. This works fine - i tested it on GTK Desktop and resized the Window while typing. 

 

> You can use BrowserUI.toolbarH (I should have point you to that sooner) instead
> of toolbar.boxObject
> 
I tried that, but it is somehow not available there.
 
> If you could provide some tests for testing the caret update (manually set, set
> with letter, "caret attack" with loop, ...) it will be really appreciated
> (could be done into another patch)
Yes, let us have this in another bug, later on. I'm  in a really tide timeframe - well everything for meego must somehow land in the next 10-14 days or so, so, it is quite impossible but well - lets talk about the tests mid to end september

Comment 24

[Jeremias Bosch (:jbos)]

Attachment: Fixed Comments.

Fixed

Comment 25

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

Assigning to Jeremias since this is his patch

Comment 26

[Jeremias Bosch (:jbos)]

Comment on attachment 468815 [details] [diff] [review]
Fixed Comments.

This workaround also the repaint issue with typing on scrolled pages.

Comment 27

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

Comment on attachment 468815 [details] [diff] [review]
Fixed Comments.


> /* form helper */
> pref("formhelper.enabled", true);
> pref("formhelper.autozoom", true);
>+#ifdef MOZ_PLATFORM_MAEMO
>+pref("formhelper.animatedZoom", false);
>+#else
>+pref("formhelper.animatedZoom", true);
> pref("formhelper.restore", false);
>+#endif
>+pref("formhelper.caretLines.portrait", 4);
>+pref("formhelper.caretLines.landscape", 1);
>+pref("formhelper.harmonizeValue", 10);
>+pref("formhelper.margin", 15);


I'm concerned about this part of the patch. You add animatedZoom and turn it off for Mameo. That means you will do immediate zooms (autozoom=true) but not animated. Why? And why is this good for Maemo, but not other platforms?

>+    this._visibleScreenArea = new Rect(0,0,0,0);;

nit: spaces between args


>+    //hide all sidebars, this will adjust the visible rect.
>+    Browser.hideSidebars();
>+    //save the element Rect and reuse it to avoid jumps in cases the element moves slighty on the website.
>+    this._currentElementRect = Rect.fromRect(aElement.rect);
>+    this._zoom(this._currentElementRect, Rect.fromRect(aElement.caretRect));

add a linebreak before the comment. makes it easier to read

Don't worry about the nits unless you need to make another patch for some other reason. We can make some tweaks before landing.

I am interested in the animatedZoom issue though

Comment 28

[Doug Turner (:dougt)]

Comment on attachment 468815 [details] [diff] [review]
Fixed Comments.

not sure if this is at all what we will use post-tiles.

Comment 29

[Jeremias Bosch (:jbos)]

well animated zoom has certain issues.

Start typing while zooming is a bad idea since animated zoom does react strange in cases where a zoom is ongoing. 

Also it feels slow. The same issue is there when having "animated" scroll, in cases you have a key repeat it jumps around because the animation is not finished yet.

If those stuff is fixed well, we can consider in having animations. For now we are basically interested in the best working solution. Having it without animations looks and feels better, to enable it later is quite easy.


(In reply to comment #28)
> Comment on attachment 468815 [details] [diff] [review]
> Fixed Comments.
> 
> not sure if this is at all what we will use post-tiles.

I dont now what you mean?!

Comment 30

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

I like this patch, but do not like the "animatedZoom" changes. Let's fix the animated zoom performace in a different bug and not add shortcuts.

Make a new patch without code to turn off animated zoom. Let's get this core code landed!

File a new bug for animated zoom performance, or wait for layers to land and see how much faster animated zoom becomes.

Comment 31

[Jeremias Bosch (:jbos)]

Attachment: removed the false

Updated

Comment 32

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

Comment on attachment 469517 [details] [diff] [review]
removed the false

>+      // hide titlebar if the remaining space would be smaller than the height of the titlebar itself
>+      // if there is enough space left than adjust the height. Since this adjust the the visible rects
>+      // there is no need to adjust the y later
>+      let toolbar = document.getElementById("toolbar-main");
>+      if (viewAreaHeight - toolbar.boxObject.height <= toolbar.boxObject.height * 2)
>+        Browser.hideTitlebar();
>+      else
>+        viewAreaHeight -= toolbar.boxObject.height;
>+

Could you open a bug for that? I'm pretty sure BrowserUI.toolbarH should work here.

>+const gKeyboardObserver = {
>+  observe: function vkb_observe(subject, topic, data) {
>+    let rect = Rect.fromRect(JSON.parse(data));
>+    rect.height = rect.bottom - rect.top;
>+    rect.width  = rect.right - rect.left;
>+
>+    Browser._browserView._visibleScreenArea = rect;
>+
>+    BrowserUI.sizeControls(rect.width, rect.height);
>+    FormHelperUI._zoom(FormHelperUI._currentElementRect, FormHelperUI._currentCaretRect);
>+  }
>+};

We probably do not want to call FormHelperUI._zoom if the form assistant is closed

r+ with nits addressed

Comment 33

[Jeremias Bosch (:jbos)]

Comment on attachment 469517 [details] [diff] [review]
removed the false

>   /** Zoom and move viewport so that element is legible and touchable. */
>   _zoom: function _formHelperZoom(aElementRect, aCaretRect) {
>     let bv = Browser._browserView;
[..]
>+    if (aElementRect && aCaretRect && this._open) {
>+      this._currentCaretRect = aCaretRect;
> 

The zoom handles that case, it only does something in case the helper is open.

Comment 34

[Jeremias Bosch (:jbos)]

Attachment: Final Patch

This is the final version, i made it able to apply against upstream, check once more through it and check if i covered all nits. (I should have).

Comment 35

[Jeremias Bosch (:jbos)]

Attachment: This is the diff to show the last changes.

Comment 36

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

Comment on attachment 469517 [details] [diff] [review]
removed the false

Asking for new patch. Talked on IRC.

Comment 37

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

Comment on attachment 469799 [details] [diff] [review]
Final Patch

I moved the KeyboardObserver code into FormHelperUI itself and made the small tweak to stop zooming on every keystroke (fixed over IRC)

Comment 38

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

pushed:
http://hg.mozilla.org/mobile-browser/rev/b03fc6bbc4ef

Comment 39

[Oleg Romashin (:romaxa)]

Comment on attachment 467707 [details] [diff] [review]
Promote size changes of the vkb

This patch was not landed to xulrunner :(

Also this patch need to be updated to latest m-c.

Also fix some style issues in new patch:
+        MInputMethodState *inputMethodState = MInputMethodState::instance();
                          ^ - minor

too long, drop args to next line
+          connect(inputMethodState, SIGNAL(inputMethodAreaChanged(const QRect &)), this, SLOT(VisibleScreenAreaChanged(const QRect &)));


+                QString rect = QString("{\"left\": %1, \"top\": %2, \"right\": %3, \"bottom\": %4}")
+                .arg(r.left()).arg(r.top()).arg(r.right()).arg(r.bottom());
                 ^ fix indent here, otherwise it looks like independent lines.

Comment 40

[Jeremias Bosch (:jbos)]

Attachment: Fixed Comments

Done. Thanks

Comment 41

[Jeremias Bosch (:jbos)]

Attachment: Fix typo...

Sorry, somehow there was a wrong piece in this patch.

Comment 42

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/rev/2acd956c4d04

Comment 43

[Jeremias Bosch (:jbos)]

Attachment: Add missing include

Just seen that somehow this include got lost. The patch should fix that.

Comment 44

[Ioana Chiorean]

Attachment: yahoo login

Comment 45

[Ioana Chiorean]

Attachment: gmail login

Comment 46

[Ioana Chiorean]

Attachment: zoom in in http://www-archive.mozilla.org/quality/browser/front-end/testcases/wallet/login.html

Comment 47

[Jeremias Bosch (:jbos)]

Seems to be a issue that the zoom positioning the input behind the awesomebar.
Awesomebar should either turned of or respected. (actually it should be the case)

Let us investigate this. Can you create another bug about this?

Comment 48

[Oleg Romashin (:romaxa)]

Comment on attachment 471054 [details] [diff] [review]
Add missing include

this is already fixed in other bug

Comment 49

[Catalin Suciu [:csuciu]]

Verified fixed on:
Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110919
Firefox/9.0a1 Fennec/9.0a1