725018 - Tapping into a field does not zoom into the field

Status: VERIFIED FIXED

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

Description

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

1. go to http://people.mozilla.com/~nhirata/html_tp/formsninput.html
2. click in a field

Expected: zoom in 
Actual: no zoom

Note: https://litmus.mozilla.org/show_test.cgi?id=43138 fails because of this.
Samsung Galaxy S 2, 2.3, Nightly Aurora 2/7/2012

Comment 1

[Aaron Train [:aaronmt]]

Regression from XUL Fennec which exposed this functionality.

Comment 2

[Martijn Wargers (dead)]

I see this also on Native Fennec 10, so I don't think this is a regression in Native Fennec.

Comment 4

[JP Rosevear [:jpr]]

Release blockers.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

form assistant isn't planned for this release, not blocking

Comment 6

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

I think we need to reconsider whether we actually want this feature. I am not missing it.

Madhava?

Comment 7

[Martijn Wargers (dead)]

I'm missing it when the text field is smallish.

Comment 8

[Paul [pwd]]

I remember filing Bug 659949. So I'd be happy without this.

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #6)
> I think we need to reconsider whether we actually want this feature. I am
> not missing it.
> 
> Madhava?
Ian, can you answer this?

Comment 10

[Ian Barlow (:ibarlow)]

I'd like to get it back in if possible. As Martijn said it's a nice detail for cases where you tap on a small text field, so you can actually see what you're typing.

Comment 11

[JP Rosevear [:jpr]]

Ian answered the question, moving back to nobody for now.

Comment 12

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

(In reply to Ian Barlow (:ibarlow) from comment #10)
> I'd like to get it back in if possible. As Martijn said it's a nice detail
> for cases where you tap on a small text field, so you can actually see what
> you're typing.

We need limits on this zooming though. Doing small zooms will be more painful than helpful. I still say this is not desirable, let alone a blocker.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

debatably nice to have, not blocking the release

Comment 14

[Kevin Brosnan [Ex-Mozilla]]

Assuming this was to be assigned to mfinkle.

Comment 15

[Wesley Johnston (:wesj)]

We have a ZoomToRect message that should make this fairly easy. We also have some logic in the double tap zoom code to figure out if an object is (mostly) on screen. It might be good to try and reuse that if possible.

Comment 16

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

Mike - Talk to Wes for direction on this.

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I do not have time to work on this currently, but if it's still open, I will most likely try to do so in late December (university break).

I apologize that I did not hand this off sooner.

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Making this a mentored bug since I think it should be fairly straightforward.

Comment 20

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

This is a pretty simple naive fix. I'm worried that it doesn't do quite as much work as our old implementation, but it doesn't seem to fare much worse in a bunch of test cases I tried. And the animated zoom is a big improvement IMO.

Basic idea is that we only want to zoom to fields when you actively tapped on them (or if you used "Next" to get there, but our ability to decide if we should show "Next" seems to suck...), so I just did this right in our tap detction code. That means if you tap on an already focused field, we may zoom out to show it again. I consider that kinda useful, but someone trying to position the cursor (without using the cursor control?) may not.

There's still a little strangeness in places. A couple that stand out that I'd like to fix

1.) the long field on bugzilla.mozilla.org will sometimes zoom in and then jank to the side

2.) There's some dumbness zooming in in cases where a field is already mostly visible (i.e. mobile Google.com). zoomToRect should prevent that but it must be too lenient.

3.) occasionally fields will zoom in before the keyboard appears and then be underneath it. There's also still a bit to much jank when the keyboard comes up (although it feels better to me now). Maybe we're better to move these transitions to happen after the keyboard appears instead...

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 695060 [details] [diff] [review]
WIP Patch

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

Overall the changes seem sane. If this works in all cases then that's great. It would be good to test on a device with a physical keyboard as well as a device with a virtual keyboard. Also you should probably update testVkbOverlap to account for this. I imagine the test will still pass because "newCount" will be the number of green pixels after zooming in, which will be more than the expected number while zoomed out. We should update the test to require newCount to be larger than it is currently to account for the zooming behaviour.

::: mobile/android/chrome/content/browser.js
@@ +1068,5 @@
>        this.setPreferences(aData);
>      } else if (aTopic == "ScrollTo:FocusedInput") {
> +      // Java only asks us to scroll when the viewport size is changing (usually page load)
> +      // avoid zooming the page in this case
> +      this.scrollToFocusedInput(browser, false);

I don't think this comment is right. The viewport size changes when the virtual keyboard comes up, which is why we trigger the ScrollTo:FocusedInput message when the viewport size changes. If the new call to BrowserApp.scrollToFocusedInput you added below gets run every time an input field gets focus, then you can take out this call entirely (and the ScrollTo:FocusedInput message) as it should be entirely redundant.

Comment 22

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #21)
> I don't think this comment is right. The viewport size changes when the
> virtual keyboard comes up, which is why we trigger the ScrollTo:FocusedInput
> message when the viewport size changes. If the new call to
> BrowserApp.scrollToFocusedInput you added below gets run every time an input
> field gets focus, then you can take out this call entirely (and the
> ScrollTo:FocusedInput message) as it should be entirely redundant.

Oh, ignore most of this ^. I re-read your comment #20 and it makes sense to leave that code in. However the comment text still needs to be fixed.

(In reply to Wesley Johnston (:wesj) from comment #20)
> 
> 3.) occasionally fields will zoom in before the keyboard appears and then be
> underneath it. There's also still a bit to much jank when the keyboard comes
> up (although it feels better to me now). Maybe we're better to move these
> transitions to happen after the keyboard appears instead...

My bugzilla-fu is failing me, but I remember making this comment on some bug somewhere. So yes, I agree.

Comment 23

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Great. Lets try this then! I added a little fix to our "don't zoom if this already zoomed code". Since we don't zoom out in these states, it was leading to times you'd tap an input field that was only partially on screen and nothing would happen. Even with double tap zoom, we do want to SCROLL to an element if possible, even if we can't zoom to it. That's a bit harder with vertical scrolling as its possible the element is just too tall to fit on the page at any available zoom level, but I think it should always be possible to horizontally fit things(?). 

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #22)
> My bugzilla-fu is failing me, but I remember making this comment on some bug
> somewhere. So yes, I agree.

Ahh. And I didn't realize this code was supposedly doing that already. Removing this is harder. It makes things like testVirtualKdb fail. We could try moving this message to GeckoInputConnection.notifyIMEEnabled(), but I keep bouncing around. We really want to do this anytime the viewport size changes, so I think maybe we're best to leave it where it is...

Comment 24

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2 - Fix form helper

The form helper really wasn't aware of our scrolling or this new zooming code. This is one way to make it aware. This patch exposes a little bit in PanZoomController, so I'm not sure if you'll love it or not. I think having a way for listeners to know when panning/zooming starts and ends is probably useful?

Comment 25

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 695890 [details] [diff] [review]
Patch v1

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

::: mobile/android/chrome/content/browser.js
@@ +3996,5 @@
>      let dw = (aRect.width - vRect.width);
>      let dx = (aRect.x - vRect.x);
>  
> +    if (Math.abs(aViewport.zoom - maxZoomAllowed) < 1e-6 && overlap.width / aRect.width > 0.9) {
> +      // we're already at the max zoom and the block is not spilling off the side of the screen so that even

This is the only part of the patch that I'm worried about. Before we would always return true if we were near the max zoom and now we might return false in some of those cases, causing a regression in bug 783208 in some scenarios. As long as we don't regress on the actual case in bug 783208's comment #0 it's unlikely to be a big deal but still something worth checking.

Comment 26

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 695891 [details] [diff] [review]
Patch 2/2 - Fix form helper

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

Yeah, I'm not a huge fan of this approach. I'd much rather see PZC directly broadcast a message that browser.js can listen for. Something like this:

private void setState(PanZoomState state) {
    if (mState != state && state == PanZoomState.NOTHING) {
        broadcastGeckoEvent("PanZoom:Complete");
    }
    mState = state;
}

and then do the FormAssist:Update stuff in a PanZoom:Complete handler instead. I'm also not really sure what the form assistant does. Is this just to reposition the autocomplete dialog if we pan/zoom while it's visible?

Comment 27

[Wesley Johnston (:wesj)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #25)
> This is the only part of the patch that I'm worried about. Before we would
> always return true if we were near the max zoom and now we might return
> false in some of those cases, causing a regression in bug 783208 in some
> scenarios. As long as we don't regress on the actual case in bug 783208's
> comment #0 it's unlikely to be a big deal but still something worth checking.

I worried about that too, and tested the CNN testcase in bug 783208. That case at least works fine with this patch.

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Sounds good.

Comment 29

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #22)
> (In reply to Wesley Johnston (:wesj) from comment #20)
> > 
> > 3.) occasionally fields will zoom in before the keyboard appears and then be
> > underneath it. There's also still a bit to much jank when the keyboard comes
> > up (although it feels better to me now). Maybe we're better to move these
> > transitions to happen after the keyboard appears instead...
> 
> My bugzilla-fu is failing me, but I remember making this comment on some bug
> somewhere. So yes, I agree.

Oh, I found the comment I made: https://bugzilla.mozilla.org/show_bug.cgi?id=765473#c3

Comment 30

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2

How about something like this kats? This sends all state changes which seems like it might be too much info, but I don't really like the "hide the form assistant whenever a touch is on the screen" code either. Partially because there's no reason for it to be in LayerView, but mostly because it should only hide when we're in some sort of pan/zoom state (i.e. touching shouldn't matter), and it doesn't currently fix itself when we come out of those states. There's also this blip on TOUCH_END even if you weren't panning where we briefly jump into a BOUNCE state and then into NOTHING. I added some code to avoid that.

I'm adding margaret too, since she wrote a bunch of this form fill stuff. When we hide the form assistant popup, we also throw away our knowledge about what it was shown on, so instead I'm checking whatever the active form field is. It also moves the "click" handler to use the fallthrough, which does the exactly same thing and hides the form helper popup if there is nothing to do.

Comment 31

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 696372 [details] [diff] [review]
Patch 2/2

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

I like this much better. Just a few nits below.

::: mobile/android/base/ui/PanZoomController.java
@@ +143,1 @@
>          mState = state;

Might as well move the assignment statement into the conditional block.

::: mobile/android/chrome/content/browser.js
@@ +1001,5 @@
>      }
>  
>      if ((focused instanceof HTMLInputElement && focused.mozIsTextField(false))
> +        || (!aOnlyInputElements && focused instanceof HTMLTextAreaElement)
> +        || (!aOnlyInputElements && focused.isContentEditable)) {

This condition is hard to read. I'd prefer if you did something like this:

let focusable = (focused instanceof HTMLInputElement && focused.mozIsTextField(false));
if (!aOnlyInputElements) {
  focusable = focusable || (focused instanceof HTMLTextAreaElement) || focused.isContentEditable;
}
if (focusable) { ...

Well, maybe that's not much better. But pull something into a temp variable :)

Also, maybe rename aOnlyInputElements to aOnlyTextFields? To me "input element" always includes things like radio/checkboxes and textareas.

Comment 32

[Kartikaya Gupta (email:[e-mail])]

Also keep an eye on talos/perf tests for this patch. The message broadcast from PZC might get hit a lot and so might be expensive.

Comment 33

[:Margaret Leibovic]

Comment on attachment 696372 [details] [diff] [review]
Patch 2/2

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

It's been a long time since I've looked at this code, but I'm slowly remembering why we did things that might seem like mistakes. I think we should think about the interaction between the validation message and the autocomplete suggestions on inputs that might have both. Here's a page to test if you want: http://people.mozilla.com/~mleibovic/test/validation.html

::: mobile/android/base/GeckoApp.java
@@ +249,5 @@
>                      break;
>                  // Fall through...
>              case SELECTED:
>                  invalidateOptionsMenu();
> +                mFormAssistPopup.hide();

Could mFormAssistPopup be null here?

::: mobile/android/base/gfx/LayerView.java
@@ -135,5 @@
>  
> -        /** We need to manually hide FormAssistPopup because it is not a regular PopupWindow. */
> -        if (GeckoApp.mAppContext != null)
> -            GeckoApp.mAppContext.hideFormAssistPopup();
> -

If this is gone, what will take care of hiding the popup when the user touches outside of it? I see in browser.js you're hiding it if we're panning/zooming, but I think this hide call was here to handle hiding the popup if you tap outside of it.

::: mobile/android/chrome/content/browser.js
@@ +4719,5 @@
> +            break;
> +
> +          if (this._showValidationMessage(focused))
> +            break;
> +          this._showAutoCompleteSuggestions(focused);

This means you're always going to prefer showing the validation message over autocomplete suggestions when panning/zooming. There could be a bug here if there is a validation message, but if we show autocomplete suggestions because of the "input" event. When the user pans, we'd end up replacing the suggestions with the validation message.

@@ +4722,5 @@
> +            break;
> +          this._showAutoCompleteSuggestions(focused);
> +        } else {
> +          // temporarily hide the form assist popup while we're panning or zooming the page
> +          this._hideFormAssistPopup();

If you're guaranteed to be showing this again, you could change the logic to have some temporarily hidden type of state, so that you don't send "FormAssist:Hidden" from Java, which would mean you wouldn't get rid of _currentInputElement. But I suppose we don't keep track of the input element for form validation, so you'd need to add something to do that as well. Meh, probably too complicated to change all that.

@@ -4764,5 @@
> -        if (this._showValidationMessage(currentElement))
> -          break;
> -        this._showAutoCompleteSuggestions(currentElement);
> -        break;
> -

Getting rid of this changes the priority of showing the autocomplete suggestions vs. the validation message, such that we will always try to show suggestions first. I think you mistakenly thought these were the same.

Comment 34

[Wesley Johnston (:wesj)]

(In reply to Margaret Leibovic [:margaret] from comment #33)
> It's been a long time since I've looked at this code, but I'm slowly
> remembering why we did things that might seem like mistakes. I think we
> should think about the interaction between the validation message and the
> autocomplete suggestions on inputs that might have both. Here's a page to
> test if you want: http://people.mozilla.com/~mleibovic/test/validation.html

> If this is gone, what will take care of hiding the popup when the user
> touches outside of it? I see in browser.js you're hiding it if we're
> panning/zooming, but I think this hide call was here to handle hiding the
> popup if you tap outside of it.

The idea was to do this in the "click" handler. i.e. we won't hide the popup the instant your finger touches the screen, but we will as soon as you start panning or (if not panning) when a click happens that isn't targeted at the input. Now that I think about it though, pages that have touch event handlers might kill off the "click" event, so I'll try and find something better. Maybe touchstart is our best bet... Then again... pages with touch event handlers are intentionally killing the mousedown, so technically our focus won't change anyway...

> This means you're always going to prefer showing the validation message over
> autocomplete suggestions when panning/zooming.

Whoops. Nice catch. Thanks.

Comment 35

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2 - Fix form helper

So I've talked myself into this. With this we'll hide the form assist popup when

1.) We enter any sort of pan/zoom state
2.) We get a click event that isn't targeted at an element that supports the form assistant.

Since focus changes on mousedown, and we only fire mousedown when we're sending click events (or doing weird stuff to move the selection/cursor which should be going away), this seems fine to me.

Alternatively, maybe its safer to just move this back into java's touchdown. I'm just trying to avoid flickering it on and off when we don't want to (there's still some of that here as we cycle through states...)

Comment 36

[:Margaret Leibovic]

Comment on attachment 697231 [details] [diff] [review]
Patch 2/2 - Fix form helper

Thanks for addressing my concerns. This sounds reasonable to me, but I don't have an up-to-date build, so I haven't tested it myself.

Comment 37

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

Wes - Ready to land this now that we merged?

Comment 38

[Wesley Johnston (:wesj)]

Yep! Thanks for the reminder.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0af640499b0e
https://hg.mozilla.org/integration/mozilla-inbound/rev/bab8a97c0ee0

Comment 39

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bab8a97c0ee0
https://hg.mozilla.org/mozilla-central/rev/0af640499b0e

Comment 40

[Paul [pwd]]

While I've admittedly never been a fan of this feature, this patch breaks Google. Which is a pretty huge thing. Is there really a user demand for this feature? I've not seen reviews in the Play Store moaning that it doesn't exist.

For me personally, it's the cause of major frustration in using the browser. BTW I go to input data into a field, I'm already zoomed in at the level I desire.

Comment 41

[Aaron Train [:aaronmt]]

(In reply to Paul [sabret00the] from comment #40)
> While I've admittedly never been a fan of this feature, this patch breaks
> Google.

Can you detail the issue you're seeing and open a new bug?

Comment 42

[Cristian Nicolae (:xti)]

This issue is not reproducible on the latest Nightly. Closing bug as verified fixed on:

Firefox for Android
Version: 21.0a1 (2013-01-29)
Device: Galaxy R
OS: Android 2.3.4