Closed
Bug 611555
Opened 14 years ago
Closed 14 years ago
should reflow on zoom
Categories
(Firefox for Android Graveyard :: Panning/Zooming, defect)
Firefox for Android Graveyard
Panning/Zooming
Tracking
(fennec2.0b4+)
VERIFIED
FIXED
Tracking | Status | |
---|---|---|
fennec | 2.0b4+ | --- |
People
(Reporter: dietrich, Assigned: mbrubeck)
References
Details
(Whiteboard: [strings])
Attachments
(2 files)
11.30 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
6.30 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
there's an add-on for that https://addons.mozilla.org/en-US/mobile/addon/157099/
Reporter | ||
Comment 2•14 years ago
|
||
I haven't noticed a huge difference yet after installing that.
Regardless, the browser should be not-annoying out of the box :)
Comment 3•14 years ago
|
||
Either way, this is a dupe of bug 545703
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → DUPLICATE
Reporter | ||
Comment 4•14 years ago
|
||
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•14 years ago
|
||
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...
Reporter | ||
Comment 6•14 years ago
|
||
Ah ok, comment #0 in bug 545703 made it sound like two distinct things.
Comment 7•14 years ago
|
||
(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•14 years ago
|
||
Let's get Brad's add-on ported into the code
Status: RESOLVED → REOPENED
tracking-fennec: --- → 2.0b4+
Resolution: DUPLICATE → ---
Updated•14 years ago
|
Assignee: nobody → mbrubeck
Comment 9•14 years ago
|
||
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?
Assignee | ||
Comment 10•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
OS: Linux → All
Hardware: x86 → All
Whiteboard: [strings]
Assignee | ||
Comment 11•14 years ago
|
||
Add a "Reformat text on zoom" setting to the "Content" section of the prefs.
Attachment #499923 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 12•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Attachment #499918 -
Flags: review?(ben)
Attachment #499918 -
Flags: review?(21)
Comment 13•14 years ago
|
||
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 14•14 years ago
|
||
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+
Assignee | ||
Comment 15•14 years ago
|
||
(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: 14 years ago → 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Flags: in-litmus?
Assignee | ||
Updated•14 years ago
|
See Also: → font-inflation
Comment 16•14 years ago
|
||
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•14 years ago
|
||
It turns out this was disabled in bug 627814.
Comment 18•14 years ago
|
||
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
Comment 19•13 years ago
|
||
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.
Description
•