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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Readability, defect)

Description

[Scott Johnson (:jwir3)]

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.

Comment 1

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

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.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 3

[Scott Johnson (:jwir3)]

Attachment: b808173

Comment 4

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

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?

Comment 5

[Scott Johnson (:jwir3)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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. ;)

Comment 6

[Scott Johnson (:jwir3)]

Inbound, with aforementioned changes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e127a0029ba

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6e127a0029ba