Closed Bug 611555 Opened 9 years ago Closed 9 years ago

should reflow on zoom

Categories

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

defect
Not set

Tracking

(fennec2.0b4+)

VERIFIED FIXED
Tracking Status
fennec 2.0b4+ ---

People

(Reporter: dietrich, Assigned: mbrubeck)

References

Details

(Whiteboard: [strings])

Attachments

(2 files)

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).
I haven't noticed a huge difference yet after installing that.

Regardless, the browser should be not-annoying out of the box :)
Either way, this is a dupe of bug 545703
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 545703
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.
Duplicate of bug: 578179
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...
Ah ok, comment #0 in bug 545703 made it sound like two distinct things.
(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.
Let's get Brad's add-on ported into the code
Status: RESOLVED → REOPENED
tracking-fennec: --- → 2.0b4+
Resolution: DUPLICATE → ---
Assignee: nobody → mbrubeck
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?
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.
Attachment #499918 - Flags: review?(mark.finkle)
OS: Linux → All
Hardware: x86 → All
Whiteboard: [strings]
Add a "Reformat text on zoom" setting to the "Content" section of the prefs.
Attachment #499923 - Flags: review?(mark.finkle)
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.
Attachment #499923 - Flags: review?(ben)
Attachment #499923 - Flags: review?(21)
Attachment #499918 - Flags: review?(ben)
Attachment #499918 - Flags: review?(21)
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.
Attachment #499918 - Flags: review?(mark.finkle)
Attachment #499918 - Flags: review?(ben)
Attachment #499918 - Flags: review?(21)
Attachment #499918 - Flags: review+
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.
Attachment #499923 - Flags: review?(mark.finkle)
Attachment #499923 - Flags: review?(ben)
Attachment #499923 - Flags: review?(21)
Attachment #499923 - Flags: review+
(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.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Depends on: 621683
Depends on: 621892
Depends on: 622087
Flags: in-litmus?
Depends on: 623313
Depends on: 627103
Depends on: 623831
Depends on: 623820
Depends on: 627814
See Also: → font-inflation
Depends on: 629038
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
It turns out this was disabled in bug 627814.
Depends on: 640217
Depends on: 641075
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
Status: RESOLVED → VERIFIED
Depends on: 642519
Depends on: 643087
Fennec 14.0 with NativeUI lacks "reflow on zoom" capabilities. It isn't even available as an option.
You need to log in before you can comment on or make changes to this bug.