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

VERIFIED FIXED

Status

VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: vingtetun, Assigned: vingtetun)

Tracking

Details

Attachments

(2 attachments)

Created attachment 487317 [details] [diff] [review]
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
Attachment #487317 - Flags: review?(mbrubeck)
Attachment #487317 - Flags: review?(mark.finkle)
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.
Attachment #487317 - Flags: review?(mbrubeck) → review+
Blocks: 605119
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)
Attachment #487317 - Flags: review?(mark.finkle) → review-
(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.
Created attachment 487972 [details] [diff] [review]
Patch v0.2

Addressed Matt's and Finkle's comments.

And yes, the text is still readable with a zoom level of 0.8
Assignee: nobody → 21
Attachment #487972 - Flags: review?(mark.finkle)
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.
Attachment #487972 - Flags: review?(mark.finkle) → review+
Summary: Automatic adjustment of the view to keep in sync with the caret rect can be called when it should'nt → Automatic adjustment of the view to keep in sync with the caret rect can be called when it shouldn't
http://hg.mozilla.org/mobile-browser/rev/ae09920a6304
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 7

8 years ago
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)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.