User Agent: Mozilla/5.0 (Linux; U; Android 4.2.1; de-de; Nexus 4 Build/JOP40D) AppleWebKit/534.30 (KHTML, like Gecko) Version/4.0 Mobile Safari/534.30 Steps to reproduce: Just set the browser.viewport.defaultZoom pref to 1000 and load some pages as described in bug 666600. Enabled the option "Pinch to reflow text" in the settings. Actual results: Zoom level is changed but text is not reflowed. Expected results: The activated "Pinch to reflow text" option should also reflow the text when browser.viewport.defaultZoom is not -1 (default) because otherwise you still have to scroll to the left/right.

Scott, are you planning to work on this? From the discussion in bug 666600 what I wanted to do for this bug was: 1) Add a pref for "reflow text on page load" 2) If the pref is enabled, then set the max line box width on page load to be basically whatever is visible on the screen, taking into account the browser.viewport.defaultZoom pref. That way, if the browser.viewport.defaultZoom pref is set to load the page 1:1, and the new pref is also enabled, it will have the desired effect of rendering the page zoomed-in and already reflowed to prevent horizontal scrolling, at least as best we can.

Do you think you can add this feature for Firefox 20? Would be very nice!

I'm probably going to change the preference to be default 'true', but right now, it defaults to off (false).

Comment on attachment 706134 [details] [diff] [review] b830645 Review of attachment 706134 [details] [diff] [review]: ----------------------------------------------------------------- Looks good but I'd like to see an updated patch with the duplicated code refactored. ::: mobile/android/chrome/content/browser.js @@ +3598,5 @@ > + let docViewer = docShell.contentViewer.QueryInterface(Ci.nsIMarkupDocumentViewer); > + let viewportWidth = gScreenWidth / vp.zoom; > + // We add in a bit of fudge just so that the end characters don't accidentally > + // get clipped. 15px is an arbitrary choice. > + docViewer.changeMaxLineBoxWidth(viewportWidth - 15); Can you pull some of this out into a separate method that can be reused in the setViewport function? Looks like most of this code is duplicated from there. The helper function just needs to take the contentWin and the viewport.

Requesting review for new patch with refactor completed (minus the debugging statements, of course). The reason I left the debugging statements in the patch is because of an issue I've noticed on mobile fennec, which I think might be a bug in the defaultZoom calculation. Specifically, when I run fennec with this patch in place and the STR, it reflows-on-zoom as expected for the first page load. However, if I open a new tab, it resets the viewport to zoom=1. I think this is incorrect behavior. (BTW, if you want to try it kats, here are my STR:) 1) Push patch/recompile. 2) Open fennec, navigate to http://people.mozilla.org/~sjohnson/junkyard/lemis.html 3) Notice that reflow on zoom seems to work correctly with the default zoom. 4) Open a new tab, navigate to http://people.mozilla.org/~sjohnson/desperationSamba.html 5) Notice that, now, the reflow-on-zoom settings are incorrect. Here is the output I get from logcat using the filter "jwir3 (oLC)": 01-26 12:52:59.363: E/GeckoConsole(17764): ***** DEBUG_jwir3 (oLC): Location change called. 01-26 12:52:59.373: E/GeckoConsole(17764): ***** DEBUG_jwir3 (oLC): Performing reflow-on-zoom with viewport zoom: 1 01-26 12:52:59.853: E/GeckoConsole(17764): ***** DEBUG_jwir3 (oLC): Setting viewport with new zoom: 1.5 01-26 12:52:59.903: E/GeckoConsole(17764): ***** DEBUG_jwir3 (oLC): Location change called. 01-26 12:52:59.903: E/GeckoConsole(17764): ***** DEBUG_jwir3 (oLC): Performing reflow-on-zoom with viewport zoom: 1.5 01-26 12:53:06.219: E/GeckoConsole(17764): ***** DEBUG_jwir3 (oLC): Setting viewport with new zoom: 1.5 01-26 12:53:06.870: E/GeckoConsole(17764): ***** DEBUG_jwir3 (oLC): Location change called. 01-26 12:53:06.870: E/GeckoConsole(17764): ***** DEBUG_jwir3 (oLC): Performing reflow-on-zoom with viewport zoom: 1 This seems to indicate that the viewport zoom is getting reset back to 1 during a setViewport() call (which also performs reflow-on-zoom). I believe this re-setting of the zoom is erroneous. Thoughts?

Comment on attachment 706802 [details] [diff] [review] b830645 (v2) (WIP) Review of attachment 706802 [details] [diff] [review]: ----------------------------------------------------------------- The patch LGTM, with the indentation fixed. ::: mobile/android/chrome/content/browser.js @@ +2881,5 @@ > + let docViewer = docShell.contentViewer.QueryInterface(Ci.nsIMarkupDocumentViewer); > + let viewportWidth = gScreenWidth / aViewport.zoom; > + // We add in a bit of fudge just so that the end characters don't accidentally > + // get clipped. 15px is an arbitrary choice. > + docViewer.changeMaxLineBoxWidth(viewportWidth - 15); Indent function body with two spaces instead of 4, please. (I wish we used 4-space everywhere but alas)

(In reply to Scott Johnson (:jwir3) from comment #5) > 01-26 12:53:06.219: E/GeckoConsole(17764): ***** DEBUG_jwir3 (oLC): Setting > viewport with new zoom: 1.5 > 01-26 12:53:06.870: E/GeckoConsole(17764): ***** DEBUG_jwir3 (oLC): Location > change called. > 01-26 12:53:06.870: E/GeckoConsole(17764): ***** DEBUG_jwir3 (oLC): > Performing reflow-on-zoom with viewport zoom: 1 > > This seems to indicate that the viewport zoom is getting reset back to 1 > during a setViewport() call (which also performs reflow-on-zoom). I believe > this re-setting of the zoom is erroneous. Hm, interesting. But you have a logging line in setViewport, and that's not getting hit. So I don't think it's a call to setViewport that's doing this. I think what's happening here is that the new tab is created in browser.js, and the default zoom for the tab is 1.0. At the time the location change is called, the "correct" zoom value for the page is unknown, because we don't know how wide the page is because we haven't done a layout on it. So at that point the zoom value is whatever was left over from the last page, or 1.0 on a new tab. Actually now that I come to think of it, maybe changing the max line box width in the location change doesn't make sense because the viewport.zoom value is going to be bogus... we need to have already done a layout to know what the real zoom value will be.

This requires b803719, so it won't land until the patch for that one does.

Comment on attachment 707837 [details] [diff] [review] b830645 (v3) Review of attachment 707837 [details] [diff] [review]: ----------------------------------------------------------------- There's a bunch of junk in the patch but the actual code changes it contains look fine. ::: layout/base/nsPresShell.cpp @@ +9273,5 @@ > mMaxLineBoxWidth = aMaxLineBoxWidth; > > +#ifdef DEBUG_jwir3 > + printf_stderr("***** DEBUG_jwir3: Max line box width is now: %d for presShell: %p\n", mMaxLineBoxWidth, this); > +#endif Remove this ::: mobile/android/chrome/content/browser.js @@ +1177,5 @@ > break; > > case "Viewport:Change": > if (this.isBrowserContentDocumentDisplayed()) > + dump("***** DEBUG_jwir3 (oLC): Setting viewport from viewport change event."); Remove this @@ +2884,5 @@ > + let viewportWidth = gScreenWidth / aViewport.zoom; > + dump("***** DEBUG_jwir3: Refloz width: " + viewportWidth); > + // We add in a bit of fudge just so that the end characters don't accidentally > + // get clipped. 15px is an arbitrary choice. > + docViewer.changeMaxLineBoxWidth(viewportWidth - 15); Remove debug statements @@ +3210,5 @@ > setScrollPositionClampingScrollPortSize(scrollPortWidth, scrollPortHeight); > }, > > setViewport: function(aViewport) { > + dump("***** DEBUG_jwir3 (oLC): Setting viewport with new zoom: " + aViewport.zoom); Remove @@ +3224,5 @@ > if (isZooming && > BrowserEventHandler.mReflozPref && > BrowserApp.selectedTab._mReflozPoint) { > + dump("***** DEBUG_jwir3 (oLC): Performing new reflow on zoom in setViewport with zoom: " + aViewport.zoom); > + BrowserApp.selectedTab.performReflowOnZoom(aViewport); Remove debug statement @@ +3331,5 @@ > case "DOMContentLoaded": { > let target = aEvent.originalTarget; > > + dump("***** DEBUG_jwir3 (oLC): DOM content has been loaded."); > + Remove @@ +3901,5 @@ > + let defaultZoom = Services.prefs.getIntPref("browser.viewport.defaultZoom") / 1000.0; > + dump("***** DEBUG_jwir3 (oLC): before-first-paint with actual zoom: " + vp.zoom); > + let documentURI = this.browser.contentWindow.document.documentURIObject.spec > + dump("***** DEBUG_jwir3 (oLC): bfp document URI: " + documentURI); > + Remove this entire block, except move the rzEnabled and rzP1 variables down to the next hunk. @@ +3940,5 @@ > + // Retrieve the viewport width and adjust the max line box width > + // accordingly. > + dump("***** DEBUG_jwir3 (oLC): Should do refloz with viewport zoom: " + vp.zoom); > + let contentWin = this.browser.contentWindow; > + BrowserApp.selectedTab.performReflowOnZoom(vp); Remove debug statement and unused variable. Define vp down here inside the if block rather than way up there in the previous hunk. @@ +3991,5 @@ > this.updateReflozPref(); > }, > > resetMaxLineBoxWidth: function() { > + dump("***** DEBUG_jwir3 (oLC): Resetting max line box width."); Remove @@ +4953,5 @@ > // this should never happen > Cu.reportError("Warning: selected tab changed during find!"); > // fall through and restore viewport on the initial tab anyway > } > + dump("***** DEBUG_jwir3 (oLC): Setting viewport from handleResult."); Remove

Here's the updated patch with debug statements removed and review comments addressed. Sorry for the previous patch, kats, I accidentally posted the one from the wrong tree. ;)

Comment on attachment 708297 [details] [diff] [review] b830645 (v4) Review of attachment 708297 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks! ::: mobile/android/chrome/content/browser.js @@ +3929,5 @@ > + if (rzEnabled && rzPl) { > + // Retrieve the viewport width and adjust the max line box width > + // accordingly. > + let vp = BrowserApp.selectedTab.getViewport(); > + let contentWin = this.browser.contentWindow; don't need contentWin here, it's not used.

Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/b536eee06d66

Great news and thanks for that! Would it be possible to include it in Firefox 20 already?

(In reply to Frederik Niedernolte from comment #14) > Great news and thanks for that! > Would it be possible to include it in Firefox 20 already? Frederik: It's entirely possible this will get pushed to FF20 (currently Aurora), but I don't know for certain.