Closed Bug 763838 Opened 13 years ago Closed 13 years ago

Spacing between lines changes while pixel scrolling on OS X

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla16
Tracking Flags:
Tracking Status
firefox15 + fixed
firefox16 + fixed

People

(Reporter: jrmuizel, Assigned: roc)

References

Details

(Keywords: regression)

Attachments

(1 file)

fix?
2.17 KB, patch
tnikkel
: review+
akeybl
: approval-mozilla-aurora+
Details | Diff | Splinter Review
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.
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).
Maybe caused by bug 681192?
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.).
tracking-firefox16: ? → +
Keywords: qawanted
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.
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.
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 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.
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.
Yeah, we are hitting that. I'll get a stack when my debug build finishes.
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.
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.
No, it's more than that, I'll look more.
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.
(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.
Attached patch fix? — — Splinter Review
Completely untested. Does this fix it?
Assignee: nobody → roc
Attachment #635690 - Flags: review?(tnikkel)
Comment on attachment 635690 [details] [diff] [review] fix? Yes it does. I was just about to post this patch and ask for your review. :)
Attachment #635690 - Flags: review?(tnikkel) → review+
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.
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.
https://hg.mozilla.org/mozilla-central/rev/4cb9b553b68a
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
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.
Attachment #635690 - Flags: approval-mozilla-aurora?
Depends on: 767710
Comment on attachment 635690 [details] [diff] [review] fix? [Triage Comment] Low risk fix to a regression in 15.
Attachment #635690 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
(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.
Is qawanted still needed on this bug, or does QA simply need to verify this fixed?
As per discussion in meeting today, removing qawanted.
Keywords: qawanted
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: