Closed Bug 1229125 Opened 4 years ago Closed 4 years ago

One can continue to cause checkerboarding after scrolling to the bottom of a page

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: jrmuizel, Assigned: mstange)

References

Details

Attachments

(3 files)

STR:
- turn on apz.highlight_checkerboarded_areas
- on MBP scroll quickly using the trackpad to the bottom of a page.
- continue to keep scrolling quickly and the page will occaisionally flicker red
Do you see a flicker (of white / the page background color) with the pref off?
I can still reproduce this in a build that has the fix for bug 1198021.
No longer depends on: 1198021
(In reply to Botond Ballo [:botond] from comment #1)
> Do you see a flicker (of white / the page background color) with the pref
> off?

I do.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> (In reply to Botond Ballo [:botond] from comment #1)
> > Do you see a flicker (of white / the page background color) with the pref
> > off?
> 
> I do.

Ok, thanks. That tells us this is actual checkerboarding happening, not just a bug in the checkerboard highlight code.
Here are the relevant parts from a layers.dump-host-layers log. Based on the scrolling "speed", we sometimes enlarge the displayport, but it gets placed too high and doesn't end up intersecting the visible part of the layer. Look for the lines towards the end that have a shadow y translation of -134345 and a valid region with bottom < 134345.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Bug 1229125 - Correct velocity computation for pan gesture events. r?botond

When scrolling down, panInput.mLocalDisplacement.y will be positive, so
aAdditionalDelta will be positive, and newVelocity must be positive.
Attachment #8694197 - Flags: review?(botond)
Bug 1229125 - Clear the velocity queue if we're at the edge of the page. r?botond

Resetting only the current velocity isn't sufficient because EndTouch will
reconstruct a velocity from the velocity queue once the fingers leave the
touchpad.
Attachment #8694198 - Flags: review?(botond)
https://reviewboard.mozilla.org/r/26701/#review24195

This causes two tests in the APZOverscrollHandoffTester gtest to fail. I need to find out why.
I'll just drop that patch for now. It's not necessary to fix the bug.
Comment on attachment 8694197 [details]
MozReview Request: Bug 1229125 - Correct velocity computation for pan gesture events. r?botond

https://reviewboard.mozilla.org/r/26699/#review24327

::: gfx/layers/apz/src/Axis.cpp:79
(Diff revision 1)
> -  float newVelocity = mAxisLocked ? 0.0f : (float)(mPos - aPos - aAdditionalDelta) / (float)(aTimestampMs - mPosTimeMs);
> +  float newVelocity = mAxisLocked ? 0.0f : (float)(mPos - aPos + aAdditionalDelta) / (float)(aTimestampMs - mPosTimeMs);

Can you help me understand why this won't regress bug 1212876, where we made the opposite change?
Attachment #8694197 - Flags: review?(botond)
Comment on attachment 8694198 [details]
MozReview Request: Bug 1229125 - Reset velocity in OnPanEnd if there's nowhere to scroll. r?botond

Dropping review flag until the test failures are cleared up.
Attachment #8694198 - Flags: review?(botond)
https://reviewboard.mozilla.org/r/26699/#review24327

I think the change I made in bug 1212876 was just wrong.
Could it be that for the x-axis we need a - and for the y-axis we need a + ?
I don't think so.

It made sense to invert the sign in bug 1212876 because the value I was passing in also got its sign inverted, but that just means that calculation had the wrong sign all along. Now that I've actually looked at the computed velocity values and compared them to the velocity values that I get from smooth scrolling with a regular mousewheel, I'm confident that the patch here is the right fix.
Comment on attachment 8694197 [details]
MozReview Request: Bug 1229125 - Correct velocity computation for pan gesture events. r?botond

https://reviewboard.mozilla.org/r/26699/#review24335

OK, thanks. I just wanted to make sure we weren't making a changing without realizing that we recently made the opposite change :)
Attachment #8694197 - Flags: review+
Comment on attachment 8694197 [details]
MozReview Request: Bug 1229125 - Correct velocity computation for pan gesture events. r?botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26699/diff/1-2/
Comment on attachment 8694198 [details]
MozReview Request: Bug 1229125 - Reset velocity in OnPanEnd if there's nowhere to scroll. r?botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26701/diff/1-2/
Attachment #8694198 - Attachment description: MozReview Request: Bug 1229125 - Clear the velocity queue if we're at the edge of the page. r?botond → MozReview Request: Bug 1229125 - Reset velocity in OnPanEnd if there's nowhere to scroll. r?botond
Attachment #8694198 - Flags: review?(botond)
Comment on attachment 8694198 [details]
MozReview Request: Bug 1229125 - Reset velocity in OnPanEnd if there's nowhere to scroll. r?botond

https://reviewboard.mozilla.org/r/26701/#review24339
Attachment #8694198 - Flags: review?(botond) → review+
https://hg.mozilla.org/mozilla-central/rev/378af567670f
https://hg.mozilla.org/mozilla-central/rev/b51a04a3514e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.