[Webkit-unassigned] [Bug 174394] Expose way to set values of classified form controls as {Legacy WebKit, WebKit} SPI

[bugzilla-daemon at webkit.org]

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

--- Comment #27 from Frederik Riedel <riedel at apple.com> ---
Comment on attachment 318452
  --> https://bugs.webkit.org/attachment.cgi?id=318452
Patch

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

>>>> Source/WebCore/html/HTMLInputElement.cpp:903
>>>> +
>>> 
>>> I don't think we need to add this method to HTMLInputElement.
>>> Whoever calls this function can just check the return value instead.
>> 
>> So this means that we expose the computeUsernameAndPasswordElements method and call it directly from outside instead of wrapping it into acceptsAutofilledLoginCredentials?
> 
> Right.

Alright, done.

>>>> Source/WebCore/html/HTMLInputElement.cpp:907
>>>> +}
>>> 
>>> Given assisted node has a very specific meaning on iOS, I don't think we should event new terminology like "assitable element".
>>> Now, how does filling date time field with username help at all?
>>> 
>>> I'd suggest supportsAutofill or something. But I don't think there's any reason this needs to be a method of HTMLInputElement.
>>> It's a lot better for this function be just a static local function which takes HTMLInputElement as an argument.
>>> 
>>> I don't think we should be checking that. If anything we should be checking for isTextField() || isEmailField().
>> 
>> I implemented this because there was a comment in WebPageIOS about this:
>>         // FIXME: This laundry list of types is not a good way to factor this. Need a suitable function on HTMLInputElement itself.
>> Wouldn't the method that I implemented be a suitable function for this?
> 
> I don't think hard coding the list of input types is what this FIXME is asking. I actually don't think we need to move this code to WebCore given the entire concept of whether an input element is assitable or not is specific to iOS.
> 
> Regardless, it doesn't make sense for your code to checking this since input with type=datetime is most certainly not a text field used for a username or a password.

Ok, that makes sense to me. Done.

>>>> Source/WebCore/html/HTMLInputElement.h:79
>>>> +    static bool computeUsernameAndPasswordElements(Element *, RefPtr<HTMLInputElement>&, RefPtr<HTMLInputElement>&, const Document&);
>>> 
>>> As I said in the person, we shouldn't be adding all these functions to HTMLInputElement.
>> 
>> If we expose the computeUsernameAndPasswordElements with WEBCORE_EXPORT, we can remove acceptsAutofilledLoginCredentials.
>> Then we also don't need autofillLoginCredentials because we can call it on the HTMLInputElements directly that we get back from the new AutofillElements struct.
>> Does that sound reasonable to you?
>> We could also move the static function somewhere else. Any ideas for that?
> 
> Yes.

Done, implemented in EditorIOS.

>>>> Source/WebCore/page/FocusController.cpp:93
>>>> +        return true;
>>> 
>>> Why specifically select * textarea? Are we looking for any form control element?
>>> This is exactly the kind of case where a single line comment as to why we consider these elements as "assistable" is useful.
>>> Again, I don't think we should be using that terminology.
>> 
>> This is not a specific function for the autofill feature, but supports the nextAssistableElement function that I moved from WebKit2 to WebCore.
> 
> I don't think we should do that. The entire concept of assisted node is specific to iOS. It has not business being inside FocusController. Again, the focus controller should be concerned about controlling the focused node, and nothing else.

Ok, done.

>>>> Source/WebCore/page/FocusController.h:81
>>>> +
>>> 
>>> This code doesn't really belong to the focus controller. FocusController controls focus. It's a place to put code that does autofill.
>> 
>> So what place would you suggest for this?
>> A new `AssistableController`?
> 
> Why do we need a class for this? It seems like we just need a function which iterates over focused elements. A free standing function in EditorIOS.mm would suffice.

EditorIOS contains now the iOS-specific code for this feature. Done.

-- 
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/20170829/717a4e59/attachment.html>