539444 - [Regression] Refreshing google.com when zoomed and scrolled to the right, messed up the rendering

Status: VERIFIED FIXED

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

Description

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

That's a regression from bug 537694. Because when we refresh a page we don't keep its current zoom level the content scroll position are out of view.

Step to repro:
 * launch fennec
 * go to google.com
 * dblclick on the logo
 * pan to the right until you see the right sidebar
 * click on refresh

Actual result:
 * The view is grayed out

Expected result:
 * the usual google.com

Comment 1

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

I've tried to play with not reseting the zoom level but it performs badly on cnn.com and slashdot.

Comment 2

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

Attachment: Patch

that's a followup of bug 537694.

This one prevent reseting the zoom level when a webpage is self refreshed or refreshed manually

Comment 3

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

Attachment: Patch v0.2

Mark point me to a typo (a big one) on irc. Thanks

Comment 4

[Benjamin Stover (:stechz)]

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

>     if (location != this.browser.lastSpec) {
>       this.browser.lastSpec = this.browser.currentURI.spec;
>       Browser.removeTransientNotificationsForTab(this._tab);
>+      this._tab.updateDefaultZoomLevel();

Why is this moving to onLocationChange?  Does startLoading not get called on refresh?

>-    let bvs = this._browserViewportState;
>-    bvs.defaultZoomLevel = bvs.zoomLevel; // ensures zoom level is reset on new pages
>+  updateDefaultZoomLevel: function resetZoomLevel() {
>+    let bvs = this._browserViewportState;
>+    bvs.defaultZoomLevel = bvs.zoomLevel;
>+  },
>+

Non-matching function names (update/resetZoomLevel) here.  I don't like updateDefaultZoomLevel because of how similar it sounds to the function is browser view.

The function name needs to say "the next time the default zoom level changes, update to that no matter what."  "resetZoomLevel" or "resetZoomLevelToDefault" both do that IMO.  Maybe Mark has a good suggestion.

Comment 5

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

"resetZoomLevel" sounds good to me

Comment 6

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

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


>+  updateDefaultZoomLevel: function resetZoomLevel() {
>+    let bvs = this._browserViewportState;
>+    bvs.defaultZoomLevel = bvs.zoomLevel;
>+  },
>+

"resetZoomLevel" and add a comment so we know what affect this code has on BrowserView and zoom level.

Also, do we have some example pages where this happens? I want to instrument the code aa little and see what codepaths are getting fired.

Comment 7

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

(In reply to comment #4)
> (From update of attachment 421456 [details] [diff] [review])
> >     if (location != this.browser.lastSpec) {
> >       this.browser.lastSpec = this.browser.currentURI.spec;
> >       Browser.removeTransientNotificationsForTab(this._tab);
> >+      this._tab.updateDefaultZoomLevel();
> 
> Why is this moving to onLocationChange?  Does startLoading not get called on
> refresh?

It is but here it means that we don't reset the zoom level when we are refreshing the same page / or when the same page refresh itself via a meta tag as example
> 
> >-    let bvs = this._browserViewportState;
> >-    bvs.defaultZoomLevel = bvs.zoomLevel; // ensures zoom level is reset on new pages
> >+  updateDefaultZoomLevel: function resetZoomLevel() {
> >+    let bvs = this._browserViewportState;
> >+    bvs.defaultZoomLevel = bvs.zoomLevel;
> >+  },
> >+
> 
> Non-matching function names (update/resetZoomLevel) here.  I don't like
> updateDefaultZoomLevel because of how similar it sounds to the function is
> browser view.
> 
> The function name needs to say "the next time the default zoom level changes,
> update to that no matter what."  "resetZoomLevel" or "resetZoomLevelToDefault"
> both do that IMO.  Maybe Mark has a good suggestion.

right.
resetZoomLevel was my first try as you can see :)

Comment 8

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

(In reply to comment #6)
> (From update of attachment 421456 [details] [diff] [review])
> 
> >+  updateDefaultZoomLevel: function resetZoomLevel() {
> >+    let bvs = this._browserViewportState;
> >+    bvs.defaultZoomLevel = bvs.zoomLevel;
> >+  },
> >+
> 
> "resetZoomLevel" and add a comment so we know what affect this code has on
> BrowserView and zoom level.
> 
> Also, do we have some example pages where this happens? I want to instrument
> the code aa little and see what codepaths are getting fired.

Sure, if you want this function not to be call just go to google.com / zoom on the logo / pan anywhere / hit F5

If you want it to fired, just do the same step but instead of hitting F5, try to go to another url and observe that the zoom level changed (you can even do that by going back/forward in yout history with a page that was already scrolled)

Comment 9

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

Attachment: Patch v0.3 - address comments

Comment 10

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

Comment on attachment 421516 [details] [diff] [review]
Patch v0.3 - address comments

It makes sense to put resetZoomLevel on BrowserViewState and not Tab. But we can file a followup bug to clean up how we use BrowserViewState.

Comment 11

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

pushed to default:
http://hg.mozilla.org/mobile-browser/file/f89aafc2fa56

Comment 12

[Aakash Desai [:aakashd]]

verified FIXED on build:
Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2pre) Gecko/20100114 Firefox/3.6pre Fennec/1.1a1pre

Based on the reporter's comments and comment #1, we should add this testcase in our Zooming BFT.

Comment 13

[LonelyBob]

Added steps to testcase id 7583, https://litmus.mozilla.org/show_test.cgi?id=7583