Users can adjust their system preference for how secondary click is interpreted by the operating system. WebKit is improperly hard-coding the button number for these mouse events, resulting in improper behavior.
<rdar://problem/20437483>
Created attachment 250222 [details] Patch
Created attachment 250240 [details] Patch (Corrects WK1 and WK2 cases)
Created attachment 250267 [details] Patch (v3: iOS Fixes)
Comment on attachment 250267 [details] Patch (v3: iOS Fixes) I don't think this is the right solution. You'll note that Ctrl-Click hands the DOM a "left mouse down", so clearly context menus do not require a "right mouse button" click. And, we should lie and re-map what AppKit says about which button went down. So, please find a solution that doesn't involve remapping the buttons.
Created attachment 250306 [details] Patch
Attachment 250306 [details] did not pass style-queue: ERROR: Source/WebCore/platform/PlatformMouseEvent.h:75: Wrong number of spaces before statement. (expected: 31) [whitespace/indent] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 250306 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250306&action=review > Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:1268 > +bool WebPage::shouldBeHandledAsContextClick(const PlatformMouseEvent& event) Can this be static and can it live in WebEventFactory, right near typeForEvent? > Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:1271 > + if (event.button() == WebCore::LeftButton && event.ctrlKey()) > + return true; Can we make sure that event.menuTypeForEvent is NSMenuTypeContextMenu and get rid of this logic?
Created attachment 250313 [details] Patch
Attachment 250313 [details] did not pass style-queue: ERROR: Source/WebCore/platform/PlatformMouseEvent.h:75: Wrong number of spaces before statement. (expected: 31) [whitespace/indent] [4] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 250334 [details] Patch
Attachment 250334 [details] did not pass style-queue: ERROR: Source/WebCore/platform/PlatformMouseEvent.h:75: Wrong number of spaces before statement. (expected: 31) [whitespace/indent] [4] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 250385 [details] Patch
Attachment 250385 [details] did not pass style-queue: ERROR: Tools/TestWebKitAPI/Tests/mac/MenuTypesForMouseEvents.mm:46: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Tools/TestWebKitAPI/Tests/mac/MenuTypesForMouseEvents.mm:47: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Tools/TestWebKitAPI/Tests/mac/MenuTypesForMouseEvents.mm:48: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Tools/TestWebKitAPI/Tests/mac/MenuTypesForMouseEvents.mm:50: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Tools/TestWebKitAPI/Tests/mac/MenuTypesForMouseEvents.mm:51: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Tools/TestWebKitAPI/Tests/mac/MenuTypesForMouseEvents.mm:52: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/PlatformMouseEvent.h:75: Wrong number of spaces before statement. (expected: 31) [whitespace/indent] [4] ERROR: Tools/TestWebKitAPI/mac/PlatformWebViewMac.mm:230: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Tools/TestWebKitAPI/mac/PlatformWebViewMac.mm:231: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Tools/TestWebKitAPI/mac/PlatformWebViewMac.mm:232: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Tools/TestWebKitAPI/mac/PlatformWebViewMac.mm:234: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Tools/TestWebKitAPI/mac/PlatformWebViewMac.mm:235: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Tools/TestWebKitAPI/mac/PlatformWebViewMac.mm:236: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 13 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 250385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250385&action=review > Tools/TestWebKitAPI/Tests/WebKit2/MenuTypesForMouseEvents.cpp:72 > +TEST(WebKit2, NormalLeftClick) these should have better names. The concatenated name WebKit2.NormalLeftClick will show up as the name of the test in the test runner. Maybe add something like NormalLeftClickMenuType or something? > Tools/TestWebKitAPI/Tests/mac/MenuTypesForMouseEvents.mm:49 > + windowNumber:[[webView.get() window] windowNumber] No .get()
Committed r182581: <http://trac.webkit.org/changeset/182581>
The new API tests fail everywhere: https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK1%20%28Tests%29/builds/4301/steps/run-api-tests/logs/stdio /Volumes/Data/slave/yosemite-release/build/Tools/TestWebKitAPI/Tests/mac/MenuTypesForMouseEvents.mm:59 Value of: pme.menuTypeForEvent() Actual: 0 Expected: expectedMenu Which is: 1 Should we roll out?
(In reply to comment #17) > The new API tests fail everywhere: > https://build.webkit.org/builders/ > Apple%20Yosemite%20Release%20WK1%20%28Tests%29/builds/4301/steps/run-api- > tests/logs/stdio > > /Volumes/Data/slave/yosemite-release/build/Tools/TestWebKitAPI/Tests/mac/ > MenuTypesForMouseEvents.mm:59 > Value of: pme.menuTypeForEvent() > Actual: 0 > Expected: expectedMenu > Which is: 1 > > Should we roll out? I'll check in a fix. This is apparently a system preference, which I didn't notice when I tested locally.
Attempted fix checked in as r182584: Committed r182584: <http://trac.webkit.org/changeset/182584>
(In reply to comment #19) > Attempted fix checked in as r182584: > > Committed r182584: <http://trac.webkit.org/changeset/182584> Normally we set up consistent user defaults for testing so that the tests pass no matter what the global system preferences are. Can’t we do that here?
We certainly can't expect one who sees a failing test to look for comments in the code. The tests should just pass unconditionally.
(In reply to comment #21) > We certainly can't expect one who sees a failing test to look for comments > in the code. The tests should just pass unconditionally. It turns out that this was not related to system preferences; it was an improper bitwise comparison. Interesting that the EWS did not catch this. Does EWS not run the API tests? Fix committed in r182589 <https://trac.webkit.org/changeset/182589>.
> Does EWS not run the API tests? It does not.