Bug 149567

Created attachment 261960 [details] Patch

Comment on attachment 261960 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=261960&action=review > Source/WebKit2/UIProcess/WebPageProxy.h:258 > + NodeAssistanceArguments(const AssistedNodeInformation& nodeInformation, bool userIsInteracting, bool blurPreviousNode, API::Object* userData) > + : m_nodeInformation(nodeInformation) > + , m_userIsInteracting(userIsInteracting) > + , m_blurPreviousNode(blurPreviousNode) > + , m_userData(userData) > + { > + } A struct does not need a constructor like this in modern C++. We can use aggregate initializer syntax instead. > Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:816 > + m_deferredNodeAssistanceArguments = std::make_unique<NodeAssistanceArguments>(information, userIsInteracting, blurPreviousNode, userDataObject); I believe the only change needed when we remove the constructor is to add braces here: std::make_unique<NodeAssistanceArguments>({ information, userIsInteracting, blurPreviousNode, userDataObject });

Comment on attachment 261960 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=261960&action=review Thanks for taking a look! I'll switch to using an aggregate initializer before landing. >> Source/WebKit2/UIProcess/WebPageProxy.h:258 >> + } > > A struct does not need a constructor like this in modern C++. We can use aggregate initializer syntax instead. Got it. I'll take out the constructor. >> Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:816 >> + m_deferredNodeAssistanceArguments = std::make_unique<NodeAssistanceArguments>(information, userIsInteracting, blurPreviousNode, userDataObject); > > I believe the only change needed when we remove the constructor is to add braces here: > > std::make_unique<NodeAssistanceArguments>({ information, userIsInteracting, blurPreviousNode, userDataObject }); Got it. I'll keep this in mind.

Committed r190278: <http://trac.webkit.org/changeset/190278>

Comment on attachment 261960 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=261960&action=review > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:248 > +@property (nonatomic) BOOL canAssistOnProgrammaticFocus; This API declaration is missing availability information. It’s surprising that this API uses the term “assist”. Is this something that’s used anywhere else in WebKit or UIKit API? Have you considered other ways to expose this ability to clients? For example, calling the delegate when an element is focused and asking it to make a decision seems like it would afford more flexibility.

<rdar://problem/22878323>

Comment on attachment 261960 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=261960&action=review >> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:248 >> +@property (nonatomic) BOOL canAssistOnProgrammaticFocus; > > This API declaration is missing availability information. > > It’s surprising that this API uses the term “assist”. Is this something that’s used anywhere else in WebKit or UIKit API? > > Have you considered other ways to expose this ability to clients? For example, calling the delegate when an element is focused and asking it to make a decision seems like it would afford more flexibility. Just to make sure -- would I add availability information via something like WK_AVAILABLE(10_12, 10_0)? Good point, it looks like assistance is not really mentioned in the WebKit API and in UIKit it refers to accessibility-related things rather than bringing up the keyboard. Maybe I could rephrase it to be something like "A Boolean value indicating whether programmatic focus on an element is allowed to display the keyboard." Also, now that I'm aware keyboardDisplayRequiresUserAction exists for UIWebView, I think we should rename this flag to keyboardDisplayRequiresUserAction for better consistency.

will this fix https://bugs.webkit.org/show_bug.cgi?id=142757 ?

Yes -- this is designed to tackle that problem. However, we're also working on an alternate (more powerful) means of providing ways to customize assistance behavior through https://bugs.webkit.org/show_bug.cgi?id=149646. Please keep in mind nothing is official yet, though.