Closed Bug 633279 Opened 9 years ago Closed 9 years ago

1px of checkerboarding between address bar and web page

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set

Tracking

(fennec2.0+)

VERIFIED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: kbrosnan, Assigned: mbrubeck)

References

Details

(Keywords: polish)

Attachments

(5 files)

Attached image screenshot
I am seeing one px of checkerboarding between the address bar and the page.
This might be fixed by fixing bug 630736. Only seems to happen after I run into the other bug.
Assignee: nobody → mbrubeck
tracking-fennec: --- → ?
Keywords: polish
Still seeing this on today's build with the fix for bug 630736. Can reproduce on several sites. 

str
Open page in landscape mode ex. mozilla.com full site
Zoom in
Rotate to portrait mode
Scroll to the top so that the address bar displays
tracking-fennec: ? → 2.0+
This can be reproduced semi-reliably:
1. Load any page.
2. Pan away from the top left of the page.
3. Zoom in using pinch (Android), scroll wheel (desktop), or volume rocker (Maemo).
4. Pan back to the top left.

Whether the bug occurs depends on the exact scroll and zoom amounts, but it happens very often when following these steps.
I should note that I'm building on desktop with --enable-mobile-optimize.  I'll test a build without that option to see if it makes a difference.

I don't see anything obviously wrong in the frontend.  This may be an issue with snapping/rounding in the graphics code, similar to bug 635035 and bug 630743.
See Also: → 635035, 630743
(In reply to comment #5)
> I should note that I'm building on desktop with --enable-mobile-optimize.  I'll
> test a build without that option to see if it makes a difference.

I can still reproduce this bug in a desktop Linux nightly build, which is not built with --enable-mobile-optimize.  So this does not seem to be caused by MOZ_GFX_OPTIMIZE_MOBILE.
This isn't pretty, but I think it's a softblocker
Whiteboard: [fennec-softblocker]
we have to fix this bug, looks terrible
Whiteboard: [fennec-softblocker]
I was curious whether the page was offset by 1px, or cut off by 1px.  It looks like it is offset, based on testing with http://people.mozilla.com/~mbrubeck/test/border.html which has a 1px green border around the edge of the page.
Attached file _panContentView log
Here's a log of the offset values passed into and out of Browser.MainDragger.prototype._panContentView, along with the results from getPosition() before and after scrolling the view.

The bug happens when the position ends up at a value between -0.5 and -1.0.
The bug is in the remote browser contentView scrollBy function:

1) Let contentView's scrollY start at 4.01.
2) Now, call scrollBy with y = -100.
3) We calculate the new position: newY = (oldY + y) = (4.01 - 100) = -95.99
4) We clamp that to newY = Math.max(0, result) = 0.
5) Then set y = (newY - scrollY) = (0 - 4.01) = -4.01.
6) Then we floor the result: y = Math.floor(y) = Math.floor(-4.01) = -5.
7) Then we scroll by -5, and end up with a new scrollY of -0.99.
Attached patch patchSplinter Review
The solution is to do the subtraction *after* the Math.floor call.  For the example above, this changes the calculation to:

1) oldY = 4.01
2) y = -100
3) newY = (oldY + y) = -95.99
4) newY = Math.max(0, Math.min(newY, scrollRangeY)) = 0
5) newY = Math.floor(newY) = 0
6) y = (newY - scrollY) = (0 - 4.01) = -4.01
7) scrollY = (oldY + y) = 0

This also causes us to always scroll to integer numbers of pixels, which I think is a good thing (?).
Attachment #516395 - Flags: review?(ben)
Comment on attachment 516395 [details] [diff] [review]
patch

Looks good. Like I said on IRC, it'd be nice to know why oldY was ever fractional in the first place, but it makes sense that this would fix the problem.
Attachment #516395 - Flags: review?(ben) → review+
http://hg.mozilla.org/mobile-browser/rev/62e1753d84e4
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Backed out because this might have caused a Tzoom regression (?!):
http://hg.mozilla.org/mobile-browser/rev/cee75aa37765

https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/Kv0ohqO8x3o
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [backed out, waiting for Talos results from backout]
Re-landed: http://hg.mozilla.org/mobile-browser/rev/5d299232e62a

The backout was inconclusive; Tzoom went down slightly but not back to the levels prior to landing.  I'll keep an eye on the numbers after the re-landing.

Before landing: 382, 377, 381, 391
After landing:  445, 461, 459, 449
After backout:  415, 406, 437

Tzoom times code that is not really in the hot path for Fennec; we should also look into better benchmarks that use the same fuzzy-zoom code path that our UI does.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: [backed out, waiting for Talos results from backout]
After re-landing: 455, 455, 419

It looks like most or all of the regression is from bug 624454, and is probably an expected trade-off for that change.
Verified Mozilla/5.0 (Android; Linux armv7l; rv:2.0b13pre)
Gecko/20110315 Firefox/4.0b13pre Fennec/4.0b6pre ID:20110315035936
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.