Summary: | onClick does not function with <select> elements in WebKit2 | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jon Lee <jonlee> | ||||||||||||||||||||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Major | CC: | ap, jonlee | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||||||
Attachments: |
|
Description
Jon Lee
2011-04-05 16:48:42 PDT
Created attachment 88727 [details]
Patch
Comment on attachment 88727 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88727&action=review > Source/WebKit2/UIProcess/API/mac/WKView.mm:946 > +- (void)Selector:(NSEvent *)theEvent \ > +{ \ > +NativeWebMouseEvent webEvent(theEvent, self); \ > +_data->_page->handleMouseEvent(webEvent); \ > +} I think we indent the body of multi-line macros. > Source/WebKit2/UIProcess/WebPageProxy.cpp:782 > + // (bug #57904) we need to keep track of the mouse down event in the case where we Please use full URLs when linking to bugs, that makes it easier to open them. Also use a capital letter in the beginning of the sentence. > Source/WebKit2/UIProcess/WebPageProxy.cpp:785 > + // when the mouse up message is received from WebProcess Period at the end :) > Source/WebKit2/UIProcess/WebPageProxy.cpp:2146 > +NativeWebMouseEvent* WebPageProxy::currentMouseDownEvent() Maybe this should be called "currentlyProcessedMouseDownEvent" or something along those lines, to indicate that this is the event that's been sent to the web process. > Source/WebKit2/UIProcess/mac/WebPopupMenuProxyMac.mm:131 > + // 57904: This code is adopted from EventHandler::sendFakeEventsAfterWidgetTracking() Bug URL. > Source/WebKit2/UIProcess/mac/WebPopupMenuProxyMac.mm:135 > + NSEvent *fakeEvent = nil; No need to initialize fakeEvent to nil. You can just initialize it with the created event. > Source/WebKit2/UIProcess/mac/WebPopupMenuProxyMac.mm:148 > + location:[[m_client->currentMouseDownEvent()->nativeView() window] convertScreenToBase:[NSEvent mouseLocation]] Do we really need to store the native view in the event? Can't we just get the window from the WKView? > Source/WebKit2/UIProcess/win/WebPopupMenuProxyWin.cpp:334 > + const MSG* initiatingWinEvent = m_client->currentMouseDownEvent()->nativeEvent(); > + MSG fakeEvent = *initiatingWinEvent; > + fakeEvent.message = WM_LBUTTONUP; > + ::DispatchMessage(&fakeEvent); This could use a comment. Also, is DispatchMessage the right event here or should we use ::PostMessage instead? Created attachment 88890 [details]
Patch
Made adjustments based on anderca's suggestions.
Created attachment 89021 [details]
Patch
Fixing windows build error
Comment on attachment 89021 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89021&action=review The indentation in WebPopupMenuProxyMac.mm is messed up. > Source/WebKit2/UIProcess/mac/WebPopupMenuProxyMac.mm:127 > + if (m_client) This could be an early return. > Source/WebKit2/UIProcess/mac/WebPopupMenuProxyMac.mm:132 > + NSEvent* initiatingNSEvent = m_client->currentlyProcessedMouseDownEvent()->nativeEvent(); I think you need to null check the event before getting the native event. > Source/WebKit2/UIProcess/win/WebPopupMenuProxyWin.cpp:328 > + if (m_client) { Early return. > Source/WebKit2/UIProcess/win/WebPopupMenuProxyWin.cpp:339 > + const MSG* initiatingWinEvent = m_client->currentlyProcessedMouseDownEvent()->nativeEvent(); Null check. Comment on attachment 89021 [details]
Patch
Whoops, I meant to r- this.
Created attachment 89078 [details]
Patch
early returns added, format fixed in ....Mac.mm
Comment on attachment 89078 [details]
Patch
There are a lot of unneeded whitespace changes in WebPopupMenuProxyMac.mm
Also, I think the currently processed mouse down event needs to be cleared out if the web process crashes (you can do this in WebPageProxy::processDidCrash()
Created attachment 89107 [details]
Patch
Clearing out cached events, including mouse down (relevant to this bug), and also mouse move and wheel.
Comment on attachment 89107 [details] Patch Rejecting attachment 89107 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'apply-..." exit_code: 2 Last 500 characters of output: 1 line). patching file Source/WebKit2/UIProcess/WebPopupMenuProxy.h patching file Source/WebKit2/UIProcess/mac/WebPopupMenuProxyMac.mm patching file Source/WebKit2/UIProcess/win/WebPopupMenuProxyWin.cpp patching file Source/WebKit2/UIProcess/win/WebView.cpp patching file Source/WebKit2/WebKit2.xcodeproj/project.pbxproj patching file Source/WebKit2/win/WebKit2.vcproj Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Anders Carlsson', u'--..." exit_code: 1 Full output: http://queues.webkit.org/results/8404382 Created attachment 89474 [details]
Patch
Trying again-- same changes, on top of newer code.
Comment on attachment 89474 [details] Patch Rejecting attachment 89474 [details] from review queue. jonlee@apple.com does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights. Comment on attachment 89474 [details] Patch Rejecting attachment 89474 [details] from commit-queue. jonlee@apple.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights. Comment on attachment 89474 [details] Patch Rejecting attachment 89474 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'apply-..." exit_code: 2 Last 500 characters of output: eProxy.h patching file Source/WebKit2/UIProcess/WebPopupMenuProxy.h patching file Source/WebKit2/UIProcess/mac/WebPopupMenuProxyMac.mm patching file Source/WebKit2/UIProcess/win/WebPopupMenuProxyWin.cpp patching file Source/WebKit2/UIProcess/win/WebView.cpp patching file Source/WebKit2/WebKit2.xcodeproj/project.pbxproj patching file Source/WebKit2/win/WebKit2.vcproj Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Anders Carlsson', u'--..." exit_code: 1 Full output: http://queues.webkit.org/results/8403570 Created attachment 89670 [details]
Patch
Created attachment 89685 [details]
Patch
Trying to fold changes into Qt.
Created attachment 89695 [details]
Patch
Comment on attachment 89695 [details]
Patch
Signing off on the updates since the last version.
Comment on attachment 89695 [details]
Patch
Signing off on the updates since the last version.
Created attachment 89721 [details]
Patch
Comment on attachment 89721 [details] Patch Rejecting attachment 89721 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'land-a..." exit_code: 2 Last 500 characters of output: World.cpp M Source/WebCore/html/HTMLCanvasElement.cpp r83990 = 52d4a4ddb5bdfde702f42fbe11d1a2d0501d21b8 (refs/remotes/trunk) M Source/WebCore/WebCore.exp.in M Source/WebCore/dom/DocumentMarkerController.h M Source/WebCore/dom/DocumentMarkerController.cpp M Source/WebCore/dom/DocumentMarker.h M Source/WebCore/ChangeLog r83991 = 3e9aec703b851aa84338cad59d52e2be93b0c66e (refs/remotes/trunk) First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/trunk. Full output: http://queues.webkit.org/results/8457195 Comment on attachment 89107 [details] Patch Cleared Anders Carlsson's review+ from obsolete attachment 89107 [details] so that this bug does not appear in http://webkit.org/pending-commit. The patch was not landing for some reason, so Anders landed it for me, but the bug never got closed. Setting resolution to resolved/fixed. |