592330 - [VKB] Enhance virtual keyboard support in the chrome UI

Status: VERIFIED FIXED

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

Description

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

Attachment: Patch

The patch helps with different areas related to the opening/closing of the vkb:
 - Position of the currently selected textbox when the VKB appears (chrome ui)
 - VKB is closed while panning the right panel
 - VKB is not closed when switching tab
 - VKB is not closed when moving from content to right panel

It still exists some issues:
 * textbox steal focus on mousedown
 * Position of the currently selected textbox when the VKB appears (content)

But I think both of them could be filed into followup.

The patch move some of the code of the MouseModule to the new ScrollUtils but I think this could be useful since the patch in bug 542480 plan to do that for having an access to the getScrollboxFromElement function.

I also use -moz-user-focus a lot here (richlistbox, richlistitem, autocompleteresult, ...) to prevent the focus to be stolen while a user try to pan a list with the VKB open

Comment 1

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

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

Unrequesting review because I've seen a bug when the VKB is opened while editing a bookmark and clicking on the history panel (the focus is not lose and the vkb stays active)

Comment 2

[Tony Chung [:tchung]]

Attachment: 5th entry hidden screenshot

Anything after the 4th entry on site menu will get hidden by the soft keyboard on android.  In this example, there should have been a "Add Bugzilla search" entry

In landscape mode, they soft kb just covers the whole screen so doesnt apply there.

Comment 3

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

(In reply to comment #2)
> Created attachment 470983 [details]
> 5th entry hidden screenshot
> 
> Anything after the 4th entry on site menu will get hidden by the soft keyboard
> on android.  In this example, there should have been a "Add Bugzilla search"
> entry

In my opinion the VKB should be closed when you open the site menu. I will add that to the patch.

Awesome screenshot!

Comment 4

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

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

Some of these | -moz-user-focus: ignore | might change the way rows are
selected and unselected.

I'm not sure bcast_contentShowing is good enough to handle the blur/focus
changes. We need to test more situations where the focus can be placed in other
elements.

Comment 5

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

This bug is about how the VKB should work in the chrome UI.

What I think is:
 * the VKB should disappear when another tab is selected
 * the VKB should disappear when we're switching from content to the right panel
 * the VKB should disappear when we're switching from the right panel to the content

Basically the VKB should disappear when an other screen is showed on the screen (not a popup)


There is also cases where the VKB should not disappear:
 * The VKB should not disapper if a tab is opened in background
 * the VKB should not closed when if it is already opened and the user decide to pan a list
 * the VKB should not closed when if it is already opened and the user decide to pan the content
 * The VKB should not focus another target when the user is putting is finger onto an other textbox without releasing it (it can be for panning)

Basically the VKB should stays active if the user does _not_ dismis it intentionally

The patch is trying to do a first step into those directions.
Madhava could you confirm how the VKB should acts into these situations?

Comment 6

[Madhava Enros [:madhava]]

Confirmed!  This all makes sense - the keyboard going away should not seem to happen randomly, uninitiated by the user.

Comment 8

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

Attachment: Patch v0.2

(In reply to comment #4)
> Comment on attachment 470820 [details] [diff] [review]
> Patch
> 
> Some of these | -moz-user-focus: ignore | might change the way rows are
> selected and unselected.

This does not change the selection/unselection of rows or at least not in a visible way.

> I'm not sure bcast_contentShowing is good enough to handle the blur/focus
> changes. We need to test more situations where the focus can be placed in other
> elements.

This will probably not handle all the VKB cases, especially the Meego cases but this patch is mostly about the chrome UI and handle all cases I've encountered.

I'm pretty sure jbos has open a bug for the Meego case... bug 593755.

Comment 10

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

Attachment: Patch v0.2 - unbitrotted

Comment 11

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

Attachment: Patch part 2 - prevent textbox to steal focus

This is an additional patch coming on top of the previous one to prevent textbox to steal focus when panning start above one of them.

Comment 12

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

Comment on attachment 479022 [details] [diff] [review]
Patch v0.2 - unbitrotted


>diff -r a23fe255b6f6 chrome/content/browser-ui.js

>+  updateCurrentBrowser: function _updateCurrentBrowser() {
>+    let state = (Elements.contentShowing.getAttribute("disabled") == "true") ? "Blur" : "Focus";
>+    Browser.selectedBrowser.messageManager.sendAsyncMessage("Browser:" + state, {});
>+  },

updateUIFocus might be a better name here. updateCurrentBrowser sounds like it has other meanings.


>diff -r a23fe255b6f6 chrome/content/browser.js

>-      let curEl = document.activeElement;
>-      if (curEl && curEl.id != "inputhandler-overlay" && curEl.scrollIntoView)
>-        curEl.scrollIntoView(false);
>+      // We want to keep the current focused element into view if possible
>+      let currentElement = document.activeElement;
>+      let [scrollbox, scrollInterface] = ScrollUtils.getScrollboxFromElement(currentElement);
>+      if (currentElement && scrollbox && currentElement != scrollbox) {
>+        // retrieve the direct child of the scrollbox
>+        while (currentElement.parentNode != scrollbox)
>+          currentElement = currentElement.parentNode;
>+
>+        setTimeout(function() { scrollInterface.ensureElementIsVisible(currentElement) }, 0);
>+      }

Make sure we don't regress the reason why "inputhandler-overlay" was added to the removed lines of code. Find the bug and re-verify with your changes.

r+ but make sure we don't regress previous fixes

Comment 13

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

> >diff -r a23fe255b6f6 chrome/content/browser.js
> 
> >-      let curEl = document.activeElement;
> >-      if (curEl && curEl.id != "inputhandler-overlay" && curEl.scrollIntoView)
> >-        curEl.scrollIntoView(false);
> >+      // We want to keep the current focused element into view if possible
> >+      let currentElement = document.activeElement;
> >+      let [scrollbox, scrollInterface] = ScrollUtils.getScrollboxFromElement(currentElement);
> >+      if (currentElement && scrollbox && currentElement != scrollbox) {
> >+        // retrieve the direct child of the scrollbox
> >+        while (currentElement.parentNode != scrollbox)
> >+          currentElement = currentElement.parentNode;
> >+
> >+        setTimeout(function() { scrollInterface.ensureElementIsVisible(currentElement) }, 0);
> >+      }
> 
> Make sure we don't regress the reason why "inputhandler-overlay" was added to
> the removed lines of code. Find the bug and re-verify with your changes.

It comes from bug 598234 and it looks like I need to add it back. I'll update the patch for it.

Comment 14

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

Attachment: Patch part 2 v0.2 - prevent textbox to steal focus

Comment 15

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

Part 1:
http://hg.mozilla.org/mobile-browser/rev/686ec7b3c735
http://hg.mozilla.org/mobile-browser/rev/8a7b2ef86a9a

Comment 16

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

Comment on attachment 479262 [details] [diff] [review]
Patch part 2 v0.2 - prevent textbox to steal focus

adding back the r?

Comment 17

[Benjamin Stover (:stechz)]

Comment on attachment 479262 [details] [diff] [review]
Patch part 2 v0.2 - prevent textbox to steal focus

>-function MouseModule(owner, browserViewContainer) {
>-  this._owner = owner;
>-  this._browserViewContainer = browserViewContainer;
>+function MouseModule() {

Good catch.

>   this._dragData = new DragData(kTapRadius);
> 
>   this._dragger = null;
>+  this._inputField = null;
> 
>   this._downUpEvents = [];
>   this._targetScrollInterface = null;
> 
>   this._kinetic = new KineticController(this._dragBy.bind(this),
>                                         this._kineticStop.bind(this));
> 
>   this._singleClickTimeout = new Util.Timeout(this._doSingleClick.bind(this));
>@@ -134,24 +133,42 @@ MouseModule.prototype = {
>       default: {
>         // Filter out mouse events that aren't first button
>         if (aEvent.button !== 0)
>           break;
> 
>         switch (aEvent.type) {
>           case "mousedown":
>             this._onMouseDown(aEvent);
>+
>+            // When panning starts over an input field, focus should not change
>+            let inputField = this._getTargetInputField(aEvent.originalTarget);
>+            if (inputField && this._dragger) {
>+              this._inputField = inputField;
>+              aEvent.preventDefault();
>+              aEvent.stopPropagation();
>+            }

This is probably a good idea. This code should go in _onMouseDown.  Ideally, this eventually should go into platform, but that's sort of true for *all* of mousemodule, so that makes me feel like this is the right place for your code.

>             break;
>           case "mousemove":
>             aEvent.stopPropagation();
>             aEvent.preventDefault();
>             this._onMouseMove(aEvent);
>             break;
>           case "mouseup":
>             this._onMouseUp(aEvent);
>+
>+            // Move the caret to the end of the target input field and focus it
>+            if (this._inputField && !this._dragData.isPan()) {
>+              let inputField = this._inputField;
>+              let textLength = inputField.textLength;
>+              inputField.setSelectionRange(textLength, textLength);
>+              inputField.focus();
>+            }
>+            this._inputField = null;
>+

Why select all input and move cursor to the end? Move this code into _onMouseUp.

>             break;
>           case "click":
>             aEvent.stopPropagation();
>             aEvent.preventDefault();
>             aEvent.target.removeEventListener("click", this, true);
>             break;
>         }
>       }
>@@ -352,16 +369,17 @@ MouseModule.prototype = {
>     // Kinetic panning could finish while user is panning, so don't finish
>     // the pan just yet.
>     let dragData = this._dragData;
>     if (!dragData.dragging) {
>       this._dragger.dragStop(0, 0, this._targetScrollInterface);
>       let event = document.createEvent("Events");
>       event.initEvent("PanFinished", true, false);
>       document.dispatchEvent(event);
>+      this._dragger = null;
>     }
>   },

Why here? If we need it here, we'll also need it when a pan doesn't start kinetic panning.

>+  /* XXXvn this can potentially be moved into ScrollUtils */
>+  _getTargetInputField: function _getTargetInputField(aTarget) {
>+    let focusedElement = document.commandDispatcher.focusedElement;
>+    let parentNode = aTarget.parentNode;
>+
>+    let inputField = null;
>+    if (aTarget.mozIsTextField && aTarget.mozIsTextField(false) && focusedElement != aTarget)
>+      inputField = aEventTarget;
>+    else if (parentNode.mozIsTextField && parentNode.mozIsTextField(false) && focusedElement != parentNode)
>+      inputField = parentNode;
>+    else if (aTarget instanceof XULElement && aTarget.inputField)
>+      inputField = aTarget.inputField;
>+
>+    return inputField;
>+  },
>+

What is ScrollUtils?

Looks good with code review changes.

Comment 18

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

Attachment: Patch v0.2

> >+            if (this._inputField && !this._dragData.isPan()) {
> >+              let inputField = this._inputField;
> >+              let textLength = inputField.textLength;
> >+              inputField.setSelectionRange(textLength, textLength);
> >+              inputField.focus();
> >+            }
> >+            this._inputField = null;
> >+
> 
> Why select all input and move cursor to the end? Move this code into
> _onMouseUp.


There is not selection at all now and this part is pretty rude. This is useful for moving the cursor always at the same place when the user try to focus a field, otherwise it can be at the previously focused place when leaving which does not make sense here since (real) selection is not implemented yet.

Ideally we want to put the cursor where the user has clicked but this was done by mousedown and I have not digged into how to emulate it now.
I can open a bug for that.

> >+  /* XXXvn this can potentially be moved into ScrollUtils */
> >+  _getTargetInputField: function _getTargetInputField(aTarget) {
> >+    let focusedElement = document.commandDispatcher.focusedElement;
> >+    let parentNode = aTarget.parentNode;
> >+
> >+    let inputField = null;
> >+    if (aTarget.mozIsTextField && aTarget.mozIsTextField(false) && focusedElement != aTarget)
> >+      inputField = aEventTarget;
> >+    else if (parentNode.mozIsTextField && parentNode.mozIsTextField(false) && focusedElement != parentNode)
> >+      inputField = parentNode;
> >+    else if (aTarget instanceof XULElement && aTarget.inputField)
> >+      inputField = aTarget.inputField;
> >+
> >+    return inputField;
> >+  },
> >+
> 
> What is ScrollUtils?

ScrollUtils is a singleton with useful functions the outside word can called if needed.

Comment 19

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

I've filled bug 602666 for the caret issue mentionned by stechz.

http://hg.mozilla.org/mobile-browser/rev/5524469c8f29

Comment 20

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

litmus test case created:
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=13657

creating more later.  keeping this open for more litmus test creation.

Comment 21

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

https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=13669

Comment 22

[Cristian Nicolae (:xti)]

Verified fixed on:
Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110915
Firefox/9.0a1 Fennec/9.0a1
Device: Samsung Galaxy S
OS: Android 2.2

Note:
Nice screenshot Tony! :D I wish to see more in the future filled bugs :)