633279 - 1px of checkerboarding between address bar and web page

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kevin Brosnan]

Attachment: screenshot

I am seeing one px of checkerboarding between the address bar and the page.

Comment 1

[Kevin Brosnan]

This might be fixed by fixing bug 630736. Only seems to happen after I run into the other bug.

Comment 2

[Kevin Brosnan [:kbrosnan]]

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

Comment 3

[Kevin Brosnan [:kbrosnan]]

Attachment: checkerboarding on the right edge of the window

Comment 4

[Matt Brubeck (:mbrubeck)]

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.

Comment 5

[Matt Brubeck (:mbrubeck)]

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.

Comment 6

[Matt Brubeck (:mbrubeck)]

(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.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

This isn't pretty, but I think it's a softblocker

Comment 8

[Stuart Parmenter]

we have to fix this bug, looks terrible

Comment 9

[Matt Brubeck (:mbrubeck)]

Attachment: screenshot of page with border

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.

Comment 10

[Matt Brubeck (:mbrubeck)]

Attachment: _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.

Comment 11

[Matt Brubeck (:mbrubeck)]

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.

Comment 12

[Matt Brubeck (:mbrubeck)]

Attachment: patch

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 (?).

Comment 13

[Benjamin Stover (:stechz)]

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.

Comment 14

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mobile-browser/rev/62e1753d84e4

Comment 15

[Matt Brubeck (:mbrubeck)]

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

Comment 16

[Matt Brubeck (:mbrubeck)]

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.

Comment 17

[Matt Brubeck (:mbrubeck)]

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.

Comment 18

[Kevin Brosnan [:kbrosnan]]

Verified Mozilla/5.0 (Android; Linux armv7l; rv:2.0b13pre)
Gecko/20110315 Firefox/4.0b13pre Fennec/4.0b6pre ID:20110315035936