Closed Bug 800805 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox for Android :: Toolbar, defect)

18 Branch
ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 23
Tracking Status
firefox18 --- affected
firefox19 --- affected
fennec + ---

People

(Reporter: AdrianT, Assigned: jwir3)

References

Details

(Whiteboard: [readability])

Attachments

(1 file, 1 obsolete file)

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
I see this on CNN, something must be throwing things off with the page layout, CC'ing :jwi3r
Severity: major → normal
Whiteboard: [readability]
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.
Blocks: reflow-zoom
Assignee: nobody → sjohnson
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.
tracking-fennec: ? → 18+
Duplicate of this bug: 816108
Depends on: 667243
tracking-fennec: 18+ → 19+
Please retest using this build https://tbpl.mozilla.org/?tree=Try&rev=5b22a167473f
Flags: needinfo?(adrian.tamas)
Keywords: qawanted
(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. ;)
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.
Flags: needinfo?(adrian.tamas)
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.
(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.
Keywords: qawanted
The behavior is the same with the build made locally from the sources as with the build from the ftp server
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.
Depends on: 803719
Attached patch b800805 (obsolete) — Splinter Review
Attachment #726881 - Flags: review?(bugmail.mozilla)
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).
Attachment #726881 - Flags: review?(bugmail.mozilla) → review-
Attached patch b800805 (v2)Splinter Review
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attachment #726881 - Attachment is obsolete: true
Attachment #727193 - Flags: review?(bugmail.mozilla)
tracking-fennec: 19+ → 21+
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.
Attachment #727193 - Flags: review?(bugmail.mozilla) → review+
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.
tracking-fennec: 21+ → +
Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee3e4ea1ea74
Target Milestone: --- → Firefox 23
https://hg.mozilla.org/mozilla-central/rev/ee3e4ea1ea74
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.