819598 - When opening keyboard, don't scrollIntoView() input elements that are already in view

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Chris Peterson [:cpeterson]]

When tapping an input element that is "above the fold" of where the keyboard will be, the element is unnecessarily scrollIntoView()'d. This causes the page jump around, which looks bad.

Unfortunately, there is not an easy way to determine whether an element is already in view. You need to manually compare the elements' position and extents with the app's client area (minus the keyboard height).

Comment 1

[Joe Cheng [:jcheng] (please needinfo)]

BB+, P3 - less severe usability issue

Comment 2

[Chris Lee [:clee]]

This is a major issue based on our usability testing as well as core usability for inputing anything into fields.  

We need this addressed and marking as P1.  Thanks.

Comment 3

[James Lal [:lightsofapollo]]

Adding to my queue let me know if you started on this already.

Comment 4

[Chris Peterson [:cpeterson]]

(In reply to James Lal [:lightsofapollo] from comment #3)
> Adding to my queue let me know if you started on this already.

lightsofapollo, thanks! <:)

Comment 5

[James Lal [:lightsofapollo]]

There two issues here I think:

- we should scroll whichever direction is closer (align top or bottom).
  (where as now we always align to the bottom)

- We should not scroll an element into view that is already visible.

I tested a few sites in the browser that have multiple field elements and I found it very difficult compared to other smartphones to edit content. There seems be a lot of "jumpyness".

Comment 6

[James Lal [:lightsofapollo]]

Casey, Can you confirm the above logic? Should be fairly simple to fix once we know what we want to do.

Comment 7

[Casey Yee]

(In reply to James Lal [:lightsofapollo] from comment #5)
> There two issues here I think:
> 
> - we should scroll whichever direction is closer (align top or bottom).
>   (where as now we always align to the bottom)

This makes sense to me.   It would also be beneficial to add additional pixels for padding so that the input fields are not right up against the top or bottom extents of the screen.

> 
> - We should not scroll an element into view that is already visible.
> 

I agree with this.   We'll also want to scroll the field into full view if it's only partially visible as well.

Comment 8

[James Lal [:lightsofapollo]]

I have a "working" patch but it exposes some flaws with the use of scrollIntoView and my first attempt logic. This patch (IMO) should really be "focus inputs correctly".

Its very easy to detect if the input element is visible and avoid the sync but its very difficult to align it correctly because scrollIntoView only takes into account the input itself and not any padding (which effectively makes labels invisible if you scroll down then re-focus).

I am going to attempt to try "centering" the element the minimal amount accounting for padding so we can actually make a usability gain here.

Comment 9

[Casey Yee]

Good point about the input field labeling.   I think centering is a good solution to ensure that the user doesn't miss out on any labels above and below the fields.

Comment 10

[Casey Yee]

From my conversations with James we have decided due to complexity of centering input elements within the viewport to implement a half-measure solution for v1.

When a input field is focused and is not fully in view.  The viewport will scroll so that the input field aligns with the top or bottom of the viewport, whichever is nearest.

Comment 11

[James Lal [:lightsofapollo]]

Attachment: Only focus inputs that are not visible. Focus them to top or bottom rather then only top.

Note, we intentionally do _not_ focus when the X axis (left) is out of alignment.

This patch probably needs a little more documentation.. I am in the process of writing some marionette tests (in gaia) to excise this more fully.

I tested this with some web content but also tested it to handle:

- top level forms
- multiple levels of iframes (iframe -> iframe)
- multiple levels of scrollable content (overflow:scroll -> overflow:scroll).

Comment 12

[James Lal [:lightsofapollo]]

The other flaw here is when the element is zoomed (in the browser app) the "focused" logic does not seem to work correctly... I will see if I can get that working as expected next.

Comment 13

[Chris Peterson [:cpeterson]]

Comment on attachment 695094 [details] [diff] [review]
Only focus inputs that are not visible. Focus them to top or bottom rather then only top.

Review of attachment 695094 [details] [diff] [review]:
-----------------------------------------------------------------

I found a bug when testing on my phone:
  STR:
  1. Go to Contacts app's "Add contact" screen
  2. Scroll down the page and test the "align to top" code path
  RESULT: The focused input element aligns to the top of the *screen*. The Contacts app's "Settings" header shifts from the top of the screen to align with the keyboard.

The "align to bottom" code path worked correctly for me in the Contacts app.

::: b2g/chrome/content/forms.js
@@ +31,5 @@
>  let HTMLOptGroupElement = Ci.nsIDOMHTMLOptGroupElement;
>  let HTMLOptionElement = Ci.nsIDOMHTMLOptionElement;
> +let HTMLBodyElement = Ci.nsIDOMHTMLBodyElement;
> +
> +let focusElement = (function() {

Why is focusElement an immediately-invoked function expression instead of just a function (with local functions)? Can you move the focusElement definition within the FormAssistant scope to avoid polluting the global namespace?

@@ +48,5 @@
> +      var isAuto = (overflow.indexOf('auto') != -1 &&
> +                   (rect.height < node.scrollHeight ||
> +                    rect.width < node.scrollWidth));
> +
> +      var isScroll = (overflow.indexOf('scroll') != -1);

The isScroll check does not depend on rect or isAuto. You can bypass all the work by moving the isScroll check above rect and isAuto and return node early.

@@ +68,5 @@
> +  function yAxisVisible(top, height, maxHeight) {
> +    if (top > 0 && (top + height) < maxHeight) {
> +      return true;
> +    }
> +    return false;

You can simplify this function by replacing the if statement with just `return (top > 0 && (top + height) < maxHeight);`.

@@ +75,5 @@
> +  function scrollablesVisible(element, rect) {
> +    while ((element = findScrollable(element))) {
> +      if (element.window && element.self === element) {
> +        break;
> +      } else {

You don't need this `else` (and its extra indentation) because the if statement breaks from the while loop.

@@ +123,5 @@
> +          result.height,
> +          parent.innerHeight
> +        );
> +
> +        isVisible = isVisible && scrollablesVisible(

These nested `isVisible` checks that keep checking isVisible are a little confusing. You might be able to simplify it with something like:

    isVisible = isVisible &&
                yAxisVisible(result.top, result.height, parent.innerHeight) &&
                scrollablesVisible(element, result);

@@ +154,5 @@
> +    if (rect.top > (window.innerHeight / 2)) {
> +      // align to bottom
> +      element.scrollIntoView(false);
> +    } else {
> +      // align to top

You can simplify this function by replacing this if statement with something like:
    var alignWithTop = (rect.top < (window.innerHeight / 2));
    element.scrollIntoView(alignWithTop);

Comment 14

[James Lal [:lightsofapollo]]

Re Contacts:

I don't think this is a "bug" exactly... 

From MDN: "If true, the scrolled element is aligned with the top of the scroll area"

The logic to detect if the element is visible or not works (as far as I can see) but scrollIntoView always aligns with the screen... and this case that causes the alignment of the contacts "views" to overlap as you observed...

We should not break any content at this point so I think we have two options...

- only align to the bottom ( everyone already developed to this )
- manually scroll elements into the viewport (don't use scrollIntoView)

Re "focusElement":

It's the module pattern but only returns a single function.
As far as I can tell its not exposing globals (unless let is more then file scope).

Next patch up soon...

Comment 15

[James Lal [:lightsofapollo]]

Attachment: Focus Patch v2 only focuses to bottom

Incorporates review suggestions, only focuses to bottom of frame (for now).

I moved the focusElement logic into its own object (which seems to be more common then the pattern I used throughout gecko)... I don't feel like that logic should be in FormAssistant and we can add more functionality here later.

I tested this on your contacts issue and my own manual tests...

Comment 16

[David Flanagan [:djf]]

Comment on attachment 695783 [details] [diff] [review]
Focus Patch v2 only focuses to bottom

Review of attachment 695783 [details] [diff] [review]:
-----------------------------------------------------------------

James,

I don't envy you having to write this code.  This (nested scrollable elements and frames) is the part of the DOM that I fear the most!

There are a couple of nits in my comments, but mostly questions about the code.  I don't feel as if I fully understand the code. And given the sensitiviity of forms.js, I'm not ready to give r+ yet.

If you address the nits, and add some more comments explaining what is going on, I'd be happy to review it again.

::: b2g/chrome/content/forms.js
@@ +32,5 @@
>  let HTMLOptionElement = Ci.nsIDOMHTMLOptionElement;
> +let HTMLBodyElement = Ci.nsIDOMHTMLBodyElement;
> +
> +let FormVisibility = {
> +

I don't understand this code pattern. If it was me, I'd just make isVisible be a function, and nest the others within it...

But this works, too.

@@ +40,5 @@
> +   * @param {HTMLElement} node element to start search at.
> +   * @return {Window|HTMLElement|Null} null when none are found window/element otherwise.
> +   */
> +  findScrollable: function fv_findScrollable(node) {
> +    let nodeContent = node.ownerDocument.defaultView;

I don't get this variable name.  How about "win" instead?

@@ +56,5 @@
> +      let isAuto = (overflow.indexOf('auto') != -1 &&
> +                   (rect.height < node.scrollHeight ||
> +                    rect.width < node.scrollWidth));
> +
> +

Is it necessary to find every potentially scrollable element as you do here, or would it suffice to just find elements that have actually been scrolled?

That is, could you drop the getComputedStyle() and getPropertyValue() code above and just look for elements where scrollTop !== 0 (or scrollLeft !== 0 if you're going to deal with the X axis)?

@@ +79,5 @@
> +   * @param {Number} maxHeight of the window.
> +   * @return {Boolean} true when visible.
> +   */
> +  yAxisVisible: function fv_yAxisVisible(top, height, maxHeight) {
> +    return (top > 0 && (top + height) < maxHeight);

This looks to me like it is testing that both the top and the bottom of the element are visible.  If that is intended, then you should update or clarify the comment.

Is the idea that if an element is larger than the viewport isVisible() will always return false and we'll call scrollIntoView()?

And are we okay with the fact that something that has scrolled sideways off the screen will not be made fully visible?  Y axis only here?

@@ +95,5 @@
> +      if (element.window && element.self === element)
> +        break;
> +
> +      let offset = element.getBoundingClientRect();
> +      let adjusted = {

no need to create an object here since you're only adjusting one property. How about just 

let adjustedTop = pos.top - offset.top

It has taken me a couple of minutes to figure out what is being computed with this subtraction.  I think I've got it now: this is the position of the top edge of the original element within this scrollable element, right?

Would you add a comment to that effect?

@@ +142,5 @@
> +      let frame = parent.frameElement;
> +      visible = visible &&
> +                this.yAxisVisible(pos.top, pos.height, parent.innerHeight) &&
> +                this.scrollablesVisible(element, pos);
> +

once visible becomes false it looks like it stays false, right?

So exit early?

  if (!visible) return false;

@@ +146,5 @@
> +
> +      if (frame) {
> +        let frameRect = frame.getBoundingClientRect();
> +        let top =
> +          parseInt(parent.getComputedStyle(frame, '').borderTopWidth, 10);

Can you use frame.clientTop here instead of doing the CSS query?

See http://www.w3.org/TR/cssom-view/#client-attributes

@@ +148,5 @@
> +        let frameRect = frame.getBoundingClientRect();
> +        let top =
> +          parseInt(parent.getComputedStyle(frame, '').borderTopWidth, 10);
> +
> +        pos.top += frameRect.top + top;

I think what you're doing here is converting the viewport coordinates of the element to coordinate system of the containing viewport.  But I'd like a comment to clarify that, please.

Comment 17

[James Lal [:lightsofapollo]]

Attachment: Focus Patch v3 only focuses to bottom

Optimizations more comments!

Comment 18

[David Flanagan [:djf]]

Comment on attachment 696819 [details] [diff] [review]
Focus Patch v3 only focuses to bottom

Review of attachment 696819 [details] [diff] [review]:
-----------------------------------------------------------------

A couple more minor nits, but r+ assuming that you've tested it thoroughly.

::: b2g/chrome/content/forms.js
@@ +59,5 @@
> +        // this node does not effect where we think
> +        // the node is even if it is scrollable it has not hidden
> +        // the element we are looking for.
> +        node = node.parentNode;
> +        continue;

unnecessary continue.  Maybe you're using it to parallel the return above. But the else clause is unnecessary too, actually.

@@ +164,5 @@
> +
> +      if (frame) {
> +        let frameRect = frame.getBoundingClientRect();
> +
> +        pos.top += frameRect.top + frame.clientTop;

It would be nice to have a comment explaining this line.

Comment 19

[James Lal [:lightsofapollo]]

This is working well but needs some final testing... Obviously this was the perfect time to figure out how to brick my otoro. I will get it re-flashed when the office opens Wednesday and then we can land this...

Comment 20

[Andrew Overholt [:overholt]]

Is this now ready for landing, James?

Comment 21

[James Lal [:lightsofapollo]]

Should be but I bricked my unagi while testing the final patch... I am PTO today but will run into the office and grab another (or borrows someones).

I am confident this works as is but given the importance of keyboard I wanted to re-check everything before landing.

Comment 22

[David Flanagan [:djf]]

Bricked, or bug 825840?

Comment 23

[James Lal [:lightsofapollo]]

Tested by directly modifying the omni.jar on Dec 31 build and latest via contacts app + vingtetun's object-dir.

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

I'll take care of landing this together with bug 823645 as soon as the trees re-open. Both these changes conflict and I had to manually re-apply the patch, nothing too bad, though.

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4bc33595dca

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f4bc33595dca

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/845d526dc539
https://hg.mozilla.org/releases/mozilla-b2g18/rev/b6f964396db1

Comment 28

[Walter Chen[:ypwalter][:wachen]]

Verified fixed in pvt unagi engineer build 2013-2-4