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

Thu Aug 17 18:19:02 PDT 2017

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

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

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

>> Source/WebCore/ChangeLog:3022
>> +2017-07-24  Frederik Riedel  <friedel at apple.com>
> 
> You should remove the previous ChangeLog entry here.

Done.

>> Source/WebKit/ChangeLog:1613
>> +2017-07-24  Frederik Riedel  <friedel at apple.com>
> 
> Ditto.

Done.

>> Source/WebKitLegacy/mac/ChangeLog:105
>> +2017-07-24  Frederik Riedel  <friedel at apple.com>
> 
> Ditto.

Done.

>> Source/WebCore/dom/Document.cpp:6897
>> +Element* Document::nextFocusableElement(Element& element)
> 
> The return type here should be a RefPtr<Element>. TBH, FocusController should probably not return a raw pointer either, but that is something we should address separately (additionally, I don't see why it's an Element& instead of a const Element&, but that's also a separate issue).

Done.

>> Source/WebCore/dom/Document.cpp:6899
>> +    return page()->focusController().nextFocusableElement(element);
> 
> Perhaps we should add a null check here too -- it looks like the frame may not necessarily exist, and so page() is not necessarily non-null. Something like:
> 
> if (auto* page = this->page())
>     return page->focusController().nextFocusableElement(element);
> return nullptr;

Done.

>> Source/WebCore/dom/Document.cpp:6902
>> +Element* Document::previousFocusableElement(Element& element)
> 
> Ditto with the RefPtr<Element>

Removed method in latest patch.

>> Source/WebCore/dom/Document.cpp:6904
>> +    return page()->focusController().previousFocusableElement(element);
> 
> Ditto with the null check.

Removed method in latest patch.

>> Source/WebCore/html/HTMLInputElement.cpp:909
>> +        usernameElement->setValue(username);
> 
> We should use setValueForUser(username) instead. This simulates user input instead of just setting the value.

Done.

>> Source/WebCore/html/HTMLInputElement.cpp:911
>> +        passwordElement->setValue(password);
> 
> Ditto for setValueForUser(password).

Done.

>> Source/WebCore/html/HTMLInputElement.cpp:919
>> +}
> 
> We should just access input element's document() and then its url() in WebKit layer instead of WebCore.

This is a good idea. I was able to get the url in WebKitLegacy using self.ownerDocument.URL. Do you know how I can something similar from WKContentView (this is the object I receive as the first responder for WebKit2)?

>> Source/WebCore/html/HTMLInputElement.cpp:921
>> +bool HTMLInputElement::computeUsernameAndPasswordElements(Element *start, HTMLInputElement*& usernameElement, HTMLInputElement*& passwordElement)
> 
> Instead of raw pointer references as arguments, I think taking a RefPtr<HTMLInputElement>& or std::optional<HTMLInputElement>& would be better.

Done.

>> Source/WebCore/html/HTMLInputElement.cpp:922
>> +{
> 
> Also, it looks like the only state on the HTMLInputElement that we use in this method is this->document(), so it's a bit strange to make this a method on HTMLInputElement. Perhaps this should be a static helper function in HTMLInputElement that takes a const Document&, or even a helper method on Document.

Is now static, done.

>> Source/WebCore/html/HTMLInputElement.cpp:924
>> +        HTMLInputElement* inputElement = downcast<HTMLInputElement>(start);
> 
> In WebKit, we use auto in most places when explicitly specifying the type is redundant (as in this case). Something like:
> 
> auto& inputElement = downcast<HTMLInputElement>(*start);
> 
> ...would be better. (we also know that the input element definitely exists here too, since is<HTMLInputElement>(start) would have been false otherwise).

I am now using RefPtr<HTMLInputElement> inputElement = &downcast<HTMLInputElement>(*start);
Can I still use auto for that?

>>> Source/WebCore/html/HTMLInputElement.cpp:927
>>> +            Element* previousElement = document().previousFocusableElement(*start);
>> 
>> I don't think we should assume that the username and password text fields are next to each other after start element.
>> Safari should just figure out which element is for username & which one is for password and call [inputElement setValue] instead.
> 
> Isn't the previousFocusableElement/nextFocusableElement the same API that we use for the little blue arrow keys on top of iOS's keyboard? In this case I would expect that it works for most login forms (simulate pressing "tab").

I am now using nextAssistableElement which is indeed the method that simulates the tab press that we use for the blue up/down arrows in iOS.

>> Source/WebCore/html/HTMLInputElement.cpp:928
>> +            if (previousElement && is<HTMLInputElement>(previousElement)) {
> 
> The previousElement null check here isn't needed, since is<HTMLInputElement>(previousElement) will return false anyways if previousElement is null.

Done.

>> Source/WebCore/html/HTMLInputElement.cpp:939
>> +            if (nextElement && is<HTMLInputElement>(nextElement)) {
> 
> Ditto with the null check.

Done.

>> Source/WebCore/html/HTMLInputElement.cpp:949
>> +    return false;
> 
> Let's also set usernameElement and passwordElement to nullptr here before returning false, to ensure that they are null if username/password element computation failed.

Done.

>>>> Source/WebKit/Platform/spi/ios/UIKitSPI.h:46
>>>> +#import <UIKit/UIKeyboardLoginCredentialsSuggestion.h>
>>> 
>>> If this is a new header, this will need to be ifdef'ed out for this to compile on older iOSes.
>> 
>> Is this DYLD_IOS_VERSION_11_0?
> 
> Assuming this is in iOS 11, __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000 is what you want.

I assume that #if __has_include(<UIKit/UIKeyboardLoginCredentialsSuggestion.h>) is even better for that?

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2764
>> +        information.representingPageUrl = element.representingPageUrl();
> 
> I think it would be better to ask the Page or Document directly here, instead of going through the HTMLInputElement.

Do you know how I can get the URL at this point from the m_page? I couldn't figure that out in the current patch.
This would also fix Ryosuke's comment.

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2830
>> +void WebPage::autofillLoginCredentials(String username, String password)
> 
> Let's change this to take const String&s here.

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/20170818/8b8e3d60/attachment.html>