Refine autozoom scaling constants and logic

Refine autozoom scaling constants and logic

Reduce the legible caret height and increase the scaling tolerance, making
tap-triggering autozooms less intrusive.  Also add some logic to prevent
auto-zooming if a user is sufficiently zoomed in within the corresponding
double-tap block region (e.g., if a user double-taps a textbox, then taps
the same textbox, we should try hard not to zoom in yet again).

BUG=345926
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186037

[jdduke (slow), 2014-03-13 19:21:36]

aelias@: PTAL.  I'm not 100% sold on the block scale logic added here, so I'd be
fine removing it.

This also addresses a bug where we were "over"-animating: the code determining
|needAnimation| was using the view coords at the *new* scale to check if the
caret/textbox were off-screen, when it should only have been using that scale if
it was sufficient to satisfy minScaleChangeToTriggerZoom.

[jdduke (slow), 2014-03-13 19:23:07]

On 2014/03/13 19:21:36, jdduke wrote:
> aelias@: PTAL.  I'm not 100% sold on the block scale logic added here, so I'd
be
> fine removing it.
> 
> This also addresses a bug where we were "over"-animating: the code determining
> |needAnimation| was using the view coords at the *new* scale to check if the
> caret/textbox were off-screen, when it should only have been using that scale
if
> it was sufficient to satisfy minScaleChangeToTriggerZoom.

There's also a corresponding Chromium patch,
https://codereview.chromium.org/197873002/, to remove resetting scale/scroll on
back press, as well as avoiding repeated autozoom commands for an already
selected editable node.

[aelias_OOO_until_Jul13, 2014-03-14 07:16:10]

Yeah, I dunno about that block scale logic either :).  Could you explain the
principle you had in mind for it?  Is the idea that the block might contain a
whole bunch of other form fields so you want to take that context into account
somehow?  I'm not convinced that helps without empirical examples of form pages.
 And even if it does maybe we would get a similar benefit just from increasing
the value of minScaleChangeToTriggerZoom.

https://codereview.chromium.org/196133011/diff/20001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/196133011/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2541:
computeScaleAndScrollForBlockRect(textboxRect.location(), blockBounds,
touchPointPadding, minimumPageScaleFactor() * doubleTapZoomAlreadyLegibleRatio,
blockScale, blockScroll);
Hmm, this invokes more code than is necessary, particularly the computation of
scroll position which you then ignore.  And I don't like introducing a
dependency on double-tap zoom code because it makes it hard to change double-tap
zoom behavior without unexpected fallout on textbox behavior (or vice versa). 
If you want to keep this, could you rewrite it from first principles instead
(maybe start by copy-pasting the scale-computation code you're relying on and
then trim down any unnecessary parts?).

[jdduke (slow), 2014-03-14 14:30:39]

On 2014/03/14 07:16:10, aelias wrote:
> Yeah, I dunno about that block scale logic either :).  Could you explain the
> principle you had in mind for it?  Is the idea that the block might contain a
> whole bunch of other form fields so you want to take that context into account
> somehow?  I'm not convinced that helps without empirical examples of form
pages.
>  And even if it does maybe we would get a similar benefit just from increasing
> the value of minScaleChangeToTriggerZoom.
> 
>
https://codereview.chromium.org/196133011/diff/20001/Source/web/WebViewImpl.cpp
> File Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/196133011/diff/20001/Source/web/WebViewImpl.c...
> Source/web/WebViewImpl.cpp:2541:
> computeScaleAndScrollForBlockRect(textboxRect.location(), blockBounds,
> touchPointPadding, minimumPageScaleFactor() *
doubleTapZoomAlreadyLegibleRatio,
> blockScale, blockScroll);
> Hmm, this invokes more code than is necessary, particularly the computation of
> scroll position which you then ignore.  And I don't like introducing a
> dependency on double-tap zoom code because it makes it hard to change
double-tap
> zoom behavior without unexpected fallout on textbox behavior (or vice versa). 
> If you want to keep this, could you rewrite it from first principles instead
> (maybe start by copy-pasting the scale-computation code you're relying on and
> then trim down any unnecessary parts?).

In general, I agree that this is probably over-complicated and unnecessary :). 
The original idea was that, if a user double-taps a particular form element,
then taps a particular field, they'd (ideally) already be at a comfortable zoom
level and we wouldn't want to trigger another zoom.

However, that does little good if the field is still "illegible" in the absolute
case, so I'm inclined to drop it entirely and slightly increase the zoom
tolerance.  I think a better solution might be loosening the tolerance for text
areas with multiple lines.  In practice, I've found that the biggest problem
with decreasing the legible caret height / increasing
minScaleChangeToTriggerZoom is that it becomes increasingly difficult to tap
single-line input elements without triggering tap disambiguation.  Text areas
shouldn't suffer from this problem, so it might be okay to relax the tolerance
there.  I'll play with that for a bit.

[jdduke (slow), 2014-03-19 16:54:33]

I've simplified the logic a bit, though I still make a small distinction between
textboxes and areas (is there a better way to determine input line count other
than the textbox height?).

[aelias_OOO_until_Jul13, 2014-03-19 22:54:19]

lgtm.  There's likely some way to fish out line count from WebCore but I don't
know of any problem with this way of inferring it.

[jdduke (slow), 2014-03-21 10:31:17]

On 2014/03/19 22:54:19, aelias wrote:
> lgtm.  There's likely some way to fish out line count from WebCore but I don't
> know of any problem with this way of inferring it.

I was playing with Safari, which greedily centers the input field, and was
wondering if there may be some more middle ground here.

At the moment, we'll only re-position the editable node if 1) the optimal scale
is sufficiently different, 2) the caret is off screen, or 3) the node can be
made fully on-screen.

What if, in addition to having a scale threshold, we have an optimal position
threshold, i.e., if the optimal position is sufficiently far from the current
position, we shift (only the offset, not the scale if we're already close to the
right scale).  This can be quite convenient if you have, say, a series of
vertically aligned fields, you progress down them, and you reach one towards the
bottom, or top, at which point we'd trigger the recentering.   Just an idea, I'm
fine with the current approach but this alternative seems like an interesting
compromise. wdyt?

[aelias_OOO_until_Jul13, 2014-03-21 19:25:38]

That makes sense.  We should just be careful that that doesn't do anything
strange with large fields though.  I suggest landing this first and working on
that separately.

[jdduke (slow), 2014-03-21 20:07:01]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-03-21 20:07:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdduke@chromium.org/196133011/60001

[commit-bot: I haz the power, 2014-03-21 20:35:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-21 20:35:22]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[jdduke (slow), 2014-03-27 20:34:28]

On 2014/03/21 20:35:22, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   tryserver.blink on mac_blink_rel

Hmm, aelias@, can you think of any (obvious?) reasons why this specific unit
test would fail on win/mac but not linux/android?

[jdduke (slow), 2014-03-27 20:41:56]

On 2014/03/27 20:34:28, jdduke wrote:
> On 2014/03/21 20:35:22, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   tryserver.blink on mac_blink_rel
> 
> Hmm, aelias@, can you think of any (obvious?) reasons why this specific unit
> test would fail on win/mac but not linux/android?

I wonder if the text sizing is different such that the cursor placed at the end
of the EditBoxWithText makes a difference...

[aelias_OOO_until_Jul13, 2014-03-27 20:54:26]

Your text size theory is plausible.  Other platform differences I've seen in the
past are non-overlay scrollbars getting created on only some platforms and
changing the window size there, and uninitialized variables tending to get
particular values on different platforms.

[jdduke (slow), 2014-11-24 23:27:04]

Patchset #12 (id:220001) has been deleted

[jdduke (slow), 2014-11-26 16:16:14]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-11-26 16:16:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/196133011/320001

[commit-bot: I haz the power, 2014-11-26 16:39:26]

Committed patchset #16 (id:320001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186037