Closed Bug 1258851 Opened 4 years ago Closed 4 years ago

Scrollbar dragging with scroll-to-click goes crazy and oscillates between the top and the bottom end of the scrollbar

Categories

(Core :: Panning and Zooming, defect)

48 Branch
All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- unaffected
firefox48 --- fixed

People

(Reporter: mstange, Assigned: kats)

References

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(2 files, 1 obsolete file)

STR:
 1. On OS X, in your system settings under General, set "Click in the scrollbar to:" to "Jump to the spot that's clicked".
 2. If you're using overlay scrollbars, scroll a little on any page to make the scrollbar visible.
 3. Click in a part of the scrollbar that is not covered by the scrollbar thumb and keep the mouse button pressed.
 4. Quickly drag up and down, quickly enough that you'd get checkerboarding if scrollbar dragging were APZ-ified.

Observe how the page wildly jumps up and down during the drag, until you pause your mouse motion and move more slowly.

It looks like we get into this state more easily on pages that are slower to draw.

attachment 8682494 [details] works well as a testcase.

I'm going to look for a regression window.
See Also: → 1256340
OS: Unspecified → Mac OS X
Hardware: Unspecified → All
Version: Trunk → 48 Branch
Pretty sure "scroll to click" isn't a thing :)
Summary: Scrollbar dragging with scroll-to-click goes crazy and oscillates between the top and the bottom end of the scrollbar → Scrollbar dragging with click-to-scroll goes crazy and oscillates between the top and the bottom end of the scrollbar
"scroll to click" in the sense of "please scroll to the click position" :)
https://dxr.mozilla.org/mozilla-central/source/layout/xul/nsSliderFrame.cpp#662
Oh, my bad. I read it as "scroll to cause clicking", and thought "scroll" and "click" were accidentally mixed up :)
Summary: Scrollbar dragging with click-to-scroll goes crazy and oscillates between the top and the bottom end of the scrollbar → Scrollbar dragging with scroll-to-click goes crazy and oscillates between the top and the bottom end of the scrollbar
Component: Layout → Panning and Zooming
Whiteboard: [gfx-noted]
I can repro on Nightly still, looking into this.
Assignee: nobody → bugmail.mozilla
Bug 1242690 had a bunch of code that skipped untransforming mouse events if they were part of a drag block that started on a scrollbar thumb. The reason for this is that the scrollbar thumb exists "outside" of the scrollable area of the page, and doing an untransform on those mouse events resulted in the oscillation in bug 1244549.

That fix was incomplete, because it targetted the scrollthumb only, when in fact it should target the entire scrollbar (including the scrolltrack) for the same reason. Doing that fixes this bug, and I have a WIP for that.

However, looking at bug 1259781, I suspect the mouse events for autoscroll also need to skip being untransformed, because it's the same underlying concept. That is, if mouse movements are directly driving a change in the scroll position on the main thread, then those mouse movements are interpreted relative to the screen and not relative to the content, and they shouldn't be untransformed.

I'm trying to come up with a cleaner model to handle all this, will think about it a bit more.
Attached patch WIP (obsolete) — Splinter Review
This is the WIP. Turns out there's already a "mIsScrollbarContainer" flag on layers which is not being propagated across IPC so I fixed that and used it.
I think the "cleaner model" to handle all this dives into the murky world of resolutions and layout-side untransforms, fixing the TODO at [1] and bug 1224748 along the way. It's also going to be a big complicated change with a lot of risk. I'll pursue it in bug 1259781 but I'd like to land the fix I came up with for this bug. I think this patch is something we'll need anyway, and it fixes the user-visible bug just fine. Bug 1259781 is much harder to notice/reproduce and so we can take some time to fix it properly with the big complicated change.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/util/APZCCallbackHelper.cpp?rev=e5232739c23c#461
Comment on attachment 8741150 [details] [diff] [review]
WIP

Will replace this with cleaned up MozReviews
Attachment #8741150 - Attachment is obsolete: true
Attachment #8741507 - Flags: review?(rbarker) → review+
Comment on attachment 8741507 [details]
MozReview Request: Bug 1258851 - Update HitTestingTreeNode::IsScrollbarNode to include the scrollbar track layers. r?rbarker

https://reviewboard.mozilla.org/r/46537/#review43149
Comment on attachment 8741506 [details]
MozReview Request: Bug 1258851 - Propagate the isScrollbarContainer layer flag to the compositor. r?mattwoodrow

https://reviewboard.mozilla.org/r/46535/#review43215
Attachment #8741506 - Flags: review?(matt.woodrow) → review+
https://hg.mozilla.org/mozilla-central/rev/96115b975df4
https://hg.mozilla.org/mozilla-central/rev/3afbfb3631bd
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1265806
Depends on: 1266176
You need to log in before you can comment on or make changes to this bug.