800805 - Reflow on zoom moves the position sideways on the page when the user pinch-zooms

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Adrian Tamas (:AdrianT)]

Aurora 18.0a2 2012-10-11
Device: Samsung Galaxy Tab 2 7.0 (Android 4.0.4)

Steps to reproduce:
1. Go cnn.com and open any article
2. Enable the reflow-on-zoom option
3. Pinch-zoom on an area in the middle column - the article column

Expected results:
The zoom is done on the are the user has zoomed on and the text is rearranged .

Actual results:
The page is zoomed but is also repositioned either left or right.

Notes:
Please see the videocapture: http://youtu.be/ezDhNkBXqsI

Comment 1

[Aaron Train [:aaronmt]]

I see this on CNN, something must be throwing things off with the page layout, CC'ing :jwi3r

Comment 2

[Scott Johnson (:jwir3)]

I'll look into this. It looks to me like the element that's being identified when you pinch-zoom is being incorrectly identified as the containing block of both the right- or left-sidebar as well as the element you _actually_ want to zoom into.

This could be fixed by the next iteration I'm working on, which will identify a block of text, rather than an element, to zoom into.

Thanks for the video capture. It was really handy to see what's actually going on.

Comment 3

[Scott Johnson (:jwir3)]

It looks like on CNN, the article containing element has the following:

padding: 0px 24px 19px 186px;

Which means that the left padding of the article sits underneath the sidebar. This is bad for the current algorithm we're using, because it means that this padding is taken into account when calculating the article rect's left position.

There might be something I can do in the short-term, though, to make this less obvious, until we get a more robust solution working, as I described in comment 2.

Comment 5

[Kevin Brosnan [Ex-Mozilla]]

Please retest using this build https://tbpl.mozilla.org/?tree=Try&rev=5b22a167473f

Comment 6

[Scott Johnson (:jwir3)]

(In reply to Kevin Brosnan [:kbrosnan] from comment #5)
> Please retest using this build
> https://tbpl.mozilla.org/?tree=Try&rev=5b22a167473f

Thanks, kbrosnan. I was actually just going to suggest the same thing. ;)

Comment 7

[Adrian Tamas (:AdrianT)]

Testing with this build (which I believe is the build corresponding to the revision): http://ftp.mozilla.org/pub/mozilla.org/mobile/try-builds/[e-mail]-5b22a167473f/try-android/ I am still able to reproduce the issue. Here is a video capture: http://youtu.be/QI9ih3S-Ex8. I am also working on downloading the sources from the source build and build them localy in order to ensure that my testing was correct.

Comment 8

[Adrian Tamas (:AdrianT)]

I ran into some issues with building the sources and I had to update the Android NDK. Will post the results with the local build as soon as possible.

Comment 9

[Scott Johnson (:jwir3)]

(In reply to adrian tamas from comment #7)
> Testing with this build (which I believe is the build corresponding to the
> revision):
> http://ftp.mozilla.org/pub/mozilla.org/mobile/try-builds/kbrosnan@mozilla.
> com-5b22a167473f/try-android/ I am still able to reproduce the issue. Here
> is a video capture: http://youtu.be/QI9ih3S-Ex8. I am also working on
> downloading the sources from the source build and build them localy in order
> to ensure that my testing was correct.

Ok. Thanks for the detailed feedback. I'll keep this issue open and see if I can reproduce.

Comment 10

[Adrian Tamas (:AdrianT)]

The behavior is the same with the build made locally from the sources as with the build from the ftp server

Comment 11

[Scott Johnson (:jwir3)]

It turns out I think this is caused by not taking into account border and padding when zooming in and snapping to a range. I added a new method that retrieves these values from the style information, and then adjusts the position horizontally according to the values retrieved. 

Note: This can't land until after bug 803719 lands, so I'm requesting review, but it won't actually be fixed until after that patch is reviewed/landed.

Comment 12

[Scott Johnson (:jwir3)]

Attachment: b800805

Comment 13

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

Comment on attachment 726881 [details] [diff] [review]
b800805

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

Seems correct but minusing for now until the comments below are addressed.

::: mobile/android/chrome/content/browser.js
@@ +4497,5 @@
> +    var valueAsString = styleArray[styleValue];
> +    if (!valueAsString) {
> +      return 0;
> +    }
> +    return Number.toInteger(valueAsString.slice(0,valueAsString.length-2));

You can just do | parseInt(valueAsString) | here - parseInt already ignores trailing non-numeric characters, so you don't need to manually strip off the "px".

@@ +4530,5 @@
> +    leftAdjustment += this._getStyleValueInPx(boundingElement, 'padding-left');
> +    leftAdjustment += this._getStyleValueInPx(boundingElement,
> +                                              'borderLeftWidth');
> +    leftAdjustment += this._getStyleValueInPx(boundingElement,
> +                                              'border-left-width');

When do padding-left and border-left-width show up in computed styles? AFAIK those should always return 0 from _getStyleValueInPx because they will never be present in the computed style.

And actually, I believe that the computed style object will always return a value for paddingLeft and borderLeftWidth, so you could simplify this entire block to this:

let style = window.getComputedStyle(boundingElement);
let leftAdjustment = parseInt(style.paddingLeft) + parseInt(style.borderLeftWidth);

.. assuming that "window" is the right window (this function isn't in my local browser.js and I'm too lazy to look at the dependent bug that presumably introduces this function to see what "window" will be used in this context).

Comment 14

[Scott Johnson (:jwir3)]

Attachment: b800805 (v2)

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #13)
> You can just do | parseInt(valueAsString) | here - parseInt already ignores
> trailing non-numeric characters, so you don't need to manually strip off the
> "px".

Cool. I didn't know that. Modified this function, but see below.

> When do padding-left and border-left-width show up in computed styles? AFAIK
> those should always return 0 from _getStyleValueInPx because they will never
> be present in the computed style.

I don't think they ever are present in the computed style. I just saw them in the array returned for the computed style, and thought there might be situations where they are propagated, so I figured I would include them for good measure. 

I've removed these now.

> let style = window.getComputedStyle(boundingElement);
> let leftAdjustment = parseInt(style.paddingLeft) +
> parseInt(style.borderLeftWidth);

Simplified, and I removed the convenience function that I previously added, since it's not needed any longer.

> .. assuming that "window" is the right window (this function isn't in my
> local browser.js and I'm too lazy to look at the dependent bug that
> presumably introduces this function to see what "window" will be used in
> this context).

So, I previously told you that this function was in Tab's prototype. This isn't correct. It's actually within the scope of BrowserEventHandler's prototype.

Comment 15

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

Comment on attachment 727193 [details] [diff] [review]
b800805 (v2)

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

::: mobile/android/chrome/content/browser.js
@@ +4508,5 @@
>      let rect = ElementTouchHelper.getBoundingContentRect(boundingElement);
>      let drRect = aRange.mozGetClientRect();
> +    let scrollTop =
> +      BrowserApp.selectedBrowser.contentDocument.documentElement.scrollTop ||
> +      BrowserApp.selectedBrowser.contentDocument.body.scrollTop;

Might be better to squash these s/var/let/ and formatting changes to the other patch since that contains a larger rewrite of this code anyway.

@@ +4515,5 @@
>      // center the area of interest on the screen.
> +    let topPos = scrollTop + drRect.top - (viewport.cssHeight / 2.0);
> +
> +    // Factor in the border and padding
> +    let boundingStyle = window.getComputedStyle(boundingElement);

This probably works fine, but I think it might be better to use BrowserApp.selectedBrowser.contentWindow.getComputedStyle so that you're using the content window that boundingElement is coming from. Actually even better would be boundingElement.ownerDocument.defaultView.getComputedStyle, because then it'll work even if boundingElement is in an iframe.

Comment 16

[Adrian Tamas (:AdrianT)]

This is still an issue on Firefox Mobile 21 beta 1 on the Samsung Galaxy Tab 2 (Android 4.1.1) on cnn.com even with the new implementation of "pinch to zoom" instead of the "reflow on zoom " general option this was reported on.

Comment 17

[Scott Johnson (:jwir3)]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ee3e4ea1ea74