Bug 73097 – Calling event.preventDefault() on mouse events messes up scroll state in presence of iframes

• Component: UI Events
• Status: RESOLVED FIXED

[Zach]

Created attachment 116546 [details]
Repro case for the bug.

To repro the bug:

1) unpack the attacked zip and load iframe_scrolling_bug.html
2) Click on the scrollhandle of the div within the iframe and without releasing the button, move the mouse pointer to the outer frame.
3) Release the mouse button.
4) Now move the mouse pointer back over the scrollbar within the frame.
5) Notice that even without clicking again the scrollbar scrolls the inner content as you move the mouse over the scrollbar.  It is somehow "stuck" in the scrolling state.

The bug is triggered because of the cal to evt.preventDefault() on the initial mousedown on the div.  If you unclick the option for preventing default, the bug does not occur.

[Nate Chapin]

Created attachment 123651 [details]
patch

I think this was a corner case missed in trac.webkit.org/changeset/40845

[Ryosuke Niwa]

Comment on attachment 123651 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123651&action=review

> LayoutTests/fast/events/scroll-div-with-prevent-default-in-subframe-expected.txt:3
> +CONSOLE MESSAGE: line 14: Scroll after move within scrollbar: 221
> +CONSOLE MESSAGE: line 20: Scroll after move to main frame: 221
> +CONSOLE MESSAGE: line 23: Scroll after mouseup and return to subframe: 221

Instead of printing these numbers, can we compare them by script and print PASS? We can print these numbers only when they don't match so that the debugging will be easier when it fails.

[Nate Chapin]

Created attachment 123757 [details]
patch with updated test

[Darin Adler]

Comment on attachment 123757 [details]
patch with updated test

View in context: https://bugs.webkit.org/attachment.cgi?id=123757&action=review

> LayoutTests/fast/events/scroll-div-with-prevent-default-in-subframe.html:24
> +        var result = (scrollAfterSetup == scrollAfterMoveToMainFrame) && (scrollAfterSetup == scrollAfterReturnToSubframe) ? "PASS" : "FAIL";

Ryosuke suggested printing the numbers when the test fails and leaving them out when the test passes.

Looks like you opted to always leave them out. Reason?

> LayoutTests/fast/events/scroll-div-with-prevent-default-in-subframe.html:25
> +        document.getElementById("console").appen

Looks like this line of code isn’t all there.

[Nate Chapin]

(In reply to comment #4)
> (From update of attachment 123757 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123757&action=review
> 
> > LayoutTests/fast/events/scroll-div-with-prevent-default-in-subframe.html:24
> > +        var result = (scrollAfterSetup == scrollAfterMoveToMainFrame) && (scrollAfterSetup == scrollAfterReturnToSubframe) ? "PASS" : "FAIL";
> 
> Ryosuke suggested printing the numbers when the test fails and leaving them out when the test passes.
> 
> Looks like you opted to always leave them out. Reason?

I misread his comment, will fix.

> 
> > LayoutTests/fast/events/scroll-div-with-prevent-default-in-subframe.html:25
> > +        document.getElementById("console").appen
> 
> Looks like this line of code isn’t all there.

No idea what happened there.

[Nate Chapin]

Created attachment 123761 [details]
Resolve darin's comments

[Ryosuke Niwa]

Comment on attachment 123757 [details]
patch with updated test

View in context: https://bugs.webkit.org/attachment.cgi?id=123757&action=review

>>> LayoutTests/fast/events/scroll-div-with-prevent-default-in-subframe.html:25
>>> +        document.getElementById("console").appen
>> 
>> Looks like this line of code isn’t all there.
> 
> No idea what happened there.

appen??

> LayoutTests/fast/events/scroll-div-with-prevent-default-in-subframe.html:42
> +Per https://bugs.webkit.org/show_bug.cgi?id=73097, because the div with the scrollbar had a mousedown event that called preventDefault(),
> +the mouse moves would not properly be handled by the scrollbar. We pass if the div's scrollTop property is the same after all 3 steps.

This description should probably be moved to the WebCore change log entry.

> LayoutTests/ChangeLog:4
> +        Test for https://bugs.webkit.org/show_bug.cgi?id=73097.
> +        Test adapted from repro case provided by [e-mail].

These two line should be:
Bug summary
Bug URL

> Source/WebCore/ChangeLog:6
> +        preventDefault() in a mousedown in a subframe should not
> +        prevent the scrollbar from handling mouse movements if the
> +        cursor leaves the subframe.
> +        https://bugs.webkit.org/show_bug.cgi?id=73097

Ditto about having a bug summary followed by a bug url. The description should go below the "reviewed by" followed by a blank line.

> Source/WebCore/ChangeLog:9
> +

Could you also explain why/how your change fixes the problem?

> Source/WebCore/page/EventHandler.cpp:1464
> +    m_capturesDragging = !swallowEvent || mev.scrollbar();

It seems somewhat arbitrary to reuse m_captureDragging to route events for scrollbars. It might be better to add an inline function, a boolean with a name, or a comment that describes why we need to set m_capturesDragging to this value.

[WebKit Review Bot]

Comment on attachment 123761 [details]
Resolve darin's comments

Attachment 123761 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11294014

New failing tests:
fast/events/scroll-div-with-prevent-default-in-subframe.html

[Ryosuke Niwa]

Nate, could you land the patch manually or fix the test?

[Nate Chapin]

Created attachment 124959 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 124959 [details]
Patch for landing

Clearing flags on attachment: 124959

Committed r106476: <http://trac.webkit.org/changeset/106476>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.