689877 - Fennec does not scroll view into plugin editable field

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Panning/Zooming, defect)

Description

[Oleg Romashin (:romaxa)]

Attachment: Scroll plugin input filed into view

When plugin input field is focused, we do not do anything in order to scroll plugin field into view.
Here is some wip approach how to fix that problem

Comment 1

[Oleg Romashin (:romaxa)]

Comment on attachment 563002 [details] [diff] [review]
Scroll plugin input filed into view

This approach is just quick way to scroll plugin input field (using last click point) without hacking formhelper functionality.
couple of questions here:
1) I've found that we do subtract toolbar height from ViewableArea for table but not for default UI.... and that seems breaking proper positioning of input field... so that is why I added height -= BrowserUI.toolbarH;
2) Should I integrate HTMLEmbed tag handling into form helper, or should I keep I as separate thing?

Comment 2

[Matt Brubeck (:mbrubeck)]

Comment on attachment 563002 [details] [diff] [review]
Scroll plugin input filed into view

Looks okay to me overall.  Some feedback below:

>   observe: function va_observe(aSubject, aTopic, aData) {
> #if MOZ_PLATFORM_MAEMO == 6
>     let rect = Rect.fromRect(JSON.parse(aData));
>     let height = rect.bottom - rect.top;
>     let width = rect.right - rect.left;
>+    height -= BrowserUI.toolbarH;
>     if (height == window.innerHeight && width == window.innerWidth) {
>       this._height = null;
>       this._width = null;
>     }

Why is this change needed?  Won't this guarantee that the (height == window.innerHeight) check on the next line always fails?  In tablet mode (on a hypothetical Maemo 6 tablet) it will also do weird things because the "ViewableAreaObserver.height" getter subtracts toolbarH again.

>+    window.addEventListener("KeyboardChanged", function(aEvent) {
>+      window.removeEventListener("KeyboardChanged", arguments.callee, false);

Please avoid using arguments.callee - among other reasons, it won't be allowed in JS strict mode.  This can be changed to:

    window.addEventListener("KeyboardChanged", function listener(aEvent) {
      window.removeEventListener("KeyboardChanged", listener, false);

>+            let timer = new Util.Timeout(function() {
>+              // ...
>+            });
>+            timer.once(0);

You should store a non-local reference to the timer, or just use setTimeout, because of http://www.joshmatthews.net/blog/2011/03/nsitimer-anti-pattern/

Comment 3

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

Comment on attachment 563002 [details] [diff] [review]
Scroll plugin input filed into view

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

>+  _waitAndZoom: function formHelperWaitAndZoom(elementRect, elSubRect) {
>+    if (ViewableAreaObserver.isKeyboardOpened) {
>+      this._zoom(elementRect, elSubRect);
>+      return;
>+    }
>+
>+    let self = this;
>+    window.addEventListener("KeyboardChanged", function(aEvent) {
>+      window.removeEventListener("KeyboardChanged", arguments.callee, false);
>+      if (ViewableAreaObserver.isKeyboardOpened) {
>+        self._zoom(elementRect, elSubRect);
>+      }
>+    }, false);
>+  },

Why do we need this when we already have a _waitForKeyboard mechanism used when calling show() ?


>     switch (aMessage.name) {
>+      case "FormAssist:ShowPlugins":
>+        this._currentElementRect = Rect.fromRect(json.rect);
>+        this._currentBrowser = getBrowser();
>+        let caretRect = new Rect(json.pressPoint.x, json.pressPoint.y, 1, 50);
>+        this._waitAndZoom(Rect.fromRect(json.rect), caretRect);

Why not use show?
I wish we were using aMessage.target and not getBrowser() - I see it's used in ForAssist:Show too

>diff --git a/mobile/chrome/content/content.js b/mobile/chrome/content/content.js

>+              if (utils.IMEStatus == 3) {

Do we have a real constant name and not just '3' ?

(and Matt's feedback)

f+ (overall, it looks OK)

Comment 4

[Oleg Romashin (:romaxa)]

Attachment: Scroll plugin input filed into view

Comment 5

[Oleg Romashin (:romaxa)]

Comment on attachment 563559 [details] [diff] [review]
Scroll plugin input filed into view

Hop it is ok to :Show, and hardcode some params for plugin case

Comment 6

[Matt Brubeck (:mbrubeck)]

Comment on attachment 563559 [details] [diff] [review]
Scroll plugin input filed into view

The code here looks good, but I have just one change I'd like to see.  I think this code belongs in FormAssistant (chrome/content/forms.js).  Can you make it so this check is called inside of FormAssistant.prototype.open?  I think you could put it in a new function that you call if the _isValidElement check fails.

Sorry for not thinking of this in my previous feedback.

Comment 7

[Oleg Romashin (:romaxa)]

Attachment: Scroll plugin input filed into view

Comment 8

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

Just an FYI: Content.formAssistant.open is used in some tests:

* http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/tests/remote_contentpopup.js#7
* http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/tests/remote_formsZoom.js#7
* http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/tests/remote_forms.js#92 (multiple)
* http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/tests/remote_autocomplete.js#7

Maybe we should make the x, y params optional. In the open call, if the valsues are undefined, default to 0?

Comment 9

[Oleg Romashin (:romaxa)]

ok, make sense

Comment 10

[Oleg Romashin (:romaxa)]

Attachment: Scroll plugin input filed into view

Added default param

Comment 11

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

Comment on attachment 563823 [details] [diff] [review]
Scroll plugin input filed into view

Should work OK

Comment 12

[Matt Brubeck (:mbrubeck)]

Pushed to Try (with some other stuff I'm hoping to check in):
https://tbpl.mozilla.org/?tree=Try&rev=654c636e0207

Comment 13

[Matt Brubeck (:mbrubeck)]

Try caught a syntax error caused by an extra curly brace after an "if" statement in content.js.

Comment 14

[Oleg Romashin (:romaxa)]

Attachment: Scroll plugin input filed into view

Fixed syntax error... wrongly synced patch between mozilla-7.0 release branch

Comment 15

[Matt Brubeck (:mbrubeck)]

New Try run is green: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=cdb8f52e8801

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/49ae33f133dd

Comment 16

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/49ae33f133dd