Closed Bug 530887 Opened 10 years ago Closed 10 years ago

Some web pages do not zoom to fit page on window resize

Categories

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

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: stechz, Assigned: stechz)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
STR on Desktop:
1. go to cnn.com
2. try to resize window
3. zoom level does not change with window resize

This is because suppress zoom flag is set to true after the page has been loaded (cnn.com goes back into a loading phase after everything has loaded), so all zoomToFits are ignored.  This patch removes the flag and uses onDefaultZoomChange to notify browser view when the default zoom level might have changed.  Now scroll area change handler deals with default zoom level changes instead of doing so awkwardly in tab loading code.
Attachment #414344 - Flags: review?(21)
Attachment #414344 - Flags: review?(mark.finkle)
Depends on: 530803
Attached patch Bit rotSplinter Review
Attachment #414344 - Attachment is obsolete: true
Attachment #414359 - Flags: review?(mark.finkle)
Attachment #414344 - Flags: review?(mark.finkle)
Attachment #414344 - Flags: review?(21)
Attachment #414359 - Flags: review?(21)
Blocks: 530803
No longer depends on: 530803
Comment on attachment 414359 [details] [diff] [review]
Bit rot


   /**
    * Swap out the current browser and browser viewport state with a new pair.
    */
-  setBrowser: function setBrowser(browser, browserViewportState, doZoom) {
+  setBrowser: function setBrowser(browser, browserViewportState) {
     if (browser && !browserViewportState) {
       throw "Cannot set non-null browser with null BrowserViewportState";
     }


browser.js#279 continue to call setBrowser with the third param
 
 
@@ -560,26 +557,46 @@ BrowserView.prototype = {

+  /** Call when default zoomToPage value may change. */
+  onDefaultZoomChange: function onDefaultZoomChange() {
+    let bvs = this._browserViewportState;
+    if (!bvs)
+      return false;
+
+    let isDefault = bvs.zoomLevel == bvs.defaultZoomLevel;

Could we add parenthesis here, otherwise it looks like an assignment.

r+ with nits addressed.

One less flag! woohoo
Attachment #414359 - Flags: review?(21) → review+
tracking-fennec: --- → ?
Comment on attachment 414359 [details] [diff] [review]
Bit rot

>+  /** Call when default zoomToPage value may change. */
>+  onDefaultZoomChange: function onDefaultZoomChange() {

Can we call this "updateDefaultZoom" or "checkDefaultZoom"? "on" prefix implies event to me. Yes "onAfterVisibleMove" makes me sad too :)


>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>+      bv.onDefaultZoomChange();

It would be nice if this call was internal to BrowserView only, but I can live with it for now. (External means that people need to be sure to call it at the right times - including add-on devs)

Yay! one less flag

r+ with name change
Attachment #414359 - Flags: review?(mark.finkle) → review+
The call needs to only be done by things that change the content size and things that change the window size (those are the two variables fit to page depends on).  Maybe bv could get its own resize handler sometime?

Pushed with nits. http://hg.mozilla.org/mobile-browser/rev/490d48e0c295
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(In reply to comment #4)

> depends on).  Maybe bv could get its own resize handler sometime?

I like that idea. No need to make a global handler do all the work.
Flags: in-testsuite?
Component: General → Panning/Zooming
Duplicate of this bug: 528139
bugspam
Assignee: nobody → ben
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.