GNOME Bugzilla – Bug 773327
epiphany not detecting form controls created by JS frameworks
Last modified: 2016-11-22 16:19:49 UTC
Epiphany is using the document-loaded signal from WebKitWebView to inspect the DOM of the document looking for any form tag. This works for statically generated content, but nowadays a lot of web applications are using frameworks like reactjs to generate the user/password form. Most of the times that JS content is not generated by the time document-loaded is emitted meaning that epy won't be aware of the form in the page. That has 2 implications a) ephy will never ask us to remember the password b) ephy will never autocomplete the form the next time we visit the web page. Perhaps the fix is to provide new API for the web extension. I was thinking about InjectedBundlePageFormClient.
(In reply to Sergio Villar from comment #0) > Perhaps the fix is to provide new API for the web extension. I was thinking > about InjectedBundlePageFormClient. Yeah, that sounds like the best way to me. I wonder if it already exists, since I don't see how else Chrome could have implemented it.
I've something working fine locally which involves (minimal) changes in WebKit and ephy but that IMO make the form filling substantially more robust. I'm planning to upload 3 patches: 1- remove the dependency of having a named password field. For some reason there are some asserts in the code that prevent the whole system from working if the password field is unnamed. Many JS frameworks correctly set the type of the input element to password but do not set any name. 2- change the way we fill in form fields so that it does not cause interferences with JS frameworks. Many of them listen to various events, onclick, blur... and carry on UI change in response to that, causing a lot of conflicts with ephy's auto fill code. 3- no longer use the "document-loaded" signal to start the form detection (and eventually form filling). As mentioned JS frameworks can dynamically create the forms way after this signal is issued. This requires a new signal that should be added to WebKitGtk first.
Created attachment 338527 [details] [review] Remove dependency with form_password Removes dependency with form_password. Having a named form password field is no longer a requirement for the ephy's form filling code.
Right, the fix added by Michael in bug 722530 was basically my second patch of this series.
Review of attachment 338527 [details] [review]: Thank you so much for working on this, Sergio! Looking forward to the other patches. I don't think the current patch makes sense on its own, since the password node really is still required to be set (e.g. in ephy_form_auth_data_compare() in ephy-web-extension.c), but maybe you have addressed that already in the work you haven't posted yet. ::: lib/ephy-form-auth-data.c @@ +74,2 @@ if (field_username) + g_hash_table_insert(attributes, g_strdup(FORM_USERNAME_KEY), g_strdup(field_username)); Remember to leave an empty space before the opening parens, here and below.
(In reply to Michael Catanzaro from comment #5) > Review of attachment 338527 [details] [review] [review]: > > Thank you so much for working on this, Sergio! Looking forward to the other > patches. > > I don't think the current patch makes sense on its own, since the password > node really is still required to be set (e.g. in > ephy_form_auth_data_compare() in ephy-web-extension.c), but maybe you have > addressed that already in the work you haven't posted yet. Right, I messed it up when splitting the patch in smaller ones.
Created attachment 338589 [details] [review] Remove dependency with form password node
Review of attachment 338589 [details] [review]: ::: lib/ephy-form-auth-data.c @@ +115,3 @@ g_return_if_fail (password); + g_return_if_fail (!form_username || (form_username && username)); + g_return_if_fail (!form_password || (form_password && password)); Hm, one more thing, you should remove the redundancy here: g_return_if_fail (!form_username || username); g_return_if_fail (!form_password || password); But I think the original check was better. Shouldn't it be this: g_return_if_fail ((form_username && username) || (!form_username && !username)); g_return_if_fail ((form_password && password) || (!form_password && !password));
(In reply to Michael Catanzaro from comment #8) > Review of attachment 338589 [details] [review] [review]: > > ::: lib/ephy-form-auth-data.c > @@ +115,3 @@ > g_return_if_fail (password); > + g_return_if_fail (!form_username || (form_username && username)); > + g_return_if_fail (!form_password || (form_password && password)); > > Hm, one more thing, you should remove the redundancy here: > > g_return_if_fail (!form_username || username); > g_return_if_fail (!form_password || password); That's right > But I think the original check was better. Shouldn't it be this: > > g_return_if_fail ((form_username && username) || (!form_username && > !username)); > g_return_if_fail ((form_password && password) || (!form_password && > !password)); Nop because that does not allow !form_password && password which is what this patch is about.
Review of attachment 338589 [details] [review]: Committed in master. Should we backport it to 3.22 ?
I'm not sure, does it fix any websites on its own? This whole suite of fixes would be desirable to backport, but you'd have to convince Carlos to backport your new WebKit API, which we do not generally do.
Created attachment 339502 [details] [review] Use the new WK API to detect forms This is the patch that gives epy dynamic form detecting capabilities. I'm not upgrading the WebKitGtk+ dependency though. I'll let reviewers decide whether we want to do it or perhaps add some #ifdef'ed code to use the old code with old WebKitGtk+ versions.
Review of attachment 339502 [details] [review]: Really, this is all that's needed? Cool! I prefer we update the dependency to 2.15.2, and just not commit the patch until the next WebKitGTK+ release, that way users are guaranteed to get the feature. (We already depend on 2.15.1 anyway.) I'll abuse the accepted-commit_after_freeze status to indicate this. ::: embed/web-extension/ephy-web-extension.c @@ +959,3 @@ + element = WEBKIT_DOM_ELEMENT (g_ptr_array_index (elements, i)); + if (!WEBKIT_DOM_IS_HTML_FORM_ELEMENT(element)) Missing space before parenthesis, please fix! @@ +962,3 @@ + continue; + + form = WEBKIT_DOM_HTML_FORM_ELEMENT (element); So what other sorts of WebKitDOMElements can get returned here? Why are they not all WebKitDOMHTMLFormElements?
(In reply to Michael Catanzaro from comment #13) > Review of attachment 339502 [details] [review] [review]: > > Really, this is all that's needed? Cool! > > I prefer we update the dependency to 2.15.2, and just not commit the patch > until the next WebKitGTK+ release, that way users are guaranteed to get the > feature. (We already depend on 2.15.1 anyway.) I'll abuse the > accepted-commit_after_freeze status to indicate this. Copy. > ::: embed/web-extension/ephy-web-extension.c > @@ +959,3 @@ > > + element = WEBKIT_DOM_ELEMENT (g_ptr_array_index (elements, i)); > + if (!WEBKIT_DOM_IS_HTML_FORM_ELEMENT(element)) > > Missing space before parenthesis, please fix! Ack > @@ +962,3 @@ > + continue; > + > + form = WEBKIT_DOM_HTML_FORM_ELEMENT (element); > > So what other sorts of WebKitDOMElements can get returned here? Why are > they not all WebKitDOMHTMLFormElements? As mentioned in the signal docs there are two posibilites: a) forms are added: then we get a list of form elements b) form elements (like <input>) are moved between forms: then we get the list of the elements which were moved