Bug 200398 – Add an SPI to suppress all WKWebView interactions except scrolling or zooming

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit2

[Luming Yin]

<rdar://problem/53871117>

[Luming Yin]

Created attachment 375450 [details]
Patch

[Wenson Hsieh]

Comment on attachment 375450 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3995
> +        [self _startSuppressingSelectionAssistantForReason:WebKit::InteractionIsHappening];

Nit - I feel like this might warrant its own separate suppression reason (InteractionIsHappening is about the new context menu for peek).

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4003
> +        [self _didChangeWebViewEditability];

It seems a bit odd to call -_didChangeWebViewEditability here, since web view edibility didn’t really change. Perhaps we could factor out the logic for resetting the double tap gesture into a separate helper method, and then call it from both places?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4004
> +        _webView._dragInteractionPolicy = _WKDragInteractionPolicyDefault;

It seems like this would enable the drag interaction after interaction suppression ends, in the case where a client sets `_webView._dragInteractionPolicy = _WKDragInteractionPolicyAlwaysDisable;` but would then use this SPI to temporarily suppress interactions. We’d probably want to restore _dragInteractionPolicy to its value prior to this call.

[Luming Yin]

Created attachment 375464 [details]
Patch

[Luming Yin]

(In reply to Wenson Hsieh from comment #2)
> Comment on attachment 375450 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375450&action=review
> 
> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3995
> > +        [self _startSuppressingSelectionAssistantForReason:WebKit::InteractionIsHappening];
> 
> Nit - I feel like this might warrant its own separate suppression reason
> (InteractionIsHappening is about the new context menu for peek).
Thanks for the context - I didn't know InteractionIsHappening was only for peek context menus. Updated my patch to introduce a new suppression reason OnlyScrollOrZoomIsAllowed.

> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4003
> > +        [self _didChangeWebViewEditability];
> 
> It seems a bit odd to call -_didChangeWebViewEditability here, since web
> view edibility didn’t really change. Perhaps we could factor out the logic
> for resetting the double tap gesture into a separate helper method, and then
> call it from both places?
Factored out into -_updateTwoFingerSingleTapGestureRecognizer to be called from both places.

> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4004
> > +        _webView._dragInteractionPolicy = _WKDragInteractionPolicyDefault;
> 
> It seems like this would enable the drag interaction after interaction
> suppression ends, in the case where a client sets
> `_webView._dragInteractionPolicy = _WKDragInteractionPolicyAlwaysDisable;`
> but would then use this SPI to temporarily suppress interactions. We’d
> probably want to restore _dragInteractionPolicy to its value prior to this
> call.
Updated the patch to cache and restore the prior _dragInteractionPolicy in _dragInteractionPolicyPriorToSuppression.

[Geoffrey Garen]

A similar API that suppressed loading proved super easy to use wrong, and we ended up with many cases when someone called _suppress... but did not balance it with a call to _restore...., and then everything mysteriously stopped working.

Is there something you can do to make this API harder to use wrong?

Some suggestions to consider:

(1) Add release logging that tracks this state, so we can diagnose bugs in the field;

(2) Instead of accepting a BOOL argument, return a token object, which offers a -cancel method, and which also calls -cancel automatically in its -dealloc method;

(3) Throw an exception if someone invokes this API while a token is still outstanding;

(4) Add a watchdog timer that will unconditionally cancel a token after a certain amount of time;

(5) Don't use a special mode on the content view at all, and instead install another view on top, whose job is to intercept events and only forward scroll and zoom events.

Also, we usually require automated API tests for new APIs like this.

[Wenson Hsieh]

(In reply to Geoffrey Garen from comment #5)
> A similar API that suppressed loading proved super easy to use wrong, and we
> ended up with many cases when someone called _suppress... but did not
> balance it with a call to _restore...., and then everything mysteriously
> stopped working.
> 
> Is there something you can do to make this API harder to use wrong?
> 
> Some suggestions to consider:
> 
> (1) Add release logging that tracks this state, so we can diagnose bugs in
> the field;

I agree that some carefully-chosen release logging would be beneficial here! However, I think the intent and implementation of this SPI is different than the more typical suppress-restore SPIs we have (such as _retainActiveFocusedState or WKDOMDocumentParserYieldToken).

It seems the SPI client invokes this when it wishes to begin temporarily begin suppressing certain types of interaction in the web view until the next user gesture — for instance, scrolling, pinching, tapping, or long pressing. When such a gesture happens, the completion handler passed in by the client is then invoked.

This means the call to -_restoreAllInteractions is actually optional, and only allows the client to stop suppressing interactions prematurely.

> 
> (2) Instead of accepting a BOOL argument, return a token object, which
> offers a -cancel method, and which also calls -cancel automatically in its
> -dealloc method;
> 
> (3) Throw an exception if someone invokes this API while a token is still
> outstanding;
> 
> (4) Add a watchdog timer that will unconditionally cancel a token after a
> certain amount of time;
> 
> (5) Don't use a special mode on the content view at all, and instead install
> another view on top, whose job is to intercept events and only forward
> scroll and zoom events.
> 
> Also, we usually require automated API tests for new APIs like this.

Yes, this should definitely be tested. I think an API test would be ideal, but extremely challenging (if not impossible) since TestWebKitAPI isn’t a UI app on iOS, so user interactions like taps or swipes can’t be simulated — at least, until webkit.org/b/175204 is resolved.

It might be easier to make a layout test for this, and introduce UIScriptController hooks and plumbing to make it possible to invoke this API from a layout test’s script, and subsequently get notified in the script when the completion handler is invoked.

[Darin Adler]

Comment on attachment 375464 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:388
> +    BOOL _suppressInteractions;

Our naming style, borrowed from Apple’s Cocoa style, is to never use a phrase that could be a verb for a data member or getter function, particularly one that has a boolean value. To follow that style we’d call this "should suppress interactions", "suppressing interactions", "is suppressing interactions", or "interactions suppressed", but not "suppress interactions".

Please consider changing the name.

[Luming Yin]

(In reply to Wenson Hsieh from comment #6)
> (In reply to Geoffrey Garen from comment #5)
> > A similar API that suppressed loading proved super easy to use wrong, and we
> > ended up with many cases when someone called _suppress... but did not
> > balance it with a call to _restore...., and then everything mysteriously
> > stopped working.
> > 
> > Is there something you can do to make this API harder to use wrong?
> > 
> > Some suggestions to consider:
> > 
> > (1) Add release logging that tracks this state, so we can diagnose bugs in
> > the field;
> 
> I agree that some carefully-chosen release logging would be beneficial here!
Also agreed. I'll add some release logging for this field.

> However, I think the intent and implementation of this SPI is different than
> the more typical suppress-restore SPIs we have (such as
> _retainActiveFocusedState or WKDOMDocumentParserYieldToken).
> 
> It seems the SPI client invokes this when it wishes to begin temporarily
> begin suppressing certain types of interaction in the web view until the
> next user gesture — for instance, scrolling, pinching, tapping, or long
> pressing. When such a gesture happens, the completion handler passed in by
> the client is then invoked.
> 
> This means the call to -_restoreAllInteractions is actually optional, and
> only allows the client to stop suppressing interactions prematurely.
Yes, a potential SPI client use case is <rdar://problem/52989609>.

> > 
> > (2) Instead of accepting a BOOL argument, return a token object, which
> > offers a -cancel method, and which also calls -cancel automatically in its
> > -dealloc method;
> > 
> > (3) Throw an exception if someone invokes this API while a token is still
> > outstanding;
> > 
> > (4) Add a watchdog timer that will unconditionally cancel a token after a
> > certain amount of time;

Adding to Wenson's comments, adding a watchdog timer may be unideal considering 
the use case of this SPI. Unconditionally cancelling the token will either lead 
to a client-side modal getting dismissed on its own without user interaction when 
the user may just be hesitating; or to cause both the modal to stay visible and 
the page to be fully interactive, which can be confusing.

> > (5) Don't use a special mode on the content view at all, and instead install
> > another view on top, whose job is to intercept events and only forward
> > scroll and zoom events.

Thanks for suggesting this! I looked into installing another passthrough view on top 
of the web view in the client app before making this WebKit patch (attachment in 
rdar://problem/52989609), but the implementation turned out to be unideal. However, 
I haven't looked into directly installing another view on top of the WKContentView 
in WebKit. I'll definitely investigate.

> > Also, we usually require automated API tests for new APIs like this.
> 
> Yes, this should definitely be tested. I think an API test would be ideal,
> but extremely challenging (if not impossible) since TestWebKitAPI isn’t a UI
> app on iOS, so user interactions like taps or swipes can’t be simulated — at
> least, until webkit.org/b/175204 is resolved.
> 
> It might be easier to make a layout test for this, and introduce
> UIScriptController hooks and plumbing to make it possible to invoke this API
> from a layout test’s script, and subsequently get notified in the script
> when the completion handler is invoked.

I agree that this should be tested. I'll look into adding a layout test for this.

(In reply to Darin Adler from comment #7)
> Comment on attachment 375464 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375464&action=review
> 
> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:388
> > +    BOOL _suppressInteractions;
> 
> Our naming style, borrowed from Apple’s Cocoa style, is to never use a
> phrase that could be a verb for a data member or getter function,
> particularly one that has a boolean value. To follow that style we’d call
> this "should suppress interactions", "suppressing interactions", "is
> suppressing interactions", or "interactions suppressed", but not "suppress
> interactions".
> 
> Please consider changing the name.
Thanks for noting our naming style and Cocoa conventions! I'll change the 
name into `isSuppressingInteractions`, and also remind myself of this for 
future patches.

[Geoffrey Garen]

> It seems the SPI client invokes this when it wishes to begin temporarily
> begin suppressing certain types of interaction in the web view until the
> next user gesture — for instance, scrolling, pinching, tapping, or long
> pressing. When such a gesture happens, the completion handler passed in by
> the client is then invoked.
> 
> This means the call to -_restoreAllInteractions is actually optional, and
> only allows the client to stop suppressing interactions prematurely.

I missed this detail the first time around. It's nice that the implementation automatically restores interaction upon the first interaction. That helps.

Even so, it looks like long press and context menu will not automatically restore? So there is still some risk here.

> > (2) Instead of accepting a BOOL argument, return a token object, which
> > offers a -cancel method, and which also calls -cancel automatically in its
> > -dealloc method;

Looking at the described used case, and at the implementation here, I do still think that a token object that held a -cancel method and a completionHandler would be a nice way to model this behavior. (Specifically, I would have WebKit hold a weak reference to the token, and the client hold a strong reference.)

This suppression is associated with a specific object in the client, so that object can own the token object.

[Luming Yin]

(In reply to Geoffrey Garen from comment #9)
> > It seems the SPI client invokes this when it wishes to begin temporarily
> > begin suppressing certain types of interaction in the web view until the
> > next user gesture — for instance, scrolling, pinching, tapping, or long
> > pressing. When such a gesture happens, the completion handler passed in by
> > the client is then invoked.
> > 
> > This means the call to -_restoreAllInteractions is actually optional, and
> > only allows the client to stop suppressing interactions prematurely.
> 
> I missed this detail the first time around. It's nice that the
> implementation automatically restores interaction upon the first
> interaction. That helps.
> 
> Even so, it looks like long press and context menu will not automatically
> restore? So there is still some risk here.
Long press and context menu will automatically restore if a tap, scroll or pinch is detected. Since 
any of the above would call -_setSuppressInteractionsExceptScrollingOrZooming:NO, 
_setSuppressInteractionsExceptScrollingOrZooming sets the ivar _suppressInteractions to be NO, and 
sets _webView.configuration._longPressActionsEnabled to YES. 

In other words, when long presses are suppressed, it would be picked up by _singleTapGestureRecognizer 
instead, which will restore everything.

> 
> > > (2) Instead of accepting a BOOL argument, return a token object, which
> > > offers a -cancel method, and which also calls -cancel automatically in its
> > > -dealloc method;
> 
> Looking at the described used case, and at the implementation here, I do
> still think that a token object that held a -cancel method and a
> completionHandler would be a nice way to model this behavior. (Specifically,
> I would have WebKit hold a weak reference to the token, and the client hold
> a strong reference.)
> 
> This suppression is associated with a specific object in the client, so that
> object can own the token object.
Makes sense, and thanks for the extra notes on object ownership. I'll look into returning a token 
object with a cancel method and a completionHandler instead.