Last Comment Bug 763838 - Spacing between lines changes while pixel scrolling on OS X
: Spacing between lines changes while pixel scrolling on OS X
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal with 1 vote (vote)
: mozilla16
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
: 756400 (view as bug list)
Depends on: 767710
Blocks: 681192
  Show dependency treegraph
 
Reported: 2012-06-11 23:41 PDT by Jeff Muizelaar [:jrmuizel]
Modified: 2013-04-11 15:33 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed


Attachments
fix? (2.17 KB, patch)
2012-06-22 04:21 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
tnikkel: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Jeff Muizelaar [:jrmuizel] 2012-06-11 23:41:34 PDT
I've been seeing this for the last little while. I'm not sure what causes it and I don't really have a good way of reproducing the problem.

Basically it feels like text lines jump up and down a single pixel every now and then during scroll.
Comment 1 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2012-06-12 00:14:07 PDT
I've been seeing this the past few weeks on Linux as well.

In the past it's been caused by trying to scroll by non-integer-pixel amounts, which means that when we repaint, we redo rounding.  It could also be caused by incorrect intermediate rounding (i.e., snapping to nearest pixel relative to something that's not actually pixel aligned).
Comment 2 Timothy Nikkel (:tnikkel) 2012-06-12 09:41:33 PDT
Maybe caused by bug 681192?
Comment 3 Alex Keybl [:akeybl] 2012-06-19 20:43:41 PDT
Adding qawanted, but it'd be great if Jeff or David could reproduce, since there's not much info for QA to go off of right now (affected URLs, etc.).
Comment 4 Jeff Muizelaar [:jrmuizel] 2012-06-19 20:52:01 PDT
I can reproduce somewhat reliably at right at the bottom of this page:
http://people.mozilla.com/~jmuizelaar/tests/thread.html

turning on paintflashing may help one see it as the changes only happen when the entire page is invalidated.
Comment 5 Timothy Nikkel (:tnikkel) 2012-06-19 22:29:57 PDT
That makes me think of http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#1033

And commenting out that chunk does seem to make the whole page invalidate go away on your testcase.
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-20 20:22:10 PDT
Undoubtedly it's bug 681192. I don't think I have the right hardware to test this unfortunately.

The bug is probably that we're not passing a big enough range to nsGfxScrollFrameInner::ScrollToWithOrigin, so we're not finding a place to scroll to that gives us a translation by a whole number of device pixels.

In nsGfxScrollFrameInner::ScrollBy, under case nsIScrollableFrame::DEVICE_PIXELS, try setting positiveTolerance to something large, say 100. I expect that would fix it, although it's not really the right fix. The current tolerances there (half a device pixel in each direction) should really be enough.
Comment 7 Timothy Nikkel (:tnikkel) 2012-06-20 22:39:56 PDT
Comment 5 wasn't completely accurate. Commenting out that chunk dramatically reduces the full page invalidates, but they still happen.

Doing what comment 6 said didn't seem to have any effect on the whole page invalidates.
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-20 22:45:48 PDT
How about in RestrictToLayerPixels, are we hitting the "return aDesired" path? Ideally we won't be. If we are, then examining the call stack state should help a lot.
Comment 9 Timothy Nikkel (:tnikkel) 2012-06-20 23:18:41 PDT
Yeah, we are hitting that. I'll get a stack when my debug build finishes.
Comment 10 Timothy Nikkel (:tnikkel) 2012-06-21 00:42:15 PDT
The scroll range for the page is not exactly an integer number of device pixels (ie not divisible by 60). When we are pixel scrolling and are near the bottom one pixel scroll event finally asks for a scroll position past where the past can scroll. nsGfxScrollFrameInner::ScrollBy calls ScrollToWithOrigin with aScrollPosition past the scroll range of the page, and a suitable aRange that surrounds this aScrollPosition. We clamp the aScrollPosition to the valid scroll range, but we pass aRange untouched to the AsyncScroll. When the AsyncScroll happens that calls ScrollToImpl and that finally clamps the range in ClampAndRestrictToLayerPixels, and it clamps it with no allowable wiggle room. So we scroll to a not integral dev pixel position and I think that causes the badness.
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-21 03:09:48 PDT
If this only happens when scrolling to the very bottom of the page and the bottom isn't at the same subpixel offset as the top, then this is expected.
Comment 12 Timothy Nikkel (:tnikkel) 2012-06-21 05:34:30 PDT
No, it's more than that, I'll look more.
Comment 13 Timothy Nikkel (:tnikkel) 2012-06-21 07:47:02 PDT
The problem occurs after the situation of comment 10. After we've scrolled to the bottom and get a scroll up ScrollBy subtracts an integral number of dev pixels from mDestination, but mDestination isn't an integral number of dev pixels at this point. ScrollToImpl makes sure we scroll to an integral dev pixel, but mDestination doesn't get updated to the integral dev pixel value, its still at a fractional dev pixel. The problem comes when we get a horizontal pixel scroll (they are pretty easy to trigger since they can be as small as a single pixel). When we get a horizontal pixel scroll ScrollBy takes the y value in mDestination and the y delta is 0 so CalcRangeForScrollBy returns a y scroll range with zero height.
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-21 19:28:21 PDT
(In reply to Timothy Nikkel (:tn) from comment #13)
> The problem occurs after the situation of comment 10. After we've scrolled
> to the bottom and get a scroll up ScrollBy subtracts an integral number of
> dev pixels from mDestination, but mDestination isn't an integral number of
> dev pixels at this point. ScrollToImpl makes sure we scroll to an integral
> dev pixel, but mDestination doesn't get updated to the integral dev pixel
> value, its still at a fractional dev pixel.

That's where the problem is, I think. 

I think nsGfxScrollFrameInner::ScrollToWithOrigin, after it synchronously calls ScrollToImpl, should set mDestination after to the new scroll position. Likewise in nsGfxScrollFrameInner::AsyncScrollCallback, when we reach the scroll destination and clear mAsyncScroll, we should also set self->mDestination to the new scroll position after ScrollToImpl has been called.
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-22 04:21:21 PDT
Created attachment 635690 [details] [diff] [review]
fix?

Completely untested. Does this fix it?
Comment 16 Timothy Nikkel (:tnikkel) 2012-06-22 09:17:02 PDT
Comment on attachment 635690 [details] [diff] [review]
fix?

Yes it does. I was just about to post this patch and ask for your review. :)
Comment 17 Timothy Nikkel (:tnikkel) 2012-06-22 09:22:38 PDT
Comment on attachment 635690 [details] [diff] [review]
fix?

I'm going to add the following comment

  // We are done scrolling, set our destination to wherever we actually ended
  // up scrolling to.

to your patch and push.
Comment 18 Timothy Nikkel (:tnikkel) 2012-06-22 09:26:00 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cb9b553b68a
Comment 19 Timothy Nikkel (:tnikkel) 2012-06-22 09:29:12 PDT
I noticed that in ScrollToWithOrigin we clamp aScrollPosition and then if aRange is null we set range to be nsRect(aScrollPosition, nsSize(0, 0)). Do we actually want to use the clamped aScrollPosition for the range? If so we can do that in a another bug.
Comment 20 Ryan VanderMeulen [:RyanVM] 2012-06-23 05:46:53 PDT
https://hg.mozilla.org/mozilla-central/rev/4cb9b553b68a
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-23 06:03:27 PDT
Comment on attachment 635690 [details] [diff] [review]
fix?

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

This fixes a regression that's in Aurora. The patch is very low risk.
Comment 22 Alex Keybl [:akeybl] 2012-06-24 13:26:18 PDT
Comment on attachment 635690 [details] [diff] [review]
fix?

[Triage Comment]
Low risk fix to a regression in 15.
Comment 23 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-24 18:12:19 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/65584e422850
Comment 24 Jeff Muizelaar [:jrmuizel] 2012-07-09 14:57:16 PDT
I still see this on this page:
http://lists.cs.uiuc.edu/pipermail/llvmdev/2012-July/thread.html
Comment 25 Timothy Nikkel (:tnikkel) 2012-07-09 21:47:36 PDT
You mean the paint flashing? I don't think my eyes are tuned to see the other effect you described. I see some paint flashing on that page, but much less than the original. If that is what you are experiencing I think a new bug would be best to handle that.
Comment 26 Jeff Muizelaar [:jrmuizel] 2012-07-10 07:17:50 PDT
(In reply to Timothy Nikkel (:tn) from comment #25)
> You mean the paint flashing? I don't think my eyes are tuned to see the
> other effect you described. I see some paint flashing on that page, but much
> less than the original. If that is what you are experiencing I think a new
> bug would be best to handle that.

I was seeing space changes, but I probably shouldn't have posted a live url because I can't reproduce it anymore either.
Comment 27 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-07-10 14:06:12 PDT
Is qawanted still needed on this bug, or does QA simply need to verify this fixed?
Comment 28 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-07-12 16:11:15 PDT
As per discussion in meeting today, removing qawanted.
Comment 29 Karl Tomlinson (:karlt) 2013-04-11 15:33:01 PDT
*** Bug 756400 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.