| Summary: | Calling event.preventDefault() on mouse events messes up scroll state in presence of iframes | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Zach <zachlloyd> | ||||||||||||
| Component: | UI Events | Assignee: | Nate Chapin <japhet> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | ap, dglazkov, gregsimon, rniwa, webkit.review.bot | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Attachments: |
|
||||||||||||||
Created attachment 123651 [details]
patch
I think this was a corner case missed in trac.webkit.org/changeset/40845
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. Created attachment 123757 [details]
patch with updated test
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. (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. Created attachment 123761 [details]
Resolve darin's comments
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 zacklloyd@google.com. 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. 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 Nate, could you land the patch manually or fix the test? Created attachment 124959 [details]
Patch for landing
Comment on attachment 124959 [details] Patch for landing Clearing flags on attachment: 124959 Committed r106476: <http://trac.webkit.org/changeset/106476> All reviewed patches have been landed. Closing bug. |
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.