Bug 138400 – Disable interaction with action menu page previews

• Component: WebKit2
• Status: RESOLVED FIXED

[Conrad Shultz]

Previews should ignore all mouse events except scrolling.

[Radar WebKit Bug Importer]

<rdar://problem/18876437>

[Conrad Shultz]

Created attachment 240996 [details]
Patch

[Anders Carlsson]

Comment on attachment 240996 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240996&action=review

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1211
> -    if ([self shouldIgnoreMouseEvents])
> +    if ([self _shouldIgnoreWheelEvents])

What does this do if _ignoreNonWheelMouseEvents returns false but shouldIgnoreMouseEvents returns true?

[Conrad Shultz]

(In reply to comment #3)
> Comment on attachment 240996 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240996&action=review
> 
> > Source/WebKit2/UIProcess/API/mac/WKView.mm:1211
> > -    if ([self shouldIgnoreMouseEvents])
> > +    if ([self _shouldIgnoreWheelEvents])
> 
> What does this do if _ignoreNonWheelMouseEvents returns false but
> shouldIgnoreMouseEvents returns true?

-_shouldIgnoreWheelEvents contains the same logic that used to be in -shouldIgnoreMouseEvents.

In the new implementation, the only way the above condition can occur is if have a thumbnail view, in which case -_shouldIgnoreWheelEvents will also return true.

[Anders Carlsson]

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 240996 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=240996&action=review
> > 
> > > Source/WebKit2/UIProcess/API/mac/WKView.mm:1211
> > > -    if ([self shouldIgnoreMouseEvents])
> > > +    if ([self _shouldIgnoreWheelEvents])
> > 
> > What does this do if _ignoreNonWheelMouseEvents returns false but
> > shouldIgnoreMouseEvents returns true?
> 
> -_shouldIgnoreWheelEvents contains the same logic that used to be in
> -shouldIgnoreMouseEvents.
> 
> In the new implementation, the only way the above condition can occur is if
> have a thumbnail view, in which case -_shouldIgnoreWheelEvents will also
> return true.

Does that mean that an existing WKView subclass who overrides shouldIgnoreMouseEvents and returns true will stop working?

[Conrad Shultz]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Comment on attachment 240996 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=240996&action=review
> > > 
> > > > Source/WebKit2/UIProcess/API/mac/WKView.mm:1211
> > > > -    if ([self shouldIgnoreMouseEvents])
> > > > +    if ([self _shouldIgnoreWheelEvents])
> > > 
> > > What does this do if _ignoreNonWheelMouseEvents returns false but
> > > shouldIgnoreMouseEvents returns true?
> > 
> > -_shouldIgnoreWheelEvents contains the same logic that used to be in
> > -shouldIgnoreMouseEvents.
> > 
> > In the new implementation, the only way the above condition can occur is if
> > have a thumbnail view, in which case -_shouldIgnoreWheelEvents will also
> > return true.
> 
> Does that mean that an existing WKView subclass who overrides
> shouldIgnoreMouseEvents and returns true will stop working?

In spite of the non-prefixed method name, -shouldIgnoreMouseEvents is purely internal and shouldn't be overridden by subclasses.

[Anders Carlsson]

Comment on attachment 240996 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240996&action=review

Patch looks good other than the declaration being in WKWebViewPrivate instead of WKWebViewinternal.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:157
> +@property (nonatomic, setter=_setIgnoresNonWheelMouseEvents:) BOOL _ignoresNonWheelMouseEvents;

If this is only used internally, it should go into WKWebViewInternal instead.

>>>>> Source/WebKit2/UIProcess/API/mac/WKView.mm:1211
>>>>> +    if ([self _shouldIgnoreWheelEvents])
>>>> 
>>>> What does this do if _ignoreNonWheelMouseEvents returns false but shouldIgnoreMouseEvents returns true?
>>> 
>>> -_shouldIgnoreWheelEvents contains the same logic that used to be in -shouldIgnoreMouseEvents.
>>> 
>>> In the new implementation, the only way the above condition can occur is if have a thumbnail view, in which case -_shouldIgnoreWheelEvents will also return true.
>> 
>> Does that mean that an existing WKView subclass who overrides shouldIgnoreMouseEvents and returns true will stop working?
> 
> In spite of the non-prefixed method name, -shouldIgnoreMouseEvents is purely internal and shouldn't be overridden by subclasses.

Fair enough, let's hope nobody replies on it.

[Conrad Shultz]

(In reply to comment #7)
> Comment on attachment 240996 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240996&action=review
> 
> Patch looks good other than the declaration being in WKWebViewPrivate
> instead of WKWebViewinternal.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:157
> > +@property (nonatomic, setter=_setIgnoresNonWheelMouseEvents:) BOOL _ignoresNonWheelMouseEvents;
> 
> If this is only used internally, it should go into WKWebViewInternal instead.

Thanks, will post a new patch that fixes this.

[Conrad Shultz]

Created attachment 241040 [details]
Patch

[Anders Carlsson]

Comment on attachment 241040 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241040&action=review

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:103
> +    [webView _setIgnoresNonWheelMouseEvents:YES];

This should use property syntax.

[Conrad Shultz]

(In reply to comment #10)
> Comment on attachment 241040 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241040&action=review
> 
> > Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:103
> > +    [webView _setIgnoresNonWheelMouseEvents:YES];
> 
> This should use property syntax.

That should actually be using a RetainPtr; will post a patch that addresses this.

[Conrad Shultz]

Created attachment 241043 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 241043 [details]
Patch

Clearing flags on attachment: 241043

Committed r175630: <http://trac.webkit.org/changeset/175630>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.