Bug 143452 – [Mac] WebKit is not honoring operating system preferences for secondary click behavior

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

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.

[Brent Fulgham]

<rdar://problem/20437483>

[Brent Fulgham]

Created attachment 250222 [details]
Patch

[Brent Fulgham]

Created attachment 250240 [details]
Patch (Corrects WK1 and WK2 cases)

[Brent Fulgham]

Created attachment 250267 [details]
Patch (v3: iOS Fixes)

[Tim Horton]

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.

[Brent Fulgham]

Created attachment 250306 [details]
Patch

[WebKit Commit Bot]

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.

[Tim Horton]

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?

[Brent Fulgham]

Created attachment 250313 [details]
Patch

[WebKit Commit Bot]

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.

[Brent Fulgham]

Created attachment 250334 [details]
Patch

[WebKit Commit Bot]

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.

[Brent Fulgham]

Created attachment 250385 [details]
Patch

[WebKit Commit Bot]

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.

[Tim Horton]

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()

[Brent Fulgham]

Committed r182581: <http://trac.webkit.org/changeset/182581>

[Alexey Proskuryakov]

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?

[Brent Fulgham]

(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.

[Brent Fulgham]

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?

[Alexey Proskuryakov]

We certainly can't expect one who sees a failing test to look for comments in the code. The tests should just pass unconditionally.

[Brent Fulgham]

(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>.

[Alexey Proskuryakov]

> Does EWS not run the API tests?

It does not.