Closed Bug 808173 Opened 8 years ago Closed 7 years ago

Make reflow-on-zoom turn off on non-text elements/blocks

Categories

(Firefox for Android Graveyard :: Readability, defect)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: jwir3, Assigned: jwir3)

References

Details

Attachments

(1 file)

Currently, if reflow on zoom is enabled, it will perform its functionality on any zoom. We probably don't want this for some types of non-text elements/blocks. For example, it's unlikely that we want the text around an image to reflow, if the user is only trying to zoom in to see the image more clearly. Similarly, we probably don't want to reflow preformatted text, such as code.

We might want to make this perform at some sophisticated way, such as detecting which parts of the page are parts of "articles", but this is a hard problem and may not be something we can do in the short-term.
To be honest I think a simple and cheap heuristic would be worth trying here before going for a more complicated one. Something like:

1) Take all of the elements that have a large intersection with the visible area (or that you can otherwise tell the user is zooming in to)
2) For each block level element in (1), get the length of the text content. If the length is more than... 200 characters, reflow it.

Adjust "200" as needed.

The reason I think this will work from a user-experience point of view is that as a user, the reason you want text to reflow is because it's too long to read without horizontal scrolling. If it's just one or two lines of text (less than ~200 characters), you can probably live with the horizontal scrolling, but if it's longer than that, you probably want it wrapped for easy reading. This applies to preformatted text, image captions, article text, headlines, really any piece of text. If you're gonna read it and it's long, you'll want it wrapped regardless of what it is on the page.
But if the model is that reflow-on-zoom does the reflows when the user zooms, and then the user can pan around without reflows, you shouldn't be deciding to make a decision on whether to reflow based on what the user is zooming in on, since that will mean that if the user zooms in on something where it's disabled and then pans to something disabled, you either end up in a broken state or you end up with reflow-on-pan (which I don't think we want).

Is the underlying push for this really that the position maintenance code for some of these things isn't really working yet?
Blocks: reflow-zoom
Attached patch b808173Splinter Review
Attachment #786966 - Flags: review?(bugmail.mozilla)
Comment on attachment 786966 [details] [diff] [review]
b808173

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

::: mobile/android/chrome/content/browser.js
@@ +4328,5 @@
>    },
>  
>    onDoubleTap: function(aData) {
>      let data = JSON.parse(aData);
> +    let zoom = BrowserApp.selectedTab._zoom;

Why did you move the zoom variable to up here? It doesn't look like it's even used, can we just remove it?
Attachment #786966 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> Comment on attachment 786966 [details] [diff] [review]
> b808173
> 
> Review of attachment 786966 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/chrome/content/browser.js
> @@ +4328,5 @@
> >    },
> >  
> >    onDoubleTap: function(aData) {
> >      let data = JSON.parse(aData);
> > +    let zoom = BrowserApp.selectedTab._zoom;
> 
> Why did you move the zoom variable to up here? It doesn't look like it's
> even used, can we just remove it?

Ah, thanks. I actually meant to remove it. I guess I inadvertantly pasted both lines after cutting. ;)
https://hg.mozilla.org/mozilla-central/rev/6e127a0029ba
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Product: Firefox for Android → Fennec Graveyard
You need to log in before you can comment on or make changes to this bug.