[webkit-reviews] review requested: [Bug 129344] [iOS WebKit2] Form controls handling : [Attachment 225197] Patch1

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has asked	for review:
Bug 129344: [iOS WebKit2] Form controls handling
https://bugs.webkit.org/show_bug.cgi?id=129344

Attachment 225197: Patch1
https://bugs.webkit.org/attachment.cgi?id=225197&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=225197&action=review


Looks good to me. I am not a WK2 Owner.

> Source/WebKit2/ChangeLog:11
> +	   select the appropriate keybaord type based on the

Typo: "keybaord" => "keyboard"

> Source/WebKit2/Shared/AssistedNodeInformation.cpp:101
> +
> +

Nit: extra blank line

> Source/WebKit2/Shared/AssistedNodeInformation.h:31
> +#include <WebCore/WebAutocapitalize.h>
> +#include <WebCore/IntRect.h>

Nit: sort order

> Source/WebKit2/Shared/AssistedNodeInformation.h:39
> +    WKTypeText,

Should Password be a specific type instead of an "isPasswordField" boolean?

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:370
> +    case WebKit::WKTypeDate:
> +    case WebKit::WKTypeDateTime:
> +    case WebKit::WKTypeDateTimeLocal:
> +    case WebKit::WKTypeTime:
> +	   return !UICurrentUserInterfaceIdiomIsPad();

Needs Month type.

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1449
> +    if (!_assistedNodeInformation.formAction.isEmpty())

Some newlines would really make this code easier to read!

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1456
> +	       [_traits
setAutocapitalizationType:toUITextAutocapitalize(_assistedNodeInformation.autoc
apitalizeType)];
> +	       [_traits
setAutocorrectionType:_assistedNodeInformation.isAutocorrect ?
UITextAutocorrectionTypeYes : UITextAutocorrectionTypeNo];

Style: double indent.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1505
> +    Node* nextNode = isForward ?
page->focusController().nextFocusableElement(FocusNavigationScope::focusNavigat
ionScopeOf(&startNode->document()), startNode, key.get())
> +	   :
page->focusController().previousFocusableElement(FocusNavigationScope::focusNav
igationScopeOf(&startNode->document()), startNode, key.get());
> +    while (nextNode && !isAssistableNode(startNode))
> +	   nextNode = isForward ?
page->focusController().nextFocusableElement(FocusNavigationScope::focusNavigat
ionScopeOf(&startNode->document()), nextNode, key.get())
> +	   :
page->focusController().previousFocusableElement(FocusNavigationScope::focusNav
igationScopeOf(&startNode->document()), nextNode, key.get());

I think this is supposed to be isAssistableNode(nextNode) not startNode.

How reducing the duplicated logic with a do/while.

    Node* nextNode = startNode;
    do {
	nextNode = isForward
	    ?
page->focusController().nextFocusableElement(FocusNavigationScope::focusNavigat
ionScopeOf(&startNode->document()), nextNode, key.get())
	    :
page->focusController().previousFocusableElement(FocusNavigationScope::focusNav
igationScopeOf(&startNode->document()), nextNode, key.get());
    } while (nextNode && !isAssistableNode(startNode)

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1554
> +	       information.elementType = element->getAttribute("pattern") ==
"\\d*" || element->getAttribute("pattern") == "[0-9]*" ? WKTypeNumberPad :
WKTypeNumber;

I think this should also happen for text inputs. If we get <input type="text"
pattern="[0-9]*"> we should show a number pad. Not only for <input
type="number">.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1574
> +	   information.valueAsNumber = element->valueAsNumber();

Will we need to be careful about only encoding valueAsNumber when it is valid?

    document.createElement("input").valueAsNumber => NaN.