873721 - Reflow-on-zoom is triggered on some sites by pinch-zooming in, then out again

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Readability, defect)

Description

[Stephen Moehle]

The "Double tap to reflow text" option in Settings does not seem to have any effect.

To reproduce:
1) Set the "Double tap to reflow text" option in Settings.
2) Go to a web that is mostly text, such as http://m.sfgate.com/sfchron/db_41154/contentdetail.htm?contentguid=EZPM7Q52&detailindex=3&pn=0&ps=10&full=true#
3) Double tap on the text.

Expected results:
The page should zoom in and the text should reflow.

Actual results:
Nothing. The page does not change in any way.

Build: 24.0a1 (2013-05-17)

If I zoom in and zoom out again after double tapping, I tend to get a very narrow column of text that is about 1/4 the width of the screen.

I have tested this on a Nexus 7 tablet and an Atrix 2 phone, and the behavior is the same on both.

Comment 1

[Scott Johnson (:jwir3)]

I can confirm that pinch-zooming in and then out again does indeed trigger a reflow where the max-line-box width is about 1/4 of the screen. 

However, the bug as it's originally filed isn't quite a bug - the reflow-on-zoom system specifically does nothing if the site is optimized for mobile. Even checking the 'Request desktop site' on the SFGate link as given produces a mobile-optimized site, which is why double-tapping does nothing. 

So, I'm changing the title of this bug to more accurately track the issue in question.

Comment 2

[Stephen Moehle]

I still think the first issue I reported is critical.  The whole reason I am double tapping in the first place is that the text is too small for me to read comfortably. I had forgotten that I have the Always Zoom extension installed, which allowed pinch-zoom and reflow text to work even on sites that are optimized for mobile

I see 3 ways to fix this: 1) Honor what the Always Zoom extension is trying to do. 2) Ignore "optimized for mobile" as far as zooming goes. 3) Add a setting that allows the user to explicitly request that "optimized for mobile" be ignored.

What this really comes down to is that I think I should be able to read the text that Firefox renders. I do not care whether the web designer thinks I can read the text. I do not care whether you the developer thinks I can read the text. All I care about is whether I can read the text or not.

Finally, I really liked reflow text on pinch-zoom and would be happy if it came back.

Comment 3

[Scott Johnson (:jwir3)]

(In reply to Stephen Moehle from comment #2)
> I still think the first issue I reported is critical.  The whole reason I am
> double tapping in the first place is that the text is too small for me to
> read comfortably. 

I understand your concerns. I apologize if you felt that I was minimizing your concerns. That wasn't my intent, simply to explain the decision process that has gone in to this feature.

> I had forgotten that I have the Always Zoom extension
> installed, which allowed pinch-zoom and reflow text to work even on sites
> that are optimized for mobile

Well, the problem here is that pinch-zooming is enabled on (most) sites that are mobile-optimized, such as the site you give as an example. On the other hand, double-tap to zoom is not enabled in these cases.

> I see 3 ways to fix this: 1) Honor what the Always Zoom extension is trying
> to do. 

This isn't really a great option, because we would be basing our platform code on the behavior of an addon.

2) Ignore "optimized for mobile" as far as zooming goes.

"optimized for mobile" usually means that zoom is specifically set, so ignoring this property would, in effect, be ignoring what the specification dictates the behavior will include.
 
> 3) Add a setting that allows the user to explicitly request that "optimized for
> mobile" be ignored.

This we might be able to do. If you could open a bug to this effect, this would probably facilitate the discussion.

> What this really comes down to is that I think I should be able to read the
> text that Firefox renders. I do not care whether the web designer thinks I
> can read the text. I do not care whether you the developer thinks I can read
> the text. All I care about is whether I can read the text or not.

As I said, I understand your concerns. The problem is that we, as developers, have to make decisions based on data that helps makes the majority of sites readable. If a web developer has specifically stated that they wish a default size or zoom range to be set for their website, we must honor that request, regardless of whether we think that we can assist the reader in reading the text. Much of our font size enhancements for readability work within the specification and what we perceive as the web developers' intent for display on the page. Sometimes tradeoffs need to be made.

> Finally, I really liked reflow text on pinch-zoom and would be happy if it
> came back.

Again, I understand, but this is something that was decided as part of our project process. No one arbitrarily decided, "Nope, we're not going to support reflow-on-zoom on pinch". It was a decision that was made based on data that we had from user research and the testing of the feature we've performed thus far. It just turned out that it works a bit better in double-tap mode than pinch-zoom mode.

However, the API for adjusting max line box widths does still exist, and it could probably be massaged to work within the context of an add-on fairly easily. (That is, I'm saying I'd be willing to assist someone if they wanted to write an addon for reflow-on-zoom triggered by pinch-zoom instead of double-tap zoom. ;> )

Comment 4

[Scott Johnson (:jwir3)]

Stephen: 

Also, I just noticed that the preference to zoom in on double-tap that makes the font size more readable didn't get added in the patch that made reflow-on-zoom double-tap only. See bug 873969.

Try changing 'browser.zoom.reflowZoom.minFontSizeTwips' to something other than it's current default value (0) in about:config. I'd recommend 120 to start with.

Comment 5

[Stephen Moehle]

I have added bug 874732 for a setting that would cause Firefox to ignore "optimized for mobile", at least as far as zooming is concerned.

Setting browser.zoom.reflowZoom.minFontSizeTwips helps a lot.

Comment 6

[Scott Johnson (:jwir3)]

Another thing that we should look into when this bug is fixed is that pinch-zoom still snaps into text when reflow-on-zoom is enabled. This should not happen.

Comment 7

[Scott Johnson (:jwir3)]

Attachment: b873721

This patch does two things: 1) it removes the code that calls the position maintenance functionality for reflow-on-zoom from the onPinchHandler, so that we don't adjust position during a pinch-zoom event, and 2) it adds back the onPinchFinish() handler so that we can reset the max line box width when pinch-zooming out.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 756054 [details] [diff] [review]
b873721

Review of attachment 756054 [details] [diff] [review]:
-----------------------------------------------------------------

Minusing for now, please see below.

::: mobile/android/chrome/content/browser.js
@@ +4269,5 @@
> +     // 0.5 is an arbitrary constant here. Basically, this causes a zoom
> +     // to page width, so we only want to do this if the user zoomed out
> +     // "enough".
> +     if (BrowserEventHandler.mReflozPref && zoom < 0.5) {
> +       this._zoomOut();

I'm not clear on why we want to zoom to page width here. If I'm reading this right, any time you end a pinch and the zoom is less than 0.5 (which can happen after a pinch-zoom-in as well) it will zoom out to page width. This can lead to annoying situations where you zoom in and it zooms you back out.

I think doing a reset of the max line box width is all that is needed here, and even then, I'm not sure about the zoom level condition - I think we probably just want to remove that clause entirely, so that any pinch zoom after a double-tap will clear the line box width (which presumably is only valid for the viewport the double-tap resulted in).

Comment 9

[Scott Johnson (:jwir3)]

Attachment: b873721 (v2)

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> Comment on attachment 756054 [details] [diff] [review]
> b873721
> 
> Review of attachment 756054 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Minusing for now, please see below.
> 
> ::: mobile/android/chrome/content/browser.js
> @@ +4269,5 @@
> > +     // 0.5 is an arbitrary constant here. Basically, this causes a zoom
> > +     // to page width, so we only want to do this if the user zoomed out
> > +     // "enough".
> > +     if (BrowserEventHandler.mReflozPref && zoom < 0.5) {
> > +       this._zoomOut();
> 
> I think doing a reset of the max line box width is all that is needed here,
> and even then, I'm not sure about the zoom level condition - I think we
> probably just want to remove that clause entirely, so that any pinch zoom
> after a double-tap will clear the line box width (which presumably is only
> valid for the viewport the double-tap resulted in).

So, I'm adding the following patch for feedback, because I think this patch is closer, but it's still not quite what I'm looking to do. 

Basically, what I want is the following behavior:

1) User double-taps to zoom in, causing a max line box width change.
2) User pinch-zooms a bit more, which DOES NOT cause a max line box width change.
3) User pinch-zooms out, which does cause the max line box width to be reset. (Actually, I think this should be another max line box width change, as ideally, if the user doesn't zoom all the way out, then the line box width should be set to the viewport width).

So, what I need is a way to distinguish between a pinch-zoom IN event and a pinch-zoom OUT event. 

> I'm not clear on why we want to zoom to page width here. If I'm reading this
> right, any time you end a pinch and the zoom is less than 0.5 (which can
> happen after a pinch-zoom-in as well) it will zoom out to page width. This
> can lead to annoying situations where you zoom in and it zooms you back out.

I thought that, after a pinch-zoom event happens, that if the zoom is greater than 1, then it's a zoom in event, otherwise it's a zoom out event. I just wanted to take a stab at the magnitude of the zoom out, which is why I chose 0.5 instead of 1.0 (i.e. if they are zooming out a large amount, then I want to reset to the page width). Granted, this could cause annoying behavior if that's not exactly what the user wanted. 

This is the behavior I've seen thus far. How could a pinch-zoom in cause the zoom to be less than 0.5? Is there a better way to detect the direction of zoom?

Comment 10

[Kartikaya Gupta (email:[e-mail])]

(In reply to Scott Johnson (:jwir3) from comment #9)
> So, what I need is a way to distinguish between a pinch-zoom IN event and a
> pinch-zoom OUT event. 
> 
> I thought that, after a pinch-zoom event happens, that if the zoom is
> greater than 1, then it's a zoom in event, otherwise it's a zoom out event.
> I just wanted to take a stab at the magnitude of the zoom out, which is why
> I chose 0.5 instead of 1.0 (i.e. if they are zooming out a large amount,
> then I want to reset to the page width). Granted, this could cause annoying
> behavior if that's not exactly what the user wanted. 
> 
> This is the behavior I've seen thus far. How could a pinch-zoom in cause the
> zoom to be less than 0.5? Is there a better way to detect the direction of
> zoom?

BrowserApp.selectedTab._zoom (which is what you were checking in the first version of the patch) represents the final zoom value the page is rendered at. A zoom of 1.0 means each CSS pixels maps to 1 device pixel. i.e. that variable is not relative to the previous zoom, but is the absolute zoom of the page. To put it another way, say the page starts of at 1.0 zoom. Then the user pinch-zooms out. At this point the zoom might go to say 0.25. Then the user zooms back in, but not all the way back to the original view. Now the zoom might be something like 0.75. Note that both pinches resulted in a zoom value < 1.0 even though one was a zoom in and one was zoom out.

To detect whether or not a pinch is a zoom in or out, you'll need to record the zoom value at the start of the pinch and after the new zoom is applied, and compare the two. I don't recall exactly when those MozMagnify events are dispatched, but it's possible that the new zoom value isn't updated on BrowserApp.selectedTab._zoom until after the MozMagnifyGesture event, so that's something else to watch out for. The zoom value is updated when browser.js receives a Viewport:Change event from java after the pinch is finished.

Comment 11

[Scott Johnson (:jwir3)]

Attachment: b873721 (v3)

New patch that does what we discussed.

Comment 12

[Scott Johnson (:jwir3)]

Try server run:
https://tbpl.mozilla.org/?tree=Try&rev=47252d170ed7

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 757604 [details] [diff] [review]
b873721 (v3)

Removing review flag to get this out of my queue. As discussed on IRC, I think using the "delta" property of the MozMagnifyGesture (as documented at mxr.mozilla.org/mozilla-central/source/dom/interfaces/events/nsIDOMSimpleGestureEvent.idl#150) would be preferable to having the _mIsZoomingOut property.

Comment 14

[Scott Johnson (:jwir3)]

Attachment: b873721 (v4)

Addressed review comments as discussed in IRC and in previous comment.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 758170 [details] [diff] [review]
b873721 (v4)

Review of attachment 758170 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: mobile/android/chrome/content/browser.js
@@ +4300,5 @@
> +     let data = {};
> +     try {
> +       data = JSON.parse(aData);
> +     } catch(ex) {
> +       console.log(ex);

Add a return here, because otherwise it might fall through and try to use data.zoomDelta which won't exist.

Comment 16

[Scott Johnson (:jwir3)]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/87f0ed19ef8d

Comment 18

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 24.0a1( 2013-06-24)
Device: HTC One 
OS: Android 2.3.5