[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 #26 from Ryosuke Niwa <rniwa at webkit.org> ---
(In reply to Frederik Riedel from comment #25)
> Comment on attachment 318452 [details]
> 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.

> >> 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.

> >> 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?

Yes.

> We could also move the static function somewhere else. Any ideas for that?

We can move it to its own file. Maybe editing/EditorIOS.mm is be a good place too?

> >> 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.

> >> 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.

-- 
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/20170818/2866c6c8/attachment-0001.html>