608703 - Automatic adjustment of the view to keep in sync with the caret rect can be called when it shouldn't

Status: VERIFIED FIXED

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

Description

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

Attachment: Patch

Step to reproduce:
 * go to google.com
 * click on the main textbox
 * navigate with the form assistant

Actual result:
 * adjustement of the view for takingthe caret is called sometimes when it shouldn't

Expected result:
 * The caret adjustment is not called

The patch also include a small minimun zoom change to let some margin around the textbox field for google.com

Comment 1

[Matt Brubeck (:mbrubeck)]

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

>+++ b/chrome/content/AnimatedZoom.js

>+      event.initEvent("onAnimatedZoomBegin", true, true);
>+    event.initEvent("onAnimatedZoomEnd", true, true);

Nit: Can we change the names to "AnimatedZoomBegin" and "AnimatedZoomEnd"?

>+++ b/chrome/content/browser.js

>+const kBrowserFormZoomLevelMin = 0.8;

Is text in typical forms still readable at 80%?

r=mbrubeck with the nit fixed.

Comment 2

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

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

>   _zoom: function _formHelperZoom(aElementRect, aCaretRect) {

When did we add aCaretRect ?

>+      if (aCaretRect) {
>+        window.addEventListener("onAnimatedZoomEnd", function() {
>+          window.removeEventListener("onAnimatedZoomEnd", arguments.callee, true);
>+          this._zoom(null, aCaretRect);
>+        }, true);

I'm not the biggest fan of making one method (_zoom) do different things depending on the params passed in. Can't we make a new method? It looks like the code in the "if" block below is all you need in a new _ensureCaretVisible method, right?

>     // Move the view to show the caret if needed
>     if (aCaretRect) {
>       this._currentCaretRect = aCaretRect;
>       let caretRect = aCaretRect.scale(browser.scale, browser.scale);

>-
>+      zoomRect = new Rect(scroll.x, scroll.y, visibleRect.width, visibleRect.height);

Where is visibleRect coming from?

r- until we see if a new method is workable and show me visibleRect (I'm old and might have missed it)

Comment 3

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

(In reply to comment #2)
> Comment on attachment 487317 [details] [diff] [review]
> Patch
> 
> >   _zoom: function _formHelperZoom(aElementRect, aCaretRect) {
> 
> When did we add aCaretRect ?

Do you mean when do we add it in the code? or when do we use it?
For the first, this is probably in May before the 1.1 code freeze, for the later we're using aCaretRect as a parameter in the initial zoom and when there is a caret update in content.

> 
> >+      if (aCaretRect) {
> >+        window.addEventListener("onAnimatedZoomEnd", function() {
> >+          window.removeEventListener("onAnimatedZoomEnd", arguments.callee, true);
> >+          this._zoom(null, aCaretRect);
> >+        }, true);
> 
> I'm not the biggest fan of making one method (_zoom) do different things
> depending on the params passed in. Can't we make a new method? It looks like
> the code in the "if" block below is all you need in a new _ensureCaretVisible
> method, right?

Yep, you're right, let's cut this method in two parts.

> 
> >     // Move the view to show the caret if needed
> >     if (aCaretRect) {
> >       this._currentCaretRect = aCaretRect;
> >       let caretRect = aCaretRect.scale(browser.scale, browser.scale);
> 
> >-
> >+      zoomRect = new Rect(scroll.x, scroll.y, visibleRect.width, visibleRect.height);
> 
> Where is visibleRect coming from?

while making the patch I've first renamed zoomRect to visibleRect but I've removed it before asking a review to not confused the reviewers.

I'll addressed comments soon.

Comment 4

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

Attachment: Patch v0.2

Addressed Matt's and Finkle's comments.

And yes, the text is still readable with a zoom level of 0.8

Comment 5

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

Comment on attachment 487972 [details] [diff] [review]
Patch v0.2

Just to clarify. You call _ensureCaretVisible after all calls to zoom (I think). If so, you can do the setTimeout(_ensureCaretVisible(), 0) trick in zoom().

I just didn't want you to call zoom() twice, once to do the zoom and a second time to do the caret positioning. Making the caret positioning a separate function is good enough for me.

Comment 6

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

http://hg.mozilla.org/mobile-browser/rev/ae09920a6304

Comment 7

[Anna (Waverley)]

Since the form assistant feature was dropped off I will mark this bug as VERIFIED FIXED.

Build ID: Mozilla /5.0 (Android;Linux armv7l;rv:7.0a1) Gecko/20110608 Firefox/7.0a1 Fennec/7.0a1 

Dvice: HTC Desire Z (Android 2.2)