Closed
Bug 800805
Opened 11 years ago
Closed 11 years ago
Reflow on zoom moves the position sideways on the page when the user pinch-zooms
Categories
(Firefox for Android Graveyard :: Toolbar, defect)
Tracking
(firefox18 affected, firefox19 affected, fennec+)
RESOLVED
FIXED
Firefox 23
People
(Reporter: AdrianT, Assigned: jwir3)
References
Details
(Whiteboard: [readability])
Attachments
(1 file, 1 obsolete file)
1.97 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
I see this on CNN, something must be throwing things off with the page layout, CC'ing :jwi3r
Severity: major → normal
Whiteboard: [readability]
Assignee | ||
Comment 2•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Blocks: reflow-zoom
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → sjohnson
Assignee | ||
Comment 3•11 years ago
|
||
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.
Updated•11 years ago
|
tracking-fennec: ? → 18+
Updated•11 years ago
|
tracking-fennec: 18+ → 19+
Comment 5•11 years ago
|
||
Please retest using this build https://tbpl.mozilla.org/?tree=Try&rev=5b22a167473f
Flags: needinfo?(adrian.tamas)
Keywords: qawanted
Assignee | ||
Comment 6•11 years ago
|
||
(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. ;)
Reporter | ||
Comment 7•11 years ago
|
||
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)
Reporter | ||
Comment 8•11 years ago
|
||
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.
Assignee | ||
Comment 9•11 years ago
|
||
(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.
Reporter | ||
Comment 10•11 years ago
|
||
The behavior is the same with the build made locally from the sources as with the build from the ftp server
Assignee | ||
Comment 11•11 years ago
|
||
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
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #726881 -
Flags: review?(bugmail.mozilla)
Comment 13•11 years ago
|
||
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-
Assignee | ||
Comment 14•11 years ago
|
||
(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)
Updated•11 years ago
|
tracking-fennec: 19+ → 21+
Comment 15•11 years ago
|
||
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+
Reporter | ||
Comment 16•11 years ago
|
||
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.
Updated•11 years ago
|
tracking-fennec: 21+ → +
Assignee | ||
Comment 17•11 years ago
|
||
Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/ee3e4ea1ea74
Target Milestone: --- → Firefox 23
Comment 18•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ee3e4ea1ea74
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•