611555 - should reflow on zoom

Status: VERIFIED FIXED

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

Description

[Dietrich Ayala (:dietrich)]

One major papercut for me using Fennec as default browser is that after zooming, page content is mostly offscreen.

The stock browser reflows when you zoom past a certain degree (from what I can tell).

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

there's an add-on for that https://addons.mozilla.org/en-US/mobile/addon/157099/

Comment 2

[Dietrich Ayala (:dietrich)]

I haven't noticed a huge difference yet after installing that.

Regardless, the browser should be not-annoying out of the box :)

Comment 3

[Aakash Desai [:aakashd]]

Either way, this is a dupe of bug 545703

Comment 4

[Dietrich Ayala (:dietrich)]

This bug is not about text zoom. It's about reflow. The page *width* post-zoom, is sub-optimal for comfortable reading on a small screen without having to scroll horizontally *per line of text read*.

So actually a dupe of bug 578179.

Comment 5

[Aakash Desai [:aakashd]]

People use different terminology for the same concept. There's no difference between them all. mbrubeck says 578179 is the main dev bug, but there's others (including the one originally duped) which are talking about the same issue. Triaging...

Comment 6

[Dietrich Ayala (:dietrich)]

Ah ok, comment #0 in bug 545703 made it sound like two distinct things.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #2)
> I haven't noticed a huge difference yet after installing that.
> 
> Regardless, the browser should be not-annoying out of the box :)

we're thinking about taking that into the product, which is why it would be useful to have people testing it.

It does a "reflow zoom"/"android style zoom" when you double tap to zoom. And it maintains our current "iphone style" zoom when you pinch to zoom.

Comment 8

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

Let's get Brad's add-on ported into the code

Comment 9

[Madhava Enros [:madhava]]

Perhaps a pref for this, defaulting to Yes (on) (on Android, anyway).

Wrap text on zoom      [ yes | no ]

("Reflow on zoom" seems to jargony to me)


In the "Content" section.


Do people think we _don't_ want a pref for this?

Comment 10

[Matt Brubeck (:mbrubeck)]

Attachment: 1. Use code from Easy Reading

This is a mostly direct copy of the code from blassey's Easy Reading add-on.  The next patch will add a preference to control the new behavior.

Note: I moved getScaleRatio to Browser.prototype just because it belongs there more than Tab.prototype.  There's no change to the function itself.

Comment 11

[Matt Brubeck (:mbrubeck)]

Attachment: 2. Add preference

Add a "Reformat text on zoom" setting to the "Content" section of the prefs.

Comment 12

[Matt Brubeck (:mbrubeck)]

Comment on attachment 499923 [details] [diff] [review]
2. Add preference

Not sure who's around this week, so I'll take a review from whomever I can.

Comment 13

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

Comment on attachment 499918 [details] [diff] [review]
1. Use code from Easy Reading

>diff -r 636ba1ae744e chrome/content/browser.js

>-    let zoomTolerance = (this.selectedTab.isDefaultZoomLevel()) ? .9 : .6666;
>+    // Don't zoom in a marginal amount.
>+    let zoomTolerance = .95;

What's the impact of moving to a fixed tolerance?

>       case "Browser:ZoomToPoint:Return":
>         // JSON-ified rect needs to be recreated so the methods exist
>         let rect = Rect.fromRect(json.rect);
>-        if (!this.zoomToPoint(json.x, json.y, rect))
>+        if (!this.zoomToPoint(json.x, json.y, rect)) {
>+          browser.messageManager.sendAsyncMessage("Browser:ResetZoom", {});

Message ping-pong is a pain, but I guess we can't just always call resetZoom in the content side.

>   tapDouble: function tapDouble(aX, aY, aModifiers) {
>     this._clearPendingMessages();
>-    this._dispatchMouseEvent("Browser:ZoomToPoint", aX, aY);
>+
>+    let tab = Browser.selectedTab;
>+    if (!tab.allowZoom)
>+      return;

This would be handled eventually in the zoomFromPoint / zoomToPoint calls in "Browser:ZoomToPoint:Return", but I guess you're saving some work by doing it early.

>+
>+    let browser = tab.browser;
>+    let p = browser.transformClientToBrowser(aX, aY);
>+    let json = { x: p.x, y: p.y, width: window.innerWidth / Browser.getScaleRatio() };
>+    browser.messageManager.sendAsyncMessage("Browser:ZoomToPoint", json);

I'm sad to not use dispatchMouseEvent and have to recreate the code, but I guess it would be worse to make it too "flexible/confusing". Maybe we could pass an aOptions object into the method and copy any properties from that object, if it exists. We could make "modifiers" and "width" work using it. Thoughts?

>diff -r 636ba1ae744e chrome/content/content.js
>+      case "pagehide":
>+        if (aEvent.target == content.document)
>+          this._resetZoom("");

No reason for the "" that I can see. Remove it

>           rect = getBoundingContentRect(element);
>+          let viewer = docShell.contentViewer.QueryInterface(Components.interfaces.nsIMarkupDocumentViewer);

Components.interfaces -> Ci

>+  _resetZoom: function _resetZoom() {
>+    let viewer = docShell.contentViewer.QueryInterface(Components.interfaces.nsIMarkupDocumentViewer);
>+    viewer.textZoom = 1;

We could call this method _setTextZoom(aZoom) and then have "pagehide" and "Browser:ResetZoom" just call this._setTextZoom(1) and "Browser:ZoomToPoint" can call it with the text zoom value. Thoughts?

Also, Components.interfaces -> Ci

r+ with nits. you can think about the other cleanup issues or we can worry about them later.

Comment 14

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

Comment on attachment 499923 [details] [diff] [review]
2. Add preference

>diff -r 3b1fca7a4907 app/mobile.js

>+// Change text wrapping based on the screen size.
>+pref("browser.doubleTap.reflow", true);

I like "browser.ui.zoom.reflow", like we use here:
http://mxr.mozilla.org/mobile-browser/source/app/mobile.js#376

I think it makes sense to keep this pref with those

>diff -r 3b1fca7a4907 locales/en-US/chrome/preferences.dtd

>+<!ENTITY reflowZoom.title                          "Reformat text on zoom">

I wonder if the fact this only happens on double tap should be mentioned. Let's get Madhava to help us out with the text.

r+ with name fixed. We can land a string change later, when we get feedback from madhava.

Comment 15

[Matt Brubeck (:mbrubeck)]

(In reply to comment #13)
> >+    // Don't zoom in a marginal amount.
> >+    let zoomTolerance = .95;
> 
> What's the impact of moving to a fixed tolerance?

This will increase the number of cases where we zoom to the element width.  We used to avoid this when the change in zoom was small, because it was not very useful to do.  But now it can be useful even without much zoom, because it will cause the text to reflow.

> I'm sad to not use dispatchMouseEvent and have to recreate the code, but I
> guess it would be worse to make it too "flexible/confusing". Maybe we could
> pass an aOptions object into the method and copy any properties from that
> object, if it exists. We could make "modifiers" and "width" work using it.
> Thoughts?

Sounds good. Done.

> >+          this._resetZoom("");
> 
> No reason for the "" that I can see. Remove it

Done.

> Components.interfaces -> Ci

Done.

> >+  _resetZoom: function _resetZoom() {
>
> We could call this method _setTextZoom(aZoom) and then have "pagehide" and
> "Browser:ResetZoom" just call this._setTextZoom(1) and "Browser:ZoomToPoint"
> can call it with the text zoom value. Thoughts?

Done.

(In reply to comment #14)
> I like "browser.ui.zoom.reflow", like we use here:
> http://mxr.mozilla.org/mobile-browser/source/app/mobile.js#376
> 
> I think it makes sense to keep this pref with those

Done.

Pushed:
http://hg.mozilla.org/mobile-browser/rev/323dfda29f00
http://hg.mozilla.org/mobile-browser/rev/385846645ac3

I'll be filing some followup bugs for known issues with reflow not working well on certain sites.

Comment 16

[Martijn Wargers (dead)]

This isn't enabled by default. Wasn't the idea that this was turned on by default?
http://mxr.mozilla.org/mobile-browser/source/app/mobile.js#401
401 pref("browser.ui.zoom.reflow", false); // Change text wrapping on double-tap

Comment 17

[Martijn Wargers (dead)]

It turns out this was disabled in bug 627814.

Comment 18

[Andreea Pod]

VERIFIED FIXED on:
Build ID: Mozilla /5.0 (Android;Linux armv7l;rv:2.0b13pre) Gecko/20110315
Firefox/4.0b13pre Fennec /4.0b6pre
Device: HTC Desire

Comment 19

[ekerazha]

Fennec 14.0 with NativeUI lacks "reflow on zoom" capabilities. It isn't even available as an option.