[Webkit-unassigned] [Bug 200398] Add an SPI to suppress all WKWebView interactions except scrolling or zooming

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=200398

--- Comment #8 from Luming Yin <luming_yin at apple.com> ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190804/04a60329/attachment.html>