Closed Bug 539444 Opened 10 years ago Closed 10 years ago

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

Categories

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

x86
Linux
defect
Not set

Tracking

(fennec1.0+)

VERIFIED FIXED
Tracking Status
fennec 1.0+ ---

People

(Reporter: vingtetun, Assigned: vingtetun)

Details

Attachments

(1 file, 2 obsolete files)

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
I've tried to play with not reseting the zoom level but it performs badly on cnn.com and slashdot.
Attached patch Patch (obsolete) — Splinter Review
that's a followup of bug 537694.

This one prevent reseting the zoom level when a webpage is self refreshed or refreshed manually
Attachment #421455 - Flags: review?(webapps)
Attachment #421455 - Flags: review?(mark.finkle)
Attached patch Patch v0.2 (obsolete) — Splinter Review
Mark point me to a typo (a big one) on irc. Thanks
Attachment #421455 - Attachment is obsolete: true
Attachment #421456 - Flags: review?(webapps)
Attachment #421456 - Flags: review?(mark.finkle)
Attachment #421455 - Flags: review?(webapps)
Attachment #421455 - Flags: review?(mark.finkle)
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.
"resetZoomLevel" sounds good to me
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.
tracking-fennec: --- → 1.0+
(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 :)
(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)
Assignee: nobody → 21
Attachment #421456 - Attachment is obsolete: true
Attachment #421516 - Flags: review?(webapps)
Attachment #421516 - Flags: review?(mark.finkle)
Attachment #421456 - Flags: review?(webapps)
Attachment #421456 - Flags: review?(mark.finkle)
Attachment #421516 - Flags: review?(webapps) → review+
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.
Attachment #421516 - Flags: review?(mark.finkle) → review+
pushed to default:
http://hg.mozilla.org/mobile-browser/file/f89aafc2fa56
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Flags: in-litmus?
Added steps to testcase id 7583, https://litmus.mozilla.org/show_test.cgi?id=7583
Flags: in-litmus? → in-litmus+
Component: General → Panning/Zooming
You need to log in before you can comment on or make changes to this bug.