Allow Password AutoFill in more text field configurations
Created attachment 419321 [details] First attempt at a patch
<rdar://problem/73997299>
Comment on attachment 419321 [details] First attempt at a patch View in context: https://bugs.webkit.org/attachment.cgi?id=419321&action=review Looks good! Just a couple of minor comments. We should also write an API test for this (similar to the existing ones in WKWebViewAutoFillTests). > Source/WebCore/editing/cocoa/AutofillElements.cpp:99 > + return AutofillElements( > + previousFieldIsTextField ? WTFMove(previousElement) : nullptr, > + WTFMove(start), > + hasSecondPasswordFieldToFill ? WTFMove(nextElement) : nullptr > + ); We could probably put this on one line, as `return {{ previousFieldIsTextField ? WTFMove(previousElement) : nullptr, WTFMove(start), hasSecondPasswordFieldToFill ? WTFMove(nextElement) : nullptr }};` > Source/WebCore/editing/cocoa/AutofillElements.cpp:104 > + RefPtr<HTMLInputElement> elementAfterNextElement = nextAutofillableElement(nextElement.get(), focusController); Nit - `auto elementAfterNextElement = nextAutofillableElement(nextElement.get(), focusController);` > Source/WebCore/editing/cocoa/AutofillElements.cpp:108 > + return AutofillElements(WTFMove(start), WTFMove(nextElement), > + hasSecondPasswordFieldToFill ? WTFMove(elementAfterNextElement) : nullptr); Nit - I think this should could just be a single line as `return {{ WTFMove(start), WTFMove(nextElement), hasSecondPasswordFieldToFill ? WTFMove(elementAfterNextElement) : nullptr }};`
(In reply to Wenson Hsieh from comment #3) > Comment on attachment 419321 [details] > First attempt at a patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=419321&action=review > > Looks good! Just a couple of minor comments. We should also write an API > test for this (similar to the existing ones in WKWebViewAutoFillTests). Actually, it looks like the existing test WKWebViewAutoFillTests.AccountCreationPage already exercises this scenario, and just needs to be rebaselined. > > > Source/WebCore/editing/cocoa/AutofillElements.cpp:99 > > + return AutofillElements( > > + previousFieldIsTextField ? WTFMove(previousElement) : nullptr, > > + WTFMove(start), > > + hasSecondPasswordFieldToFill ? WTFMove(nextElement) : nullptr > > + ); > > We could probably put this on one line, as `return {{ > previousFieldIsTextField ? WTFMove(previousElement) : nullptr, > WTFMove(start), hasSecondPasswordFieldToFill ? WTFMove(nextElement) : > nullptr }};` > > > Source/WebCore/editing/cocoa/AutofillElements.cpp:104 > > + RefPtr<HTMLInputElement> elementAfterNextElement = nextAutofillableElement(nextElement.get(), focusController); > > Nit - `auto elementAfterNextElement = > nextAutofillableElement(nextElement.get(), focusController);` > > > Source/WebCore/editing/cocoa/AutofillElements.cpp:108 > > + return AutofillElements(WTFMove(start), WTFMove(nextElement), > > + hasSecondPasswordFieldToFill ? WTFMove(elementAfterNextElement) : nullptr); > > Nit - I think this should could just be a single line as `return {{ > WTFMove(start), WTFMove(nextElement), hasSecondPasswordFieldToFill ? > WTFMove(elementAfterNextElement) : nullptr }};`
Created attachment 419433 [details] Second try
Comment on attachment 419433 [details] Second try View in context: https://bugs.webkit.org/attachment.cgi?id=419433&action=review r=mews > Source/WebCore/editing/cocoa/AutofillElements.cpp:78 > : m_username(username) > , m_password(password) > + , m_secondPassword(secondPassword) (Not exactly new to this patch, but these could all be `WTFMove`d into the class members)
(In reply to Wenson Hsieh from comment #6) > Comment on attachment 419433 [details] > Second try > > View in context: > https://bugs.webkit.org/attachment.cgi?id=419433&action=review > > r=mews > > > Source/WebCore/editing/cocoa/AutofillElements.cpp:78 > > : m_username(username) > > , m_password(password) > > + , m_secondPassword(secondPassword) > > (Not exactly new to this patch, but these could all be `WTFMove`d into the > class members) Perhaps we can just do this as a followup.
Comment on attachment 419433 [details] Second try C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\inspector/agents/InspectorDOMAgent.cpp(1761,99): error C2514: 'WTF::Optional': class template cannot be constructed (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-84c9f43f-5.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj] C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\inspector/agents/InspectorDOMAgent.cpp(1761,99): error C2451: conditional expression of type 'WTF::Optional' is illegal (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-84c9f43f-5.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj] C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\inspector/agents/InspectorDOMAgent.cpp(1762,36): error C2039: 'setLayoutContextType': is not a member of 'Inspector::Protocol::DOM::Node' (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-84c9f43f-5.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj] C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\inspector/agents/InspectorDOMAgent.cpp(1762,54): error C2663: 'WTF::Optional<T>::value': 3 overloads have no legal conversion for 'this' pointer (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-84c9f43f-5.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj] Windows build failure does not appear to be related.
Committed r272448: <https://trac.webkit.org/changeset/272448> All reviewed patches have been landed. Closing bug and clearing flags on attachment 419433 [details].