WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
19033
Scrolling does not work when the mouse down is handled by a node
https://bugs.webkit.org/show_bug.cgi?id=19033
Summary
Scrolling does not work when the mouse down is handled by a node
Ananta
Reported
2008-05-13 11:19:20 PDT
Launch Safari 3.1 and navigate to
http://cn.msn.com
Wait for the page to load fully. Now click on the vertical/horizontal scrollbars. Observe that the page does not scroll. It does scroll with the mousewheel. Debugged this and found that the mouse down event is handled by the node returned as a result of the hit test. The hit test does not have a scrollbar associated with it. This causes the page to not scroll. The code should also check if there is a scrollbar under the mouse and pass it off if yes. The webkit code already does this in the case when the mouse down event is not handled. Will submit a patch for this.
Attachments
The patch containing the proposed fix for this issue.
(1.86 KB, patch)
2008-05-13 14:27 PDT
,
Ananta
no flags
Details
Formatted Diff
Diff
The patch containing the proposed fix for this issue.
(1.86 KB, patch)
2008-05-13 14:30 PDT
,
Ananta
darin
: review-
Details
Formatted Diff
Diff
Reduced test case
(683 bytes, text/html)
2013-08-27 15:08 PDT
,
Steven Wittens
no flags
Details
Patch, no tests yet. For EWK
(5.48 KB, patch)
2016-03-04 09:52 PST
,
Antonio Gomes
tonikitoo
: review-
Details
Formatted Diff
Diff
Patch, no tests yet. For EWK (v2)
(5.49 KB, patch)
2016-03-04 10:21 PST
,
Antonio Gomes
no flags
Details
Formatted Diff
Diff
Patch with tests (v1)
(9.02 KB, patch)
2016-03-04 13:31 PST
,
Antonio Gomes
buildbot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from ews106 for mac-yosemite-wk2
(815.05 KB, application/zip)
2016-03-04 14:19 PST
,
Build Bot
no flags
Details
Patch with tests (v2)
(9.27 KB, patch)
2016-03-05 07:31 PST
,
Antonio Gomes
buildbot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from ews105 for mac-yosemite-wk2
(951.11 KB, application/zip)
2016-03-05 08:43 PST
,
Build Bot
no flags
Details
Patch with tests (v3)
(9.37 KB, patch)
2016-03-07 11:12 PST
,
Antonio Gomes
no flags
Details
Formatted Diff
Diff
Patch with tests (v3.1)
(9.25 KB, patch)
2016-03-08 07:45 PST
,
Antonio Gomes
simon.fraser
: review+
Details
Formatted Diff
Diff
Patch for landing.
(9.25 KB, patch)
2016-03-08 10:18 PST
,
Antonio Gomes
no flags
Details
Formatted Diff
Diff
Show Obsolete
(10)
View All
Add attachment
proposed patch, testcase, etc.
Ananta
Comment 1
2008-05-13 14:27:36 PDT
Created
attachment 21112
[details]
The patch containing the proposed fix for this issue.
Ananta
Comment 2
2008-05-13 14:29:37 PDT
Comment on
attachment 21112
[details]
The patch containing the proposed fix for this issue. The patch has tabs in it. Will submit a new patch for review.
Ananta
Comment 3
2008-05-13 14:30:06 PDT
Created
attachment 21113
[details]
The patch containing the proposed fix for this issue.
Darin Adler
Comment 4
2008-05-24 22:51:16 PDT
Comment on
attachment 21113
[details]
The patch containing the proposed fix for this issue. For this to get checked in, we'll need a test case demonstrating the problem, in the form of a regression test. I can't tell from reading the patch alone if this is correct. The test case will both help me understand that there truly is a bug here, and also is required as part of our "all bug fixes must have a regression test" policy. To simulate a mouse click, you can use the eventSender feature of the regression test engine. The test in fast/events/mousedown_in_scrollbar.html is an example of one that tests a mouse event in a scrollbar, and could be used as a starting point in making a test.
Eric Seidel (no email)
Comment 5
2008-11-11 16:23:27 PST
We still have this diff in our tree, and I'm not sure why.
http://www.corp.google.com/~dglazkov/merge/WebCore-page-EventHandler.cpp-before.html#difflib_chg_to58__1
Eric Seidel (no email)
Comment 6
2008-11-11 16:24:11 PST
My apologies. This is the external URL:
http://build.chromium.org/merge/WebCore-page-EventHandler.cpp-before.html#difflib_chg_to106__2
casey
Comment 7
2009-12-31 02:37:24 PST
jQuery UI Dialog is affected by this issue:
http://dev.jqueryui.com/ticket/4671
Antonio Gomes
Comment 8
2010-04-27 10:55:26 PDT
I think the change looks ok (I am not a reviewer) since MouseEventWithHitTestResults does not set "scrollbar" when hit-testing a frame scrollbar, but only in-frame scrollbars (like scrollable <div>, <textarea>, etc). (In reply to
comment #3
)
> Created an attachment (id=21113) [details] > The patch containing the proposed fix for this issue.
However I could not reproduce the problem on Safari (webkit - trunk) on Snow Leonard. Given PLATFORM "WIN XP", I think it is Windows specific. Could you confirm?
Antonio Gomes
Comment 9
2013-08-21 09:03:18 PDT
(In reply to
comment #0
)
> Launch Safari 3.1 and navigate to
http://cn.msn.com
> Wait for the page to load fully. > Now click on the vertical/horizontal scrollbars. > Observe that the page does not scroll. It does scroll > with the mousewheel.
Safari seems to scroll the page ok now. Please provide a (reduced) test case or a new URL that shows the bug or it is not actionable.
> Debugged this and found that the mouse down event is handled by the > node returned as a result of the hit test. The hit test does not have > a scrollbar associated with it. This causes the page to not scroll. > The code should also check if there is a scrollbar under the mouse and > pass it off if yes. The webkit code already does this in the case when > the mouse down event is not handled.
>
Steven Wittens
Comment 10
2013-08-27 15:08:57 PDT
Created
attachment 209805
[details]
Reduced test case This bug is occurring on
http://acko.net/
anywhere at the top. Debugging confirms a document mousedown handler fires when clicking the scrollbar. Emptying out the entire DOM does not fix this. This is on Mac OS X, both with and without permanent scrollbars enabled. When using temporary scrollbars, you can also click the Gear button in the top right through the scrollbar. I've attached a reduced test case. Note: 1) The scrollbar is unusable with the mouse 2) The grey div punches through the scrollbar for hit testing Bug appears on Safari 6.0.5, Chrome 29, Chrome 31.
Antonio Gomes
Comment 11
2016-03-03 11:58:50 PST
Reproduced on Linux (webkit-gtk) and Mac/Safari. Taking..
Antonio Gomes
Comment 12
2016-03-04 09:52:30 PST
Created
attachment 273008
[details]
Patch, no tests yet. For EWK
Chris Dumez
Comment 13
2016-03-04 09:59:20 PST
Comment on
attachment 273008
[details]
Patch, no tests yet. For EWK View in context:
https://bugs.webkit.org/attachment.cgi?id=273008&action=review
I am not familiar with this code but have a couple of minor comments.
> Source/WebCore/page/EventHandler.cpp:1580 > +Scrollbar* scrollbarForMouseEvent(const MouseEventWithHitTestResults mouseEvent, FrameView* view)
const MouseEventWithHitTestResults&
> Source/WebCore/page/EventHandler.cpp:1582 > + Scrollbar* scrollbar = nullptr;
Scrollbar* scrollbar = view ? view->scrollbarAtPoint(mouseEvent.event().position()) : nullptr;
Chris Dumez
Comment 14
2016-03-04 09:59:44 PST
Comment on
attachment 273008
[details]
Patch, no tests yet. For EWK View in context:
https://bugs.webkit.org/attachment.cgi?id=273008&action=review
>> Source/WebCore/page/EventHandler.cpp:1580 >> +Scrollbar* scrollbarForMouseEvent(const MouseEventWithHitTestResults mouseEvent, FrameView* view) > > const MouseEventWithHitTestResults&
also should be static.
Antonio Gomes
Comment 15
2016-03-04 10:21:33 PST
Created
attachment 273012
[details]
Patch, no tests yet. For EWK (v2)
WebKit Commit Bot
Comment 16
2016-03-04 10:23:35 PST
Attachment 273012
[details]
did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Antonio Gomes
Comment 17
2016-03-04 13:31:37 PST
Created
attachment 273032
[details]
Patch with tests (v1)
Build Bot
Comment 18
2016-03-04 14:19:07 PST
Comment on
attachment 273032
[details]
Patch with tests (v1)
Attachment 273032
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.webkit.org/results/923600
New failing tests: fast/events/prevent-default-prevents-interaction-with-scrollbars.html
Build Bot
Comment 19
2016-03-04 14:19:10 PST
Created
attachment 273040
[details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Antonio Gomes
Comment 20
2016-03-05 07:31:09 PST
Created
attachment 273084
[details]
Patch with tests (v2) Attempt to make the test run on EWK-Mac2.
Build Bot
Comment 21
2016-03-05 08:43:27 PST
Comment on
attachment 273084
[details]
Patch with tests (v2)
Attachment 273084
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.webkit.org/results/927157
New failing tests: fast/events/prevent-default-prevents-interaction-with-scrollbars.html
Build Bot
Comment 22
2016-03-05 08:43:30 PST
Created
attachment 273085
[details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Antonio Gomes
Comment 23
2016-03-07 11:12:40 PST
Created
attachment 273196
[details]
Patch with tests (v3)
Antonio Gomes
Comment 24
2016-03-08 07:32:56 PST
Adding Simon/Beth/Darin, which I believe are familiar with this code. This is a simple fix, that affect real world websites, and differ WebKit from Gecko/Blink these days.
Antonio Gomes
Comment 25
2016-03-08 07:45:24 PST
Created
attachment 273295
[details]
Patch with tests (v3.1) Rebased against ToT, fixed typos on WebCore/ChangeLog.
Darin Adler
Comment 26
2016-03-08 08:55:03 PST
Comment on
attachment 273295
[details]
Patch with tests (v3.1) View in context:
https://bugs.webkit.org/attachment.cgi?id=273295&action=review
> Source/WebCore/page/EventHandler.cpp:1585 > + Scrollbar* scrollbar = view ? view->scrollbarAtPoint(mouseEvent.event().position()) : nullptr; > + if (!scrollbar) > + scrollbar = mouseEvent.scrollbar(); > + return scrollbar;
Now that this is factored out into a function, it can be refactored to make the logic slightly clearer. Like this, perhaps. if (view) { if (auto* scrollbar = view->scrollbarAtPoint(mouseEvent.event().position())) return scrollbar; } return mouseEvent.scrollbar();
Antonio Gomes
Comment 27
2016-03-08 10:18:51 PST
Created
attachment 273303
[details]
Patch for landing.
Antonio Gomes
Comment 28
2016-03-08 10:19:17 PST
(In reply to
comment #26
)
> Comment on
attachment 273295
[details]
> Patch with tests (v3.1) > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=273295&action=review
> > > Source/WebCore/page/EventHandler.cpp:1585 > > + Scrollbar* scrollbar = view ? view->scrollbarAtPoint(mouseEvent.event().position()) : nullptr; > > + if (!scrollbar) > > + scrollbar = mouseEvent.scrollbar(); > > + return scrollbar; > > Now that this is factored out into a function, it can be refactored to make > the logic slightly clearer. Like this, perhaps. > > if (view) { > if (auto* scrollbar = > view->scrollbarAtPoint(mouseEvent.event().position())) > return scrollbar; > } > return mouseEvent.scrollbar();
Sounds good, done.
WebKit Commit Bot
Comment 29
2016-03-08 10:57:06 PST
The commit-queue encountered the following flaky tests while processing
attachment 273303
[details]
: http/tests/xmlhttprequest/workers/xmlhttprequest-file-not-found.html
bug 155176
(authors:
atwilson@chromium.org
and
levin@chromium.org
) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 30
2016-03-08 10:57:44 PST
Comment on
attachment 273303
[details]
Patch for landing. Clearing flags on attachment: 273303 Committed
r197784
: <
http://trac.webkit.org/changeset/197784
>
WebKit Commit Bot
Comment 31
2016-03-08 10:57:51 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug