518642 - zoom level is reset when softkb is displayed

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

on the n810 with the 20090924 1.9.2 nightly build, the softkb pops up and resizes the window.  This causes the zoom level to be reset.

Comment 1

[Aakash Desai [:aakashd]]

This is bigger than a normal bug and I'm setting the blocking-fennec? flag as a result.

Comment 2

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

Any window resizes are causing the zoom to reset. Fullscreen to normal for example.

We have an unprotected call to zoomToPage here:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser.js#416

BTW, the call to hideSidebars below it is disconcerting too

Comment 3

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

Attachment: patch

This patch fixes the bug and does some minor cleanup:
* zoomChanged was not being set on the right object: "this" -> "bvs"
* zoomChanged was not being checked in our resize code before a call to zoomToPage
* some useless whitespace was removed
* an unused & broken "clone" method was removed

One assumption: In the resize handler we call hideSidebars() and I assume this should only ever happen if we zoomToPage. So I added the perhaps redundant call to bv.onAfterVisibleMove just to make sure it was called.

Comment 4

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

Attachment: patch 2

updated to trunk

Comment 5

[Benjamin Stover (:stechz)]

Comment on attachment 404646 [details] [diff] [review]
patch 2

>+      // Assume this was a user action and change the flag. Callers should reset
>+      // the flag if this was not in response to a user action
>+      bvs.zoomChanged = true;

Good catch.  Your comment makes me think this is a bad assumption.  zoomToPage also calls this function and is not a user-willed action: so if _resizeAndPaint is called more than once, it will not zoomToPage the second time.  IMHO we shouldn't use fit to zoom on load anyways (bug 520593), but if we do not end up doing that we should probably rethink where the zoomChanged flag is set.

>+    // zoomToPage is a non-user action, so reset the zoomChanged flag
>+    let bvs = this._browserViewportState;
>+    if (bvs)
>+      bvs.zoomChanged = false;

Ah, I see you've already figured it out.  This is a hacky solution though: ideally this flag should only be set when the user changed the zoom level.

>+      // We do not auto-zoom if the user has manually changed the zoom level
>+      if (!Browser.selectedTab.browserViewportState.zoomChanged) {
>+        bv.zoomToPage();
>+        Browser.hideSidebars();
>+      }
>+      
>+      bv.onAfterVisibleMove();
>       bv.commitBatchOperation();
>     }

The hideSidebars alarms me too--this should only be done when the window first comes up.  Is there a better place for hideSidebars?

>     bv.beginBatchOperation();
>-

I actually like begins and ends to have whitespace around them so that it is easy to tell they are matched.

Comment 6

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

Attachment: Patch v0.3

* adresses comments except the hideSidebars part

Comment 7

[Benjamin Stover (:stechz)]

Comment on attachment 413658 [details] [diff] [review]
Patch v0.3

>diff -r f305d4de5ece chrome/content/BrowserView.js
>-      bvs.zoomChanged = true;

Hmm.  I have two problems with putting this code into setVisibleRect:
1) setVisibleRect is changing the browser viewport state directly.
2) This misses incremental zoom (ctrl-up and ctrl-down).

I'm starting to feel like zoomChanged is a bad, bad flag that does not allow us to write good code.  Let's just get it functionally correct and perhaps refactor the flag away later.  Please fix #2 however you see fit and I'll r+.

>-      bv.zoomToPage();
>-      Browser.hideSidebars();
>+      if (!Browser.selectedTab.browserViewportState.zoomChanged) {
>+        bv.zoomToPage();
>+        // hidesidebars calls bv.onAfterVisibleMove();
>+        Browser.hideSidebars();
>+      }

As the comment says, hideSidebars calls onAfterVisibleMove, so there's a chance this path will not call onAfterVisibleMove even though it should.  We should probably just strike the comment and always call onAfterVisibleMove instead of being clever.

Comment 8

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

Attachment: Patch v0.4

* Adress comments

And because you ask I've punished the zoomChanged flag : enjoy :)

Comment 9

[Benjamin Stover (:stechz)]

Comment on attachment 414267 [details] [diff] [review]
Patch v0.4

I love it.  Excellent work Vivian!  I have a couple of suggestions but this is definitely the right idea.

>   handleEventImpl: function handleEventImpl(zoomlevel) {
>-	this.pendingEvent = 0;
>-	Browser.zoom(zoomlevel);
>+	  this.pendingEvent = 0;
>+	  Browser.zoom(zoomlevel);

Uh oh, I see a tab!

>+      let bvs = Browser.selectedTab.browserViewportState;
>+      if (bvs.zoomLevel == bvs.defaultZoomLevel) {
>+        bv.zoomToPage();
>+        Browser.hideSidebars();
>+        bv.onAfterVisibleMove();
>+      }

How about a function for this?  Mark is the function name master, but I think bv.updateDefaultZoomLevel() or bv.onDefaultZoomLevelChange() might be good suggestions.  The function would change the default zoom level and zoomToPage() if ZL was at the default previously.  The function could return true if there was a change so that you can call Browser.hideSidebars() here (if that's really what we want to do).

I believe you should take bv.onAfterVisibleMove() outside the if, because things outside the if could have moved things around as well.

>     bv.commitBatchOperation();
>     bv.commitOffscreenOperation();
>+
>   },

Nit.  Why the extra space?

>-      if (!this._browserViewportState.zoomChanged && !restoringPage) {
>+      if (this._browserViewportState.zoomLevel == this._browserViewportState.defaultZoomLevel && !restoringPage) {
>         // Only fit page if user hasn't started zooming around and this is a page that
>         // isn't being restored.
>         bv.zoomToPage();
>-        
>-        // zoomChanged gets set to true, but user did not change zooming
>-        this._browserViewportState.zoomChanged = false;
>       }

Just an idea: if we have the above on default zoom change, the size change handler on browser view could call it and we'd no longer have to do this code at all!  And we could get rid of suppressZoom flag!

>   startLoading: function() {
>     this._loading = true;
>-    this._browserViewportState.zoomChanged = false;
>+    this._browserViewportState.defaultZoomLevel = this._browserViewportState.zoomLevel;

Why is this here exactly?  I'm sure there's a reason, I just don't know it.

r+'ing with nits because this is fine as is, and we can always do another patch if you agree with me on the bigger stuff.

Comment 10

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

I agree with you on bigger stuff :)

I'll be happy to land this asap as possible since it blocks one of my blocker and open a followup for refactoring/enhancing our code.

Comment 11

[Benjamin Stover (:stechz)]

Pushed: http://hg.mozilla.org/mobile-browser/rev/fc179b035b60

Comment 12

[Benjamin Stover (:stechz)]

(with nits)

Comment 13

[Aakash Desai [:aakashd]]

verified FIXED on builds:
Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b4pre) Gecko/20091125 Firefox/3.6b4pre Fennec/1.0b6pre

and

Mozilla/5.0 (X11; U; Linux armv6l; Nokia N8xx; en-US; rv:1.9.3a1pre) Gecko/20091125 Firefox/3.7a1pre Fennec/1.0b5

Comment 14

[Al Billings [:abillings - ex-MoCo]]

in+litmus+

https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=9765