After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 696867 - form filler
form filler
Status: RESOLVED OBSOLETE
Product: epiphany
Classification: Core
Component: Passwords, Cookies, & Certificates
3.18.x (obsolete)
Other Linux
: Normal enhancement
: ---
Assigned To: Epiphany Maintainers
Epiphany Maintainers
https://wiki.gnome.org/Design/Apps/We...
Depends on:
Blocks:
 
 
Reported: 2013-03-29 17:59 UTC by Adam Dingle
Modified: 2018-08-03 19:56 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
The patch file; mostly .ui files (54.58 KB, patch)
2015-04-24 23:03 UTC, Abdullah Alansari
needs-work Details | Review
Fix styling issues (12.23 KB, patch)
2015-04-26 01:43 UTC, Abdullah Alansari
none Details | Review
Fix fields filled without user consent and store using libsecret (58.63 KB, patch)
2015-04-30 00:09 UTC, Abdullah Alansari
needs-work Details | Review
Fix some security issues. Works now on SPA websites. (140.71 KB, patch)
2015-05-11 18:57 UTC, Abdullah Alansari
needs-work Details | Review
Close the popup when the page is closed or reloaded. Use labels in addition to name attribute. (152.57 KB, patch)
2015-05-18 06:09 UTC, Abdullah Alansari
needs-work Details | Review
revised minimal patch (less than half the original patch size with the same functionality) (76.31 KB, patch)
2015-06-14 18:39 UTC, Abdullah Alansari
none Details | Review
A much more usable and secure version of Autofill (867.14 KB, patch)
2015-08-19 03:52 UTC, Abdullah Alansari
none Details | Review
remove unnecessary changes in po/POTFILES.in (769.53 KB, patch)
2015-08-24 05:44 UTC, Abdullah Alansari
none Details | Review
A one-commit patch re-based against master (178.02 KB, patch)
2015-09-06 14:36 UTC, Abdullah Alansari
none Details | Review
Fix some styling issues; one-commit patch rebased against master (175.55 KB, patch)
2015-10-01 13:41 UTC, Abdullah Alansari
none Details | Review
AutoFill mockup v1 (94.11 KB, image/png)
2015-10-01 20:02 UTC, Diogo Campos
  Details
AutoFill mockup v2 (99.86 KB, image/png)
2015-10-01 21:23 UTC, Diogo Campos
  Details
AutoFill mockup v2 (B) (63.67 KB, image/png)
2015-10-01 23:26 UTC, Diogo Campos
  Details
AutoFill mockup v3 (81.26 KB, image/png)
2015-10-02 15:37 UTC, Diogo Campos
  Details
AutoFill mockup v4 (219.85 KB, image/png)
2015-10-02 23:08 UTC, Diogo Campos
  Details
Mockup v4 implementation (392.05 KB, patch)
2015-10-27 15:30 UTC, Abdullah Alansari
none Details | Review
Add form autofill (174.71 KB, patch)
2015-12-09 10:50 UTC, Michael Catanzaro
none Details | Review
Autofill GtkPopover initial implementation (173.69 KB, patch)
2016-02-18 17:37 UTC, Abdullah Alansari
none Details | Review
Screenshot #1 (97.02 KB, image/png)
2016-02-23 20:37 UTC, Michael Catanzaro
  Details
screenshot #2 (34.04 KB, image/png)
2016-02-23 20:40 UTC, Michael Catanzaro
  Details
Screenshot #3 (36.05 KB, image/png)
2016-02-23 20:41 UTC, Michael Catanzaro
  Details
Show popup on single click and add "Do not autofill" button + don't fill apartment fields (177.71 KB, patch)
2016-03-09 07:49 UTC, Abdullah Alansari
none Details | Review
Autofill popover with "Do not autofill" button (86.17 KB, image/png)
2016-03-09 07:50 UTC, Abdullah Alansari
  Details
Don't autofill invisible elements (178.41 KB, patch)
2016-03-09 12:02 UTC, Abdullah Alansari
none Details | Review
Latest patch rebased against latest upstream (178.37 KB, patch)
2017-02-03 19:48 UTC, Abdullah Alansari
needs-work Details | Review
Current Preferences Mockup (38.00 KB, image/png)
2017-03-20 05:12 UTC, Abdullah Alansari
  Details

Description Adam Dingle 2013-03-29 17:59:51 UTC
I'd love for Epiphany to include a form filler which can automatically fill in my name, address, and maybe even credit card information on a Web page. I hate having to type this information in over and over again on every Web form I fill out, e.g. when ordering something online.

Name/address/credit card information could appear in a new tab in the Personal Data window.
Comment 1 Carlos Alberto Lopez Perez 2014-04-17 15:49:10 UTC
Seems there is bounty of 1K for developing this feature: https://www.bountysource.com/issues/1325823-form-filler
Comment 2 Amine kabab 2014-06-23 16:17:43 UTC
Hi, I like to work on this bug and give with some suggestions and questions
 - first of all, it should be a setting window where the user can enter his personal information, his addresses and his credit cards.
 - When the user is going to connect to the website where there's credit form or address form, Epiphany should detect that form and fill it, but the question is, how to detect that form ? is there some standards or just we should implements all the possibility of the inputs names ? 
 - Another thing is should this feature add the possibility of saving the old entered informations and reuse it for autofill ? 
 - And we should know how the setting window will appear ? 
 - For information storage I think we can use a sqlite database, because I see epiphany already use it for saving bookmarks and history.
Comment 3 Adam Dingle 2014-06-24 01:07:04 UTC
Amine, thanks for your interest in working on this.  Here are my thoughts about your points:

 - first of all, it should be a setting window where the user can enter his personal information, his addresses and his credit cards.

Yes.  Specifically, I think this should be a new tab in the Preferences (or Personal Data) dialog.

 - When the user is going to connect to the website where there's credit form or address form, Epiphany should detect that form and fill it, but the question is, how to detect that form ? is there some standards or just we should implements all the possibility of the inputs names ? 

First, there's an IETF standard for field names for electronic commerce.  I don't know how widely it's used, but we should detect these at least:

http://www.ietf.org/rfc/rfc3106.txt

Because many sites don't use these names, you'll also need to be able to automatically detect form fields such as first name, last name, address and so on automatically.  This is the hard part.  :)  It's hard for several reasons:

1. You need to detect which labels apply to which text fields.  Often that will be easy (because the label is right next to the field in the DOM), but sometimes it might be tricky since, for instance, a label might be in one table column and its field might be in another.

2. There are various sets of fields that might appear.  For example, there might be separate fields for first and last names, or a single field that holds both.  A field for a credit card expiration date might have two dropdowns for specifying the month and year, or a single text field that lets you enter something like "4/15".

3. The labels themselves will have several possible variations, even within a single language.  For example, an expiration date field might be "Expiration" or "Expires" or "Expiration Date".

Ideally you'll have heuristics that can handle all the above variations.

The only way to make this work well will to be build a test set that contains dozens of web pages from real world sites.  You should manually mark the correct answers for the test set, and build a test suite that attempts to autofill each page and evaluates how many pages it got correctly.  By tweaking your heuristics, you can improve the accuracy over time.

I know that all the above is possible, because I used to work at Google and I wrote the original autofiller in the Google Toolbar.   :)  I implemented all of the above, including the test suite I mentioned.  I'm an experienced programmer and it took me several months of full-time work.  But maybe you'll be faster.  :)

Of course the Google Toolbar implementation is not free for us to use here, but you also look around to see if there are open source implementations of autofillers for Firefox, Chrome or other browsers.  If so, and you can reuse them or simply port them to Epiphany, that might greatly reduce the work to complete this project.

 - Another thing is should this feature add the possibility of saving the old entered informations and reuse it for autofill ?

In theory that would be nice, but for a first implementation I'd suggest simply filling the information from the settings window.

 - And we should know how the setting window will appear ? 

See my first comment above.

 - For information storage I think we can use a sqlite database, because I see epiphany already use it for saving bookmarks and history.

You could store the personal information there, or in a file in some directory such as ~/.config/epiphany, or in DConf.  All of those might be reasonable options.

One final point: I think the autofill feature should automatically highlight form fields that it can autofill, e.g. by turning them yellow.  That will alert the user that the autofiller can help them on that page.

Let us know if you'd still like to work on this project given all the points above.  If so, good luck with it!
Comment 4 Abdullah Alansari 2015-04-23 16:58:47 UTC
You can change auto-fill (form filler) settings from the preferences window (Ctrl-E) under Privacy tab click on "Manage Autofill..." button. A dialog will appear with fields' values. Modify the fields and the changes will be persisted automatically. You can then close the dialog and the appropriate fields will be filled.

You can find the code here: https://github.com/Ahimta/epiphany/tree/autofill

The form filler now still have some rough edges, please provide feedback to make it better.

Last time I wrote C code was in CS50 fall of 2012, since then I worked with many other languages (Java, Scala, Ruby, Python, Javascript, Racket, Haskell StandardML), however for some reason, it feels especially good to work with C.
Comment 5 Claudio Saavedra 2015-04-24 06:45:03 UTC
Hi Abdullah, thanks for working on this. I skimmed quickly the latest commit in your clone. It would be great if you could clean the patch a bit (there are some spurious changes), split it in smaller independent patches, and then attach them here for review.
Comment 6 Michael Catanzaro 2015-04-24 14:17:51 UTC
Please also be sure to follow the same coding style as is used in the files you are editing (for example, indent the same as in the rest of the file, put opening braces in the same places, and add spaces before the opening parentheses of each function call).
Comment 7 Abdullah Alansari 2015-04-24 23:03:50 UTC
Created attachment 302320 [details] [review]
The patch file; mostly .ui files

More than half the changes are ui files which are better opened in Glade.
Many other changes are removed trailing spaces, my editor removes them upon saving, and so they are only removed from modified files.

In addition I used the same style for each file, including writing code in C90 standard.

If you still find the commits too big please tell me how you want them to be divided.

I would also like to have some feedback from someone who really wants this feature,
in my opinion it will be better than me guessing how it should be done and, as almost
always the case for such situations, spending too much time and getting it wrong at the same time.
Comment 8 Michael Catanzaro 2015-04-25 14:48:19 UTC
Review of attachment 302320 [details] [review]:

Thanks for your patch! This is a much larger patch tackling a much harder issue than we normally see for a first contribution. You did a good job finding all the right files to edit.

::: data/org.gnome.epiphany.gschema.xml
@@ +257,3 @@
 		</key>
 	</schema>
+	<schema path="/org/gnome/epiphany/autofill/" id="org.gnome.Epiphany.autofill">

I don't think gsettings is a good place to store credit card info or other personal data. We should use libsecret for this instead. See https://developer.gnome.org/libsecret/0.18/

::: embed/web-extension/ephy-web-extension.c
@@ +897,3 @@
+autofill_fill_fields (WebKitDOMDocument *document)
+{
+  int select_nodes_length, input_nodes_length, i;

There are some coding style issues in this file. I'll mention each one just once for this first review. First off: one variable declaration per line, please.

@@ +903,3 @@
+
+  WebKitDOMHTMLSelectElement *select_element;
+  WebKitDOMHTMLInputElement  *input_element;

Please get rid of the extra space before the variable names.

@@ +908,3 @@
+
+  firstname = g_settings_get_string (EPHY_SETTINGS_AUTOFILL, EPHY_PREFS_AUTOFILL_FIRSTNAME);
+  lastname  = g_settings_get_string (EPHY_SETTINGS_AUTOFILL, EPHY_PREFS_AUTOFILL_LASTNAME);

Also please don't add extra spaces before the assignment operator; we don't keep these aligned as you did.

@@ +925,3 @@
+
+  for (i = 0; i < input_nodes_length; i++)
+  {

In this file, opening braces go at the end of the line.

@@ +931,3 @@
+    name          = webkit_dom_html_input_element_get_name (input_element);
+    type          = webkit_dom_html_input_element_get_input_type (input_element);
+    if (name != NULL) ephy_string_tolower (name);

Please put conditional bodies on the line below the conditional, never on the same line.

@@ +933,3 @@
+    if (name != NULL) ephy_string_tolower (name);
+
+    if      (name == NULL) {}

This long conditional can be split into a function that takes name and type as parameters and returns NULL. I think that would make this function much easier to read.

@@ +935,3 @@
+    if      (name == NULL) {}
+
+    else if (g_ascii_strcasecmp(type, email) == 0 ||

Be sure to leave one space before the opening parentheses:

g_ascii_strcasecmp (type, email)

@@ +1036,3 @@
   forms_n = webkit_dom_html_collection_get_length (forms);
 
+  autofill_fill_fields(document);

Hm, there is a problem with this: real-world websites are evil and want to steal users' personal data without consent. Also, real-world websites are served over HTTP, which makes it easy to control the DOM of non-evil sites and inject malicious javascript. If we autofill automatically, then the page could use javascript to report the autofilled value to a server without any user interaction at all. E.g. I visit http://www.gnome.org, the attacker intercepts my connection and adds a credit-card-number input element to the bottom of the page and some javascript that runs when the input element is filled, and now the attacker has my credit card number. Or he tricks me into visiting https://www.gnȯme.org (with the teensy dot on the o) and then doesn't even need to intercept my connection.

So we need to require some user interaction before personal data is autofilled, in a way that can't be scripted by the page. I'm curious how Chromium and Firefox handle this; probably with a dropdown (we'd use GtkEntryCompletion). This might require changes in WebKit.

::: lib/ephy-string.c
@@ +85,3 @@
  * @str: the string to shorten, in UTF-8
  * @target_length: the length of the shortened string (in characters)
+ *

Please set your editor to not trim trailing whitespace from files, or else submit first one patch that only trims trailing whitespace and then a second patch for the rest of your changes.

::: src/prefs-autofill-dialog.c
@@ +1,3 @@
+/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- */
+/*
+ *  Copyright © 2013 Red Hat, Inc.

This should be 2015, and it should be your name or the name of your employer (unless you work for Red Hat? :)

::: src/prefs-dialog.c
@@ +159,3 @@
 static void
+on_manage_autofill_button_clicked (GtkWidget *button,
+				                           PrefsDialog *dialog)

The type of each parameter should be vertically aligned, like this:

on_manage_autofill_button_clicked (GtkWidget *button,
                                   PrefsDialog *dialog)

@@ +164,3 @@
+
+	autofill_dialog = g_object_new (EPHY_TYPE_PREFS_AUTOFILL_DIALOG,
+					                        "use-header-bar", TRUE,

Arguments should be vertically aligned as well:

autofill_dialog = g_object_new (EPHY_TYPE_PREFS_AUTOFILL_DIALOG,
                                "use-header-bar", TRUE
                                NULL);

@@ +169,3 @@
+	gtk_window_set_transient_for (GTK_WINDOW (autofill_dialog), GTK_WINDOW (dialog));
+	gtk_window_set_modal (GTK_WINDOW (autofill_dialog), TRUE);
+	gtk_window_present (GTK_WINDOW (autofill_dialog));

Can we use gtk_dialog_run() here instead?

I think we also need to use gtk_dialog_destroy()?
Comment 9 Michael Catanzaro 2015-04-25 14:50:54 UTC
(In reply to Michael Catanzaro from comment #8)
> @@ +933,3 @@
> +    if (name != NULL) ephy_string_tolower (name);
> +
> +    if      (name == NULL) {}
> 
> This long conditional can be split into a function that takes name and type
> as parameters and returns NULL.

I meant to say: "and return element_value or NULL."
Comment 10 Abdullah Alansari 2015-04-26 01:43:08 UTC
Created attachment 302356 [details] [review]
Fix styling issues

I fixed most styling issues.

I don't know if we should call gtk_destroy() and gtk_dialog_run(). I just copied the code from on_manage_passwords_button_clicked() and it may well be the case that gtk_destroy() is called when the dialog is closed in this case.

As for libsecret and the issue that the fields are populated without the user consent, I wanted these changes to be last, since most likely they would depend on the current implementation, which will be very different by the time I am finished with the core functionality.

Actually, all the changes so far are my way of starting a conversation about this issue (there's still a lot to be done), however, it seems that my approach is creating unnecessary extra work for the team (and emails going back and forth).

Funny thing I thought of not even freeing memory in my code until I finished the feature so the code is easier to change, however, then it would give the wrong impression that I don't know how to manage memory in C. By the way what do you use to fix memory leaks, Valgrind or something else?

What fields do you think I should store using libsecret? The credit card info or everything.

As for asking the user to auto-fill the fields, I think it should be like the info bar for saving the password.

Your remark on my forgetting to write my name instead of 'Red Hat' just made my day
and no need to be disappointed, I just managed to forget to change the year too :)
Comment 11 Michael Catanzaro 2015-04-26 18:04:47 UTC
Hey Adam! See the bottom of this comment please:

(In reply to Abdullah Alansari from comment #10)
> I don't know if we should call gtk_destroy() and gtk_dialog_run(). I just
> copied the code from on_manage_passwords_button_clicked() and it may well be
> the case that gtk_destroy() is called when the dialog is closed in this case.

I guess we should be consistent with how those functions work, yes. Look at how those dialogs destroy themselves by connecting to the response signal and calling gtk_widget_destroy() in the callback.

> As for libsecret and the issue that the fields are populated without the
> user consent, I wanted these changes to be last, since most likely they
> would depend on the current implementation, which will be very different by
> the time I am finished with the core functionality.
> 
> Actually, all the changes so far are my way of starting a conversation about
> this issue (there's still a lot to be done), however, it seems that my
> approach is creating unnecessary extra work for the team (and emails going
> back and forth).

Not at all! This is just the usual bugzilla mail.

> Funny thing I thought of not even freeing memory in my code until I finished
> the feature so the code is easier to change, however, then it would give the
> wrong impression that I don't know how to manage memory in C. By the way
> what do you use to fix memory leaks, Valgrind or something else?

Valgrind is the only tool I know of. Address sanitizer is not really usable because it's hard to compile everything to support it, and because it stops at the first leak, whereas we have so many memory leaks, you'll never get to the one you're looking for.

Best is to just be careful when you're developing: there's no tool that's good enough to find mistakes only in the code that you've modified.

> What fields do you think I should store using libsecret? The credit card
> info or everything.

Everything. Let's not store any personal data in gsettings.

> As for asking the user to auto-fill the fields, I think it should be like
> the info bar for saving the password.

Well, that would certainly be easier to implement, but I think it would be better if the user had to click on the form before being prompted to autofill. So that a form anywhere in the page that the user may have no intention of filling out doesn't trigger the info bar. It looks like Adam Dingle is the expert on form autofill; I wonder what he would recommend?

> Your remark on my forgetting to write my name instead of 'Red Hat' just made
> my day
> and no need to be disappointed, I just managed to forget to change the year
> too :)
Comment 12 Adam Dingle 2015-04-28 23:02:08 UTC
Abdullah, thanks for your work on this so far.  I built and ran your code and this looks like a promising start.  I'm eager to see this land in Epiphany - I would use it all the time.

A few comments:

- I think the dialog should definitely store the user's address (street, city, state/region, country, zip or postal code) as well.  It's a big pain to have to enter all this information on lots of sites.

- In the dialog, "Firstname" and "Lastname" should be "First Name" and "Last Name".  (Alternatively, there could be a single field "Name" which Autofill could parse into first/last names automatically.)

- It would be nice to store the credit card type (e.g. MasterCard/Visa) since many sites make you enter that.

- I don't think Autofill really belongs in the Privacy tab.  It's true that this personal information is private, but the feature itself is not primarily about user privacy.  I'd suggest putting this in the General tab, unless the Epiphany developers have a different opinion.

- I completely agree that Autofill should absolutely not fill information unless the user explicitly tells it to - anything otherwise would be a serious security hole.  I think an info bar prompt would be OK.  Even when the user does explicitly ask for information to be filled, there's still a risk that a malicious page could have credit card fields that are invisible or hard to see: the user might think they are filling only their email address, but are actually providing the page with their full credit card information.

  One way to avoid this would be to explicitly tell the user what is being filled.  For example, the info bar could say something like "Press [Autofill] to fill in your name and email address on this page", or "Press [Autofill] to fill in your name, email address, mailing address and credit card information on this page".  That might be hard to internationalize, however, so another approach would be to show a dialog listing the categories of information that will be filled (e.g. mailing address, credit card info), ideally with an icon for each.  The user would have to confirm the autofill by pressing OK in that dialog.

  A simpler alternative would be to simply display a warning message "A malicious site may have fields that are hard to see.  You should only use Autofill on sites you trust."  We could show the message the first the the user uses the feature, perhaps with a checkbox they can check to never see the message again.  Of course, if we take this approach then most users will simply check the box and never think about it again.  :)

- Another security point: Autofill should also never fill in credit card information on a page that isn't using HTTPS.

- I don't have a strong opinion about whether the user should need to click in a form before triggering the info bar.  We could just implement an info bar that triggers automatically and see if it's annoying.  It might be, though.
Comment 13 Adam Dingle 2015-04-28 23:04:16 UTC
Also, if the user had to click in the form, then click on the info bar, then click OK in a confirmation dialog, that might be too much.  So if we want to require them to click in the form, we might want to jump right to the confirmation dialog (if we want one) and skip the info bar altogether.
Comment 14 Reinout van Schouwen 2015-04-29 06:29:28 UTC
> 
> - In the dialog, "Firstname" and "Lastname" should be "First Name" and "Last
> Name".  (Alternatively, there could be a single field "Name" which Autofill
> could parse into first/last names automatically.)

Please don't parse automatically unless you can get this right for all styles of first names and last names being used over the world. Too many sites think my last name is 'Schouwen' instead of 'van Schouwen'.
> ..

>   One way to avoid this would be to explicitly tell the user 
>   A simpler alternative would be to simply display a warning message "A
> malicious site may have fields that are hard to see.  You should only use
> Autofill on sites you trust."  We could show the message the first the the
> user uses the feature, perhaps with a checkbox they can check to never see
> the message again.  Of course, if we take this approach then most users will
> simply check the box and never think about it again.  :)

Such checkboxes don't do anything to protect users, indeed, so try to avoid them. :-)

> - Another security point: Autofill should also never fill in credit card
> information on a page that isn't using HTTPS.

Why not restrict it to HTTPS, period?
Comment 15 Abdullah Alansari 2015-04-30 00:09:37 UTC
Created attachment 302619 [details] [review]
Fix fields filled without user consent and store using libsecret

- A form is now only filled if the user focuses on a field and chooses "Autofill form" from a drop-down list, much like Chrome and Opera, and only empty fields are filled.

- Fill-able forms have a yellow background color.

- All data (personal info, credit card info) are stored using libsecret.

- Almost all the auto-fill logic is in lib/ephy_autofill.{c,h} the only dependency at the moment to epiphany is ephy_web_dom_utils_get_absolute_position_for_element() helper function.

For now there's all sorts of memory leaks and the blocking libsecret functions are used for simplicity.

In addition, the UI needs more work (e.g: a dropdown for year and month instead of a text field).

And for now, fields that are added after the document is loaded are not supported.
This is crucial, many fields are added after the documents is loaded, in the worst case the website can be a single page and the routes are handled using a SPA javascript framework.

I am learning most of this stuff as I go and going through the documentation (GTK+, Webkit), so it could take some time, however luckily, I have no college today (Thursday) so I will be able to get more stuff done.
Comment 16 Michael Catanzaro 2015-04-30 18:50:22 UTC
Hi, next time you update this, can you merge all your patches together and attach just one patch, please, instead of attaching separate patches that build on each other?

(In reply to Abdullah Alansari from comment #15)
> - A form is now only filled if the user focuses on a field and chooses
> "Autofill form" from a drop-down list, much like Chrome and Opera, and only
> empty fields are filled.

That sounds great, but I'm worried that an attacker could simulate this mouse event (e.g. see [1]). That might be a bit tricky to solve; perhaps open a GtkPopover when the user clicks the yellow form, which will be outside the DOM and inaccessible from js.

[1] http://marcgrabanski.com/simulating-mouse-click-events-in-javascript/
Comment 17 Abdullah Alansari 2015-05-11 18:57:02 UTC
Created attachment 303232 [details] [review]
Fix some security issues. Works now on SPA websites.

Michael, thanks for pointing that out.
You can even simulate most events very easily using EventTarget#addEventListener().
Comment 18 Abdullah Alansari 2015-05-18 06:09:25 UTC
Created attachment 303503 [details] [review]
Close the popup when the page is closed or reloaded. Use labels in addition to name attribute.
Comment 19 Abdullah Alansari 2015-06-14 18:39:49 UTC
Created attachment 305251 [details] [review]
revised minimal patch (less than half the original patch size with the same functionality)
Comment 20 Abdullah Alansari 2015-06-14 18:46:27 UTC
The patch is based on the latest code in the repo (June 13th).
In addition changes are distributed across a reasonable number of commits.
Comment 21 Abdullah Alansari 2015-08-19 03:52:31 UTC
Created attachment 309519 [details] [review]
A much more usable and secure version of Autofill

You can change autofill (form filler) settings from the preferences window (Ctrl-E) in General tab under "Autofill".

You can find a demo on YouTube here: https://youtu.be/qV85bcLyxEU

In addition you can find screenshots, a simple flowchart, the patch and the demo itself on Dropbox here:
https://www.dropbox.com/sh/a3u1dope7prmtcc/AAByYMZFFU-jZAYmih-IvCoIa?dl=0

Unfortunately the patch file is too big (almost 23K lines).
For a more accessible use `git diff` against master or just view it on GitHub here: https://github.com/GNOME/epiphany/compare/master...Ahimta:autofill

I've been looking at some commits in Epiphany repo
and it seems that changes as big as this patch are very unusual.
Good thing, it's now only (72 commits, 4K additions, 8 deletions) :)

I also filled the code with comments, so if you find something confusing tell me
so I can make the code comments more clear for everyone's benefit.

Adam I can now see how it took you several months to work on Autofill :).

I've been using Trello while working on this feature you can find the board here: https://trello.com/b/CJiUXbu4
Comment 22 Michael Catanzaro 2015-08-23 19:40:36 UTC
This looks like great work; thanks very much Abdullah! Since this is a big patch, it will probably be a few days before we can review it properly.

A couple things I immediately see:

* I like the use of a GTK+ dialog to prevent Javascript from triggering the form filler, but it doesn't look very good. Maybe we could use an actual GtkPopover instead of a GtkDialog for this.
* The translation changes should be split into a separate patch, so that the can be reviewed separately by the Brazilian Portuguese team.
Comment 23 Abdullah Alansari 2015-08-24 05:44:23 UTC
Created attachment 309904 [details] [review]
remove unnecessary changes in po/POTFILES.in

I removed changes to po/ directory; translators can deal with them using "intltools"

I didn't use GtkPopover, because as I understand it, the main UI resides in a different
process than a web page, which would require IPC to link the GtkPopover with the WebKitWebView.

However, using GtkWindow with GTK_WINDOW_POPUP, GtkWindow::leave-notify-event and GtkWindow::scroll-event
produces excellent results with very simple code.
Comment 24 Michael Catanzaro 2015-09-06 01:16:27 UTC
Hey Abdullah. Sorry for the delay in reviewing this patchset. Could you please make sure they all apply cleanly on master? For example, it'd be nice for me to be able to apply them all in one command by running 'git bz apply 696867'. There seems to be a merge conflict between the fourth and fifth patches:

Applying: fix: ephy_web_dom_utils_get_absolute_position_for_element returns faulty results when using g_object_get()
Using index info to reconstruct a base tree...
M	lib/ephy-web-dom-utils.c
Falling back to patching base and 3-way merge...
Auto-merging lib/ephy-web-dom-utils.c
CONFLICT (content): Merge conflict in lib/ephy-web-dom-utils.c
Failed to merge in the changes.
Patch failed at 0005 fix: ephy_web_dom_utils_get_absolute_position_for_element returns faulty results when using g_object_get()

I fixed it, then the 16th patch failed:

Applying: add: Autofill personal info dialog
fatal: sha1 information is lacking or useless (src/Makefile.am).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0016 add: Autofill personal info dialog

Usually you can fix these by running 'git rebase master' from your branch, which will remove your changes, apply all the changes from master, and then apply all of your changes again, one commit at a time.

Although, for the purposes of Bugzilla review, it would probably be easiest to squash all 72 patches into one single patch. That would probably dramatically reduce the size of the patch, as well. It is a bit intimidating to see 20000 lines of changes to review, but I'm sure most of that is sequential improvements over the same points in the code, so it should be much smaller with a patch that contains only the difference between the original commit and the final commit.
Comment 25 Abdullah Alansari 2015-09-06 14:36:57 UTC
Created attachment 310753 [details] [review]
A one-commit patch re-based against master

Michael, sorry for the trouble, I made many careless changes (especially in the beginning).

The patch is now almost 4600 lines.
According to GitHub the patch introduces 4002 additions and 0 deletions.
You can view the diff on GitHub here:
https://github.com/GNOME/epiphany/compare/master...Ahimta:autofill-rebased
Comment 26 Michael Catanzaro 2015-09-28 02:28:54 UTC
Review of attachment 310753 [details] [review]:

Sorry again for the delay on this. Big reviews are intimidating for reviewers, and this is by far the biggest contribution we've received in a long while! I have a long review, but only because it's a big patch. Fortunately your code is quite good, especially for a new contributor; it's clear that you took time learning our conventions, and that helps a lot.

Do you have a website or HTML file you use for testing this? I haven't tried it out yet; just took a first look over the code....

The new autofill preferences dialogs don't need the save and cancel buttons. They can be an instant-apply presentation dialog, with just the normal X close button: https://developer.gnome.org/hig/stable/dialogs.html.en

::: data/org.gnome.epiphany.gschema.xml
@@ +194,3 @@
 			<description>Whether to block the embedded advertisements that web pages might want to show.</description>
 		</key>
+		<key type="b" name="enable-autofill-personal-info">

I am not sure if we actually need these settings? I think if the user has filled in personal data in the preferences dialog, it should be autofilled, and otherwise, not.

This means you can get rid of the toggle buttons in the preferences dialog, I think.

::: embed/web-extension/Makefile.am
@@ +21,3 @@
 	$(top_srcdir)/embed/uri-tester.h \
+	$(top_srcdir)/lib/ephy-autofill.c \
+	$(top_srcdir)/lib/ephy-autofill.h \

You'll find this has a merge conflict with master. Now the web extension is linked to libephymisc.la, and you don't have to list the files here anymore.

::: embed/web-extension/ephy-web-extension.c
@@ +1128,3 @@
+   * then this page/tab will have Autofill functionality, otherwise,
+   * this page won't have Autofill functionality, even if you enabled it
+   * and refreshed the page; It will only work on newly created pages/tabs.

Nit: use a lowercase 'i' in "it."

@@ +1134,3 @@
+   */
+  if (is_autofill_personal_info_enabled || is_autofill_card_info_enabled)
+    g_signal_connect (web_page, "document-loaded",

Nit: put brackets around the conditional body when it requires multiple lines, even though it's just one statement, to improve readability. (Unless the surrounding code is not doing this.)

::: lib/Makefile.am
@@ +57,3 @@
 libephymisc_la_SOURCES = \
+	ephy-autofill.c				\
+	ephy-autofill-dom.c			\

Since there are so many files, I think it would be ideal to move these to a lib/autofill subdirectory.

::: lib/ephy-autofill-dom.c
@@ +46,3 @@
+ * ephy_autofill_dom_web_page_document_loaded:
+ * @web_page: a #WebKitWebPage
+ * @user_data: (allow-none): data passed to the callback. Not used currently

You don't need to mark user_data as (allow-none), by convention.

::: lib/ephy-autofill-form-element.c
@@ +46,3 @@
+    bool is_autofill_personal_info_enabled = g_settings_get_boolean (EPHY_SETTINGS_WEB,
+                                                                EPHY_PREFS_WEB_ENABLE_AUTOFILL_PERSONAL);
+    bool is_autofill_card_info_enabled = g_settings_get_boolean (EPHY_SETTINGS_WEB,

Some whitespace errors error.

@@ +76,3 @@
+           bool allow_credit_card_info)
+{
+  if (is_valid_form (form)) {

When you have the entire body of the function inside a conditional statement like this, it's better to invert the condition and return early, so that you have one less level of indentation throughout the rest of the function.

@@ +78,3 @@
+  if (is_valid_form (form)) {
+    WebKitDOMHTMLCollection *elements = webkit_dom_html_form_element_get_elements (form);
+    long length = webkit_dom_html_collection_get_length (elements);

unsigned long

@@ +93,3 @@
+                                          personal_info_enabled,
+                                          credit_card_info_enabled && allow_credit_card_info);
+

No blank line before the else if, please.

@@ +111,3 @@
+
+  if (is_valid_form (form)) {
+    // Did the user agree to fill credit card info

We only use /* old-style comments, */ though maybe it's time to consider allowing the // style, as there's no way we care about any compiler that can't handle // comments anymore.

@@ +158,3 @@
+ephy_autofill_form_element_prepare (WebKitDOMHTMLFormElement *form)
+{
+  if (is_valid_form (form)) {

Return early here, too

@@ +160,3 @@
+  if (is_valid_form (form)) {
+    WebKitDOMHTMLCollection *elements = webkit_dom_html_form_element_get_elements (form);
+    long collection_length = webkit_dom_html_collection_get_length (elements);

unsigned long

@@ +177,3 @@
+                                             G_CALLBACK (input_dbclick_cb),
+                                             NULL);
+

No blank line

::: lib/ephy-autofill-input-element.c
@@ +47,3 @@
+  if (ephy_autofill_matchers_is_fullname (element_key))
+    return EPHY_AUTOFILL_FIELD_FULLNAME;
+

Omit the blank lines

::: lib/ephy-autofill-matchers.c
@@ +49,3 @@
+// Regexes below are inspired by Chromium Autofill implementation.
+// You can find them here:
+// https://chromium.googlesource.com/chromium/src.git/+/master/components/autofill/core/browser/autofill_regex_constants.cc

Why not add the localized strings from Chromium, as well?

@@ +80,3 @@
+static const char CARD_TYPE_PATTERN[] = "debit.*card|(card|cc).?type";
+
+static GHashTable *regexes = NULL;

We should free this, as well as the regexes in it, else we're doomed to complaints from valgrind and asan. I will have to think about the best way to do this... I guess we need to either somehow tie it to the lifetime of the web extension (maybe by making this a RegexManager object, owned by the EphyWebExtension? but then it could not live in the lib directory), or else use an exit handler (yuck!).

Hm, this needs more thought. Don't worry about it for now, unless you see an easy way to handle this.

::: lib/ephy-autofill-select-element.c
@@ +130,3 @@
+                 const char *name)
+{
+  long select_element_length;

unsigned long

@@ +132,3 @@
+  long select_element_length;
+
+  g_object_get (select_element, "length", &select_element_length, NULL);

No need for this, you can use webkit_dom_html_select_element_get_length()

@@ +200,3 @@
+
+  if (field != EPHY_AUTOFILL_FIELD_UNKNOWN)
+    ephy_autofill_storage_get (field, (GAsyncReadyCallback)fill_cb, select_element);

I prefer to omit the cast; if the declaration is the same type as expected, you don't need a cast, and if it's not the same type, that's something to fix!

::: lib/ephy-autofill-ui.c
@@ +34,3 @@
+#define MAX_OPEN_POPUPS_PER_PAGE 1
+
+static volatile unsigned int current_open_popups_per_page = 0;

Why is this declared volatile? (It surely shouldn't be!)

@@ +43,3 @@
+{
+  WebKitDOMDocument *dom_document = webkit_dom_node_get_owner_document (dom_node);
+  WebKitDOMDOMWindow *dom_window = webkit_dom_document_get_default_view (dom_document);

get_default_view is transfer full, so you need to unref this.

@@ +205,3 @@
+  if (current_open_popups_per_page < MAX_OPEN_POPUPS_PER_PAGE) {
+    long element_height = webkit_dom_element_get_offset_height (WEBKIT_DOM_ELEMENT (input_element));
+    long element_width = webkit_dom_element_get_offset_width (WEBKIT_DOM_ELEMENT (input_element));

Hm, the functions return double, you store them in longs, and you pass them to a function expecting an int... I think int is fine here.

@@ +215,3 @@
+    window = create_popup (element_left, element_top, element_width, element_height, form, window_accept_cb);
+
+    g_object_weak_ref (G_OBJECT (form), (GWeakNotify)form_unrefed_cb, g_object_ref (G_OBJECT (window)));

g_object_ref receives a gpointer, so that you don't have to cast it with G_OBJECT. And again, it's best to not cast the function.

::: lib/ephy-autofill-utils.c
@@ +48,3 @@
+  if (key == NULL)
+    return FALSE;
+  else {

No need for the else after the return.

::: lib/ephy-autofill.c
@@ +33,3 @@
+ *
+ * Long story short, Unless you are fixing/improving Autofill
+ * You shouldn't concern your self with other Autofill files

lowercase u in Unless, and lowercase y in You

::: lib/ephy-web-dom-utils.c
@@ +3,3 @@
 /*
  *  Copyright © 2013 Igalia S.L.
+ *  Copyright © 2015 Abdullah Alansari

In the end, you must have removed all your changes in this file, so you should remove the added copyright here.

::: src/prefs-autofill-card-dialog.h
@@ +52,3 @@
+};
+
+GType prefs_autofill_card_dialog_get_type  (void);

Bonus points if you delete all of this and switch to G_DECLARE_FINAL_TYPE. For example: https://git.gnome.org/browse/epiphany/commit/?h=wip/modern_gobject&id=c2c6b19eb03408ba997c7f8982ce97d1084e2fc1

::: src/prefs-autofill-personal-dialog.c
@@ +46,3 @@
+};
+
+G_DEFINE_TYPE_WITH_PRIVATE (PrefsAutofillPersonalDialog, prefs_autofill_personal_dialog, GTK_TYPE_DIALOG);

If you upgrade to G_DECLARE_FINAL_TYPE, you would need to use plain G_DEFINE_TYPE here (since the instance struct would be moved out of the header, you won't need the private struct anymore).

@@ +165,3 @@
+prefs_autofill_personal_dialog_dispose (GObject *object)
+{
+  G_OBJECT_CLASS (prefs_autofill_personal_dialog_parent_class)->dispose (object);

No need to override dispose, since you don't have anything to unref in here.

::: src/prefs-autofill-utils.c
@@ +60,3 @@
+prefs_autofill_utils_set_free_cb (GObject *source_object,
+                                  GAsyncResult *res,
+                                  char *value)

If you use a gpointer for the final parameter, and cast it to a char * in the function body, then you don't need to use the (GAsyncReadyCallback) casts all over the place, and the compiler would warn you if you got the declaration wrong.

::: src/prefs-dialog.c
@@ +151,3 @@
+                                             PrefsDialog *prefs_dialog)
+{
+	PrefsAutofillCardDialog *autofill_dialog = g_object_new (EPHY_TYPE_PREFS_AUTOFILL_CARD_DIALOG,

It's customary to provide an prefs_autofill_card_dialog_new() function to wrap g_object_new, so you don't have to use it manually here.

@@ +163,3 @@
+                                                 PrefsDialog *prefs_dialog)
+{
+	PrefsAutofillPersonalDialog *autofill_dialog = g_object_new (EPHY_TYPE_PREFS_AUTOFILL_PERSONAL_DIALOG,

same

::: src/resources/prefs-autofill-card-dialog.ui
@@ +1,2 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!-- Generated with glade 3.19.0

Wow, you must have been persistent. I've never managed to get far with Glade before giving up on it....
Comment 27 Michael Catanzaro 2015-09-28 02:29:29 UTC
I forgot to say: thanks a bunch for this big contribution. :)
Comment 28 Abdullah Alansari 2015-09-28 03:05:47 UTC
It's thanks to Adam for supporting this issue on Bountysource https://www.bountysource.com/issues/1325823-form-filler

Otherwise, I wouldn't have been able to work on an issue of this size.

For testing, I downloaded a bunch of HTML files, to use offline, from: Amazon, Ebay, PayPal, Apple, GitHub, LinkedIn, Twitter, etc...

I didn't add the localized string for simplicity. I thought it reasonable to delay it to future versions.

Anyways, I will try to fix the issues you mentioned above in a week or so (hopefully).
Comment 29 Abdullah Alansari 2015-09-28 03:07:44 UTC
And then I will rebase the whole thing against the latest Epiphany upstream.
Comment 30 Abdullah Alansari 2015-10-01 13:41:50 UTC
Created attachment 312478 [details] [review]
Fix some styling issues; one-commit patch rebased against master

Hi, Michael. I fixed the issues you illustrated above, with a few exceptions.

1. I kept toggle buttons to make Autofill disabled by default, so its behavior or
bugs (if ever :)) won't affect users who didn't enable it or knew it even existed.
For that purpose I could have added only one, but then the UI could be more cluttered, and having the ability to disable both (personal and card) is nice,
especially credit card info (disclaimer: I'am not a UI expert -_-).

2. I still didn't add support for non-english matching, this mostly apply to placeholders and labels.
Aside from laziness, it's not as easy to test other languages and most of the matching is from name and id attributes, and I've never seen a website
using a language other than english for those.

Finally, I moved Autofill files from lib/ to lib/autofill, added a Makefile.am
and made the necessary changes to other Makefile.am files and configure.ac.

I did that mostly by looking at Makefile.am files in the project and
studying the similarities and differences (didn't find enough info in Automake manual -_-).
I also took full advantage of "make changes until it's no longer broken" principle :), which was immensely helpful.
Comment 31 Abdullah Alansari 2015-10-01 13:48:44 UTC
In addition, Autofill dialogs still use an explicit apply.
Mainly, because changes are saved by making about a dozen async secret lib calls,
and I don't know of an easy way to make them use an implicit apply.
Comment 32 Michael Catanzaro 2015-10-01 15:39:03 UTC
OK, great. So I think the code is fine, but the UI will need some more work with the designers. Next step: please take screenshots of the new UI (the first page of the preferences dialog, the two new preferences dialogs, the actual pop-up that appears on a web page when autofilling) and preferably also get a screencap of the autofill in actual using Ctrl+Alt+Shift+R (which will record your desktop for 30 seconds, or until you hit that key combo again). Attach all that to this bug. Then I will ask some of our designers to visit this bug and provide feedback.
Comment 33 Abdullah Alansari 2015-10-01 16:14:00 UTC
You can find the screencap, the screenshots, the patch and more on Dropbox here:
https://www.dropbox.com/sh/a3u1dope7prmtcc/AAByYMZFFU-jZAYmih-IvCoIa?dl=0

You can also find the screencap on YouTube here: https://youtu.be/qV85bcLyxEU
Comment 34 Diogo Campos 2015-10-01 20:02:37 UTC
Created attachment 312526 [details]
AutoFill mockup v1

This is my suggestion.

Since this has already been implemented, I tried to change the minimal possible.

Also, I don't know if relevant, but, I should note that 'multiple credit cards' and 'multiple personal informations' aren't being implemented.
Comment 35 Michael Catanzaro 2015-10-01 20:24:52 UTC
Thanks for the mockup! I am a bit concerned about buttons that appear and disappear depending on state... normally we prefer to make buttons insensitive instead, to avoid possibly-confusing UI changes. In this case, I think we really only need the one Edit button, and not the Add/Remove buttons as well. We can have a Remove button in the header bar of the dialog that gets opened.

Regarding the popover... I do agree it would be ideal to have a popover, rather than a GtkWindow. I think Abdullah had trouble getting that to work, but maybe he can figure it out. If it's too hard to get the popover working, we could try a GtkMessageDialog (with no header bar, and buttons across the bottom) instead of the dialog that's there now.

(In reply to Abdullah Alansari from comment #30)
> 1. I kept toggle buttons to make Autofill disabled by default, so its
> behavior or
> bugs (if ever :)) won't affect users who didn't enable it or knew it even
> existed.

I still don't understand why we need these options, though. Surely if you simply do not fill in any of the data, then it's effectively off, right? :)
Comment 36 Diogo Campos 2015-10-01 20:44:12 UTC
(In reply to Michael Catanzaro from comment #35)
> Thanks for the mockup! I am a bit concerned about buttons that appear and
> disappear depending on state... normally we prefer to make buttons
> insensitive instead, to avoid possibly-confusing UI changes. In this case, I
> think we really only need the one Edit button, and not the Add/Remove
> buttons as well. We can have a Remove button in the header bar of the dialog
> that gets opened.

Sure, 'Remove' can be placed inside the 'Edit' Window.
But, the button that only changes the label ('Add...' --> 'Edit...') is really undesirable?

> Regarding the popover... I do agree it would be ideal to have a popover,
> rather than a GtkWindow. I think Abdullah had trouble getting that to work,
> but maybe he can figure it out. If it's too hard to get the popover working,
> we could try a GtkMessageDialog (with no header bar, and buttons across the
> bottom) instead of the dialog that's there now.

Fine by me.

> (In reply to Abdullah Alansari from comment #30)
> > 1. I kept toggle buttons to make Autofill disabled by default, so its
> > behavior or
> > bugs (if ever :)) won't affect users who didn't enable it or knew it even
> > existed.
> 
> I still don't understand why we need these options, though. Surely if you
> simply do not fill in any of the data, then it's effectively off, right? :)

Just to confirm, the 'thinking' in the mockup is:
    - If the user added info, popover will appear.
    - If no info was added, popover will not appear.
    - So, there is no need for a Checkbox/Switch.
Comment 37 Diogo Campos 2015-10-01 21:23:41 UTC
Created attachment 312528 [details]
AutoFill mockup v2

Changelog:

Buttons consistency.
Comment 38 Diogo Campos 2015-10-01 21:26:55 UTC
(In reply to Diogo Campos from comment #37)
> Created attachment 312528 [details]

*sigh*
Ignore the 'Changes to Edit after added' phrase in the mockup.
Comment 39 Michael Catanzaro 2015-10-01 22:56:44 UTC
(In reply to Diogo Campos from comment #36)
> Sure, 'Remove' can be placed inside the 'Edit' Window.
> But, the button that only changes the label ('Add...' --> 'Edit...') is
> really undesirable?

Why change the label? Just always leave it as Edit.
Comment 40 Diogo Campos 2015-10-01 23:26:41 UTC
Created attachment 312536 [details]
AutoFill mockup v2 (B)

Changelog:

Centralized workflow in 1 button.
Comment 41 Diogo Campos 2015-10-01 23:47:52 UTC
Also, would not be better to switch the Popover/FloatingThing for a bottom (or top) bar type this?


------------------------------
HEADER BAR
------------------------------
TABS
------------------------------
CONTENT
CONTENT
CONTENT
CONTENT
------------------------------
Forms in this page can be automatically filled. Do you want? [Cancel] [Fill]
------------------------------
Comment 42 Diogo Campos 2015-10-01 23:49:35 UTC
(In reply to Diogo Campos from comment #41)
> Also, would not be better to switch the Popover/FloatingThing for a bottom
> (or top) bar type this?

"bar like this?", I mean.
Comment 43 Michael Catanzaro 2015-10-02 02:13:59 UTC
Hey Adam, if you're still following this, your input would be dandy.

Anyway, maybe:

------------------------------
Forms on this page can be automatically filled with your personal information. [Don't fill] [Fill]
------------------------------

and

------------------------------
Forms on this page can be automatically filled with your credit card number and other personal information. [Don't fill] [Fill]
------------------------------

I thought about using an info bar (though we always put that on the top of pages, not the bottom... maybe moving it would be a good idea, though!) but my worry is that it's a lot of mouse movement, and it's a bit divorced from the actual form itself. But, I'm also worried that it could be rather hard to get the popover looking good, a problem we simply do not have to worry about with the info bar. So I'm fine with either approach.

P.S. The button in the popover should specify which type of data to filled: "Fill with postal code," for example.

P.S.S. The new preferences dialogs should really be presentation dialogs: red Clear All button in the upper-left, and just a close X button in the upper right. Abdullah, you can simply start the async libsecret calls when the dialog is closed.
Comment 44 Diogo Campos 2015-10-02 03:10:49 UTC
(In reply to Michael Catanzaro from comment #43)
> ------------------------------
> Forms on this page can be automatically filled with your personal
> information. [Don't fill] [Fill]
> ------------------------------

Good.

Which brings us to another question: 'Credit Card' is also a 'Personal Information', so, could it be managed in the same window as 'Personal Information'?

> I thought about using an info bar (though we always put that on the top of
> pages, not the bottom... maybe moving it would be a good idea, though!)

I thought on bottom because when on top the content is moved down. But maybe this is a good thing, to call attention. I'm not sure. Both are fine to me.

> but
> my worry is that it's a lot of mouse movement, and it's a bit divorced from
> the actual form itself. But, I'm also worried that it could be rather hard
> to get the popover looking good, a problem we simply do not have to worry
> about with the info bar. So I'm fine with either approach.
> 
> P.S. The button in the popover should specify which type of data to filled:
> "Fill with postal code," for example.

If the implementation can do this, sure.
But the user would need to click in every field.
Maybe two buttons, then: one to fill the clicked field, and another to fill all fields.

> P.S.S. The new preferences dialogs should really be presentation dialogs:
> red Clear All button in the upper-left, and just a close X button in the
> upper right.

But in all the other 'Manage' dialogs (History, Cookies and Passwords), the content is automatically generated by the browser, and the user can only delete (some or all) entries.

On the other hand, in this case, the content is manually created by the user, and he can change the values later, so, I still think that makes more sense a 'Add/Edit/Remove' window and workflow (be it with 3 or 1 buttons).

However, both should work, anyway.
Comment 45 Michael Catanzaro 2015-10-02 14:08:13 UTC
(In reply to Diogo Campos from comment #44)
> Good.
> 
> Which brings us to another question: 'Credit Card' is also a 'Personal
> Information', so, could it be managed in the same window as 'Personal
> Information'?

Oh yeah, of course, I completely forgot to mention that.

> If the implementation can do this, sure.
> But the user would need to click in every field.
> Maybe two buttons, then: one to fill the clicked field, and another to fill
> all fields.

I was thinking about this too. The problem is that if the popover originates from a particular field, it would be bogus to fill in other fields automatically, so you would have to click on each field. But that would be pretty lousy. An infobar avoids that problem.

> > P.S.S. The new preferences dialogs should really be presentation dialogs:
> > red Clear All button in the upper-left, and just a close X button in the
> > upper right.
> 
> But in all the other 'Manage' dialogs (History, Cookies and Passwords), the
> content is automatically generated by the browser, and the user can only
> delete (some or all) entries.
> 
> On the other hand, in this case, the content is manually created by the
> user, and he can change the values later, so, I still think that makes more
> sense a 'Add/Edit/Remove' window and workflow (be it with 3 or 1 buttons).
> 
> However, both should work, anyway.

We should only have an explicit-apply dialog if there is some technical reason it's needed, say because the settings have to be changed all at once, or validated against each other. In this case, it's not needed. (Explicit-apply creates problems with closing the dialog with the Cancel button, since you have to remember what the state of the dialog was before you entered. With the instant-apply pattern, the user has no question what the state of the dialog will be after it's closed: it's exactly what he sees on the screen. So instant-apply is easier to use, and less complicated -- fewer buttons! -- thus preferred.)
Comment 46 Diogo Campos 2015-10-02 15:37:00 UTC
Created attachment 312576 [details]
AutoFill mockup v3
Comment 47 Michael Catanzaro 2015-10-02 15:56:00 UTC
Comment on attachment 312576 [details]
AutoFill mockup v3

OK, this looks good to me. Abdullah, what do you think?
Comment 48 Abdullah Alansari 2015-10-02 16:17:13 UTC
The UI looks good. I am not sure about the behavior, though.

It seems to me, from the UI that the user would focus on an input,
then the info bar would appear, and the user would fill the whole form by clicking "Fill".

So in this case, how is credit card fields handled?
Do they just get filled without the user consent?

The main concern here is that an attacker could have some credit card fields
on the page, that are not visible to the user.
Comment 49 Michael Catanzaro 2015-10-02 17:31:16 UTC
I was thinking that the infobar would appear when the page is loaded for any page with the forms (and it would never appear again on that page if the user says don't fill), rather than on focus. Then the info bar shows whether credit card data will be filled in or not.

If we fill each field on focus, the concern is that's a lot of clicking on different fields. But I'm OK with that approach too, if we can make the popover look good.
Comment 50 Diogo Campos 2015-10-02 21:25:55 UTC
(In reply to Abdullah Alansari from comment #48)
> The main concern here is that an attacker could have some credit card fields
> on the page, that are not visible to the user.

This can happen with any method of "Fill All", no?

If yes, the same two solutions comes to my mind:
    1. Warn that Credit Card info will be inserted.
    2. Only fill individually, on focus.

And I think "1" is the best.

(In reply to Michael Catanzaro from comment #49)
> I was thinking that the infobar would appear when the page is loaded for any
> page with the forms

...any page with auto-fillable forms.

>(and it would never appear again on that page if the
> user says don't fill).

Was not my thinking...

I think this behavior will need, at least, an extra Button in the Infobar (to better inform), and a extra (three-state) Checkbox in the Preferences (to be possible to revert the choice), like this:

[Never For This Site] [Not Now] [Fill]

and

[x] Ask to automatically fill forms with my personal data.
    (yes for all sites - default).
[-] Ask to automatically fill forms with my personal data.
    (no for some sites - when clicked at least once in "Never For This Site").
[ ] Ask to automatically fill forms with my personal data.
    (no for all sites).
Comment 51 Diogo Campos 2015-10-02 23:08:50 UTC
Created attachment 312593 [details]
AutoFill mockup v4
Comment 52 Abdullah Alansari 2015-10-03 04:37:04 UTC
You can't, in any given moment, have access to all input fields and
forms of a page.

Because, the content of a page can change dynamically
using scripts (e.g: Javascript).
And these dynamic changes can happen a lot. Some websites are only one page
that changes dynamically using scripts (e.g: Javascript).

For example all Bountysource pages are actually one HTML page,
and the content relevant to the URL get filled near "ng-view".
And changes in the URL (e.g: when you click to view an issue) don't request a
new page, but change the existing page (near "ng-view" as well).
You can see that by downloading different pages using "curl".

Long story short, when you browse through different pages in Bountysource,
from the web browser's perspective, you're only viewing a single page.

The current behavior of Autofill is inspired by Chromium's.
The idea is when the user focuses on an input, we highlight fillable fields,
and then when the user double clicks on a fillable field, we show them a menu
to ask them what to do ("Fill personal info", "Fill card info").
If the user clicks either choice, we fill the appropriate info for the whole form.
Otherwise if the user scrolls the page, or clicks outside the menu, the menu closes.

This behavior actually has a touch of genius.
It takes into account many technical details, and significantly reduces
the execution paths (simplify implementation), and still delivers good UX.
I suspect that's why both Chromium and Opera are using this approach
(or they share the same implementation :)).

As for multiple data for the same field in the prefs dialog (as in Chromium),
I think, it shouldn't be implemented for the first Autofill release (if ever).
And in the future if anyone asks for it (if ever), we could consider it.
But then we must take into account how much useful it is to be added and how
hard it is to implement.

Finally, I think we should go with mockup v4, using a popover, or a menu
with two entries ("Fill Personal Info", "Fill Credit Card Info")
and the menu would disappear when the user clicks outside it, or scrolls the page.
Comment 53 Adam Dingle 2015-10-04 15:41:59 UTC
(In reply to Michael Catanzaro from comment #43)
> Hey Adam, if you're still following this, your input would be dandy.

I am still following this, and am happy to see all the progress!  Mockup v4 looks pretty good to me. From Abdullah's last comment it seems like the infobar might not work since JavaScript can add and remove fields on the fly - if that's true we need consider option 1 (the context menu) or option 2 (the popover).

Either of these would be fine with me, though it seems annoying to have to take two actions to fill out a single form: one for personal data and one for credit card data.  If we use the context menu, maybe it could have

  Fill this
  Fill all with personal data
  Fill all with personal and credit card data

The last option could appear only when the form actually has credit card fields.

Or, for the popover, if the form has credit card data then the top of the popover could read

  Some fields can be automatically filled with your personal and credit card data.
  [Fill This]
  [Fill All]
  [Fill None]

Then I'd be able to fill with a single click on Fill All.

The popover seems a little nicer, though it is also potentially annoying if it appears automatically and the user doesn't actually want to use autofill.  Given that, I'm somewhat neutral about whether to use a context menu or popover.
Comment 54 Diogo Campos 2015-10-04 22:11:33 UTC
(In reply to Adam Dingle from comment #53)
> The popover seems a little nicer, though it is also potentially annoying if
> it appears automatically and the user doesn't actually want to use autofill.

That's the reason for the 'Fill None' button, it's a 'don't show me again' kind of action (can be renamed, though).

And additionally, the popover should not appear if the user has not added data.
Comment 55 Michael Catanzaro 2015-10-07 21:00:56 UTC
We also need to make sure not to add empty secrets to the keyring, and delete them when cleared, rather than leaving them present in the keyring but blank.
Comment 56 Abdullah Alansari 2015-10-10 13:57:31 UTC
Hi, Michael. Sorry for the delay!

I think we mostly agree on:
1. Merge both preferences dialogs into one.
2. Personal and card info are always enabled (remove the settings from schema).
3. Don't keep empty secrets in the keyring.

As for how to fill the form, I think the context menu makes more sense.

I am no expert on WebKit, but I think the popover would require inter-process
communication, like the way Epiphany handles closing tabs with modified forms.
In the case of modified forms there's no obvious way to do it otherwise.

And I honestly can't see any advantage it has over the context menu.
Quite the opposite, the context menu seems cleaner and less intrusive.
Comment 57 Michael Catanzaro 2015-10-10 15:14:23 UTC
(In reply to Abdullah Alansari from comment #56)
> Hi, Michael. Sorry for the delay!
> 
> I think we mostly agree on:
> 1. Merge both preferences dialogs into one.
> 2. Personal and card info are always enabled (remove the settings from
> schema).
> 3. Don't keep empty secrets in the keyring.
> 
> As for how to fill the form, I think the context menu makes more sense.

OK, that's all fine by me.

> I am no expert on WebKit, but I think the popover would require inter-process
> communication, like the way Epiphany handles closing tabs with modified
> forms.

Probably. This is not hard though, especially not compared to the work you've already done. See EphyWebExtensionProxy, the UI process object that communicates with the web process.

> In the case of modified forms there's no obvious way to do it otherwise.
> 
> And I honestly can't see any advantage it has over the context menu.
> Quite the opposite, the context menu seems cleaner and less intrusive.

Well the one advantage is that a popover could appear automatically, whereas if you don't realize you can right-click to get the form filler in the context menu, you'll never find it. The context menu is less discoverable.

But like I said, I'm fine with the context menu approach. Go ahead and implement whichever you prefer. :)
Comment 58 Abdullah Alansari 2015-10-27 15:30:07 UTC
Created attachment 314237 [details] [review]
Mockup v4 implementation

Hi, just finished the implementation of mockup v4.

The patch contains two commits:
1. All previous changes.
2. All new changes.

If you think it is better to have a single commit, I will gladly make the
appropriate changes.

In addition you can find the demo on YouTube here: https://youtu.be/XzJFbSxn4po
And you can find the screenshots and the demo itself on Dropbox here:
https://www.dropbox.com/sh/a3u1dope7prmtcc/AAByYMZFFU-jZAYmih-IvCoIa?dl=0

Note also that the patch is rebased against upstream (27 Oct 2015)
And yes I ended up using IPC :)
Comment 59 Michael Catanzaro 2015-11-01 13:36:55 UTC
(Just a comment to let you know I haven't forgotten this, just need to find time for a review. The video looks good, so I think we're nearing the end!)
Comment 60 Michael Catanzaro 2015-12-09 10:50:51 UTC
Created attachment 317011 [details] [review]
Add form autofill

This is Abdullah's patch, rebased.
Comment 61 Michael Catanzaro 2015-12-09 14:16:48 UTC
Comment on attachment 317011 [details] [review]
Add form autofill

(obsoleting only to get this off my personal patch tracker)
Comment 62 Michael Catanzaro 2015-12-09 14:25:14 UTC
OK, sorry again for the delay on this review, Abdullah. We had a meeting today about your patch. We tested the patch on https://tools.usps.com/go/ScheduleAPickupAction!input.action but were not able to see it working properly due to a crash in the UI process:

ERROR:ephy-autofill-context-menu.c:270:ephy_autofill_context_menu_fill: assertion failed: (WEBKIT_DOM_IS_HTML_SELECT_ELEMENT (element) || WEBKIT_DOM_IS_HTML_INPUT_ELEMENT (element) || WEBKIT_DOM_IS_HTML_FORM_ELEMENT (element))

We looked over the preferences dialog changes, the yellow used to mark fillable-forms, and the context menu. We agreed on a few points:

* The yellow bar doesn't look very good as a way to determine what content should be autofilled. We expect we'll be receiving bug reports about the strange color, and what would happen if the form already had a background color set by the page?
* It's awkward that one context menu entry autofills the entire form, rather than a single from element.

We would rather display an info bar instead, asking whether to autofill personal information on the page. That would avoid the need to mark individually which form elements to autofill with the yellow bar, and avoid the need to modify the context menu.

Also, we agreed Carlos will give a more detailed review of the code in your patch, like I did for your earlier revision.
Comment 63 Michael Catanzaro 2015-12-09 14:41:25 UTC
(In reply to Abdullah Alansari from comment #52)
> You can't, in any given moment, have access to all input fields and
> forms of a page.
> 
> Because, the content of a page can change dynamically
> using scripts (e.g: Javascript).
> And these dynamic changes can happen a lot. Some websites are only one page
> that changes dynamically using scripts (e.g: Javascript).

By the way, we're fine with the infobar appearing after the page has already loaded, when the user focuses the form... maybe it's not the cleanest design possible, but I think it'd be a step up from the current implementation, and would be good enough to ship.
Comment 64 Abdullah Alansari 2015-12-09 18:30:54 UTC
Thank you all for taking the time to review my patch;
It seems much bigger of a deal than I thought :)

> OK, sorry again for the delay on this review, Abdullah. We had a meeting today about your patch. We tested the patch on https://tools.usps.com/go/ScheduleAPickupAction!input.action but were not able to see it working properly due to a crash in the UI process:

I couldn't make out what the patch test page really is about.

Regarding the UI process crash, I think the real issue is that there are a lot of
wrong assumptions in Autofill, especially about parameters.
One way to mitigate this issue, is to have exported functions (in *.h files)
expect literally anything, and return gracefully, when inputs are invalid.

The way I understand it, you want the same behavior as "AutoFill mockup v4"
(https://bugzilla.gnome.org/attachment.cgi?id=312593&action=edit)
with the infobar, except for:

* Fillable elements aren't changed (background color set to yellow).
* The infobar may appear more than once.
* Fill the whole form (when "Fill" pressed), nothing otherwise.

Finally, I'd like to note that I have 5 exams this semester,
from (20th Dec 2015) to (3rd Jan 2016).

I've already started studying, but I can't know for sure if I will be able to work on this before I finish all my exams (3rd Jan 2016); Sorry about that :).
Comment 65 Michael Catanzaro 2015-12-10 09:21:39 UTC
Please only show the info bar once per page! After the first time, you can assume the user consents to sharing personal info.

Also, I recommend waiting until after your exams to work on this. No rush; good luck with them. :)
Comment 66 Abdullah Alansari 2016-01-12 06:53:55 UTC
Hi, Michael. Sorry for the delay I just started a side project for fuel prices on Saudi Arabia :)

I'd like first to say that I strongly think that the infobar is not fit for Autofill because:
* The context of Autofill is forms and inputs; The infobar is far from them, which requires extra cognitive effort to relate it to the actual context.
* This is perfect for Form Submission, since its context is the current website.
* The more we use the infobar, the less users pay attention to it. We can add an option to never show the infobar for Autofill for this site/page, but still some users would have to do this for a lot of pages/sites.
* Highlighting fields isn't perfect, but still a great way to tell users what fields can be filled before they autofill the form, especially if they're already filled; Otherwise it will be harder to know which fields are fillable and impossible unless the user autofills the form.
* All this may lead to irritating experience for users who don't want to use Autofill, and sub-par experience for users who do.
* Autofill adds as little entries to the context menu as possible; It only adds entries when it can autofill something, nothing otherwise.

So, here's my proposal:
* Keep the current implementation using the context menu, and make it more fault-tolerant (as I mentioned before).
* If you find it important to make the context menu as short as possible, make Autofill disabled by default.
* If you find that highlighting may cause bug reports in the future (which it will :)), then make it disabled by default, but still keep it as an option with a note that it may not work properly with all pages; It's too useful to not be available.

I'd also like to say that enabling a feature by default can put its implementation in a compromising position, since you want to provide good experience to users who want to use the feature and still maintain a good experience for those who don't.

I know that you may well have already considered this option and deemed it unfit.
Regardless, please do tell me what you think and I will go ahead and implement whatever you decide is best.
Comment 67 Michael Catanzaro 2016-01-12 15:50:21 UTC
I think it's time to get some help from the GNOME design team; this is a tricky problem. Abdullah, would you mind contacting them to ask for help with this? They have a web page here: https://wiki.gnome.org/Design
Comment 68 Abdullah Alansari 2016-01-13 08:39:34 UTC
I will try to read as much as I can of the design wiki relevant to this issue, then contact the design team.
Comment 69 Abdullah Alansari 2016-02-06 08:39:20 UTC
Hi. First and formost, sorry for the delay.

I contacted the design team a couple of times on the IRC channel to no avail.
Honestly, I'm not really sure how this works.

Regardless, this issue is always at the back of mind, and I would really like to wrap it up.

So please try to resolve the design issues so I can finish it up; I want to compile WebKit for one final time :)
Comment 70 Michael Catanzaro 2016-02-06 15:51:50 UTC
Hm, perhaps you could investigate how Chrome handles this?
Comment 71 Abdullah Alansari 2016-02-07 08:49:26 UTC
Good point. I will play with different browsers (Opera, Chrome, Firefox) and dig deeper in the source code (Chromium, Blink) and see how it can be of use for Epiphany's Autofill.
Comment 72 Abdullah Alansari 2016-02-09 15:41:20 UTC
I just finished skimming through Chromium source code. Unfortunately, I couldn't find the exact portion of code that shows the popup UI (too many layers of abstraction probably for a good reason and my not so great knowledge of C++).

I then went ahead and experimented a little bit with Chrome, and it seems the popup is not using the DOM (for obvious reasons).

I think the closest thing to Chrome's implementation is GtkPopover.

It appears that you aren't the biggest of fans for the context menu implementation, so what do you think is most appropriate for now, Popover, Infobar, or something else?
Comment 73 Michael Catanzaro 2016-02-09 17:40:39 UTC
My biggest concern is the yellow highlight over the forms to indicate they can be autofilled; how does Chrome handle that?

I'd be fine with either a popover or an infobar.
Comment 74 Abdullah Alansari 2016-02-10 04:20:07 UTC
You can find a simple demo of Chrome's Autofill here: https://youtu.be/dH_t9yAhBQs

Chrome never highlights fillable fields before they're filled. They probably came to the same conclusion that highlighting fields before they're filled is inappropriate for Autofill.

However, Chrome highlights fields after they're filled without modifying the DOM ,which doesn't seem to be supported in WebKit, and isn't really a priority in my opinion.

I feel more biased to the popover approach. As for highlighting, as you mentioned earlier, it will lead to unsatisfactory results.
Comment 75 Michael Catanzaro 2016-02-10 14:05:14 UTC
Yeah, what Chrome does looks good. I agree, changing the color of the field after it is filled is not super important (but I don't think this would be too hard). Using a popover instead of a dropdown menu would probably work fine, but please make sure the popover doesn't steal input focus from the user (so you can continue typing and just ignore the popover if you want to).
Comment 76 Abdullah Alansari 2016-02-11 04:01:41 UTC
I will start by implementing the easy case, where the user double clicks on a fillable element and the popover appears.

I will then ensure the implementation is more stable.
And finally, work on the harder case where the popover appears when the user starts typing an autofill value (e.g: his first name on a first name field).

I go to college these days, but still it shouldn't take more than 2 weeks as an initial estimate to finish this.
Comment 77 Abdullah Alansari 2016-02-18 17:37:04 UTC
Created attachment 321606 [details] [review]
Autofill GtkPopover initial implementation

You can find a demo on YouTube here: https://youtu.be/ri_OKC5M0vo

This implementation doesn't handle the harder case where the popover appears when the user starts typing an autofill value.

This is mainly because the harder case is much harder than I thought (why am I not surprised :)), so I'd like to suggest that the first Autofill release doesn't include it and focus on other aspects.
Comment 78 Michael Catanzaro 2016-02-23 20:34:00 UTC
I think we're nearing the finish line. Thanks for your persistence working on this for so long!

A few notes:

* The test page I'm using, https://tools.usps.com/go/ScheduleAPickupAction!input.action no longer crashes and almost works perfectly. The one glitch is that it fills my street address twice, once in the address entry, and once in the apartment/suite number entry. The easiest solution would be to try to avoid filling the apartment number entry; it'd probably be hard to add a way to autofill apartment number separately, as some web forms would probably not have a field for that, it's probably safest to expect users to specify apartment number in the street address field.

* Maybe makes sense to add a phone extension field?

* It was *really* hard for me to figure out how to open the popover, I managed it once, then couldn't figure out how to get it back for about 20 seconds, before I discovered that it opens on double-click. I think it should open on single-click, and have a "Do not autofill" button that can be used to prevent the popover from opening again until the next page load.

* I will try to get designer feedback regarding the UI. The autofill data preferences dialog looks pretty good. The modifications to the General tab of the dialog, not so good, but I don't think it's a blocker; we can land this early next cycle and improve that later. The popover itself could also look better, but again, I think not a blocker.

I just looked over the user interface; we still need to find time to review the code again.
Comment 79 Michael Catanzaro 2016-02-23 20:37:31 UTC
Created attachment 322179 [details]
Screenshot #1
Comment 80 Michael Catanzaro 2016-02-23 20:40:29 UTC
Created attachment 322180 [details]
screenshot #2
Comment 81 Michael Catanzaro 2016-02-23 20:41:23 UTC
Created attachment 322181 [details]
Screenshot #3
Comment 82 Abdullah Alansari 2016-02-25 03:16:01 UTC
Sorry for the inconvenience, but I have midterms next week, so I won't be able to work on this until (Sunday March 3rd).
Comment 83 Michael Catanzaro 2016-02-25 14:50:15 UTC
It's not any problem at all, Abdullah; exams are important, and we have a major release at the end of March and would not want to merge this before then, anyway. It'd be nice to get this in soon after that, though, so we can maximize the amount of time we have to test it!
Comment 84 Abdullah Alansari 2016-03-09 07:49:29 UTC
Created attachment 323476 [details] [review]
Show popup on single click and add "Do not autofill" button + don't fill apartment fields

I'm not sure if phone extension is a good idea for a first release.

One major issue that I will focus on is not filling fields not visible to the user.
Comment 85 Abdullah Alansari 2016-03-09 07:50:48 UTC
Created attachment 323477 [details]
Autofill popover with "Do not autofill" button
Comment 86 Abdullah Alansari 2016-03-09 12:02:47 UTC
Created attachment 323502 [details] [review]
Don't autofill invisible elements

It's implemented using 'offsetWidth' and 'offsetHeight'.
I'm not sure if this is enough, please do tell if you any suggestions.

BTW, why doesn't WebKitGTK+ have 'WebKitDOMElement#getBoundingClientRect()'?
Comment 87 Michael Catanzaro 2016-03-26 15:20:16 UTC
Useful test site: http://www.456bereastreet.com/lab/html5-input-types/

(In reply to Abdullah Alansari from comment #86)
> BTW, why doesn't WebKitGTK+ have 'WebKitDOMElement#getBoundingClientRect()'?

Dunno....
Comment 88 Michael Catanzaro 2016-03-31 20:56:54 UTC
Hey Abdullah, one of our designers is requesting a test page for form autofill, because "the HTML test page that you pointed me to doesn't contain a lot of the fields that are involved." I could throw something together, but perhaps you'd like to work on this, to help speed it along, since you're the expert on this now?
Comment 89 Abdullah Alansari 2016-04-02 03:59:37 UTC
Hi Michael, I wrote a test page that better illustrates Autofill's behavior.
You can find it here: https://ahimta.github.io/epiphany-form-filler-sample-page/

It's based on the model:
| Field/Type   | Personal | Card | Personal + Card | None |
| ------------ | -------- | ---- | --------------- | ---- |
| Without form | +        | +    | -               | +    |
| With form    | +        | +    | +               | +    |
Comment 90 Michael Catanzaro 2016-04-02 14:20:23 UTC
Thanks, I've passed this on to our designers :)
Comment 91 Allan Day 2016-07-29 10:51:24 UTC
I haven't tried this, so passing on some comments based on the screenshots and a bit of conversation with Adbullah. I think there are some issues around the basic behaviour that need to be worked out:

 * It needs to pick up details as they are entered into forms: if you solely rely on users finding the autofill setting and manually entering the details there, it will never get used.
 * Likewise, it needs to complete the forms as the details are entered, (probably using suggestion drop downs): you can't rely on users finding an option in a context menu in order to fill the form. Again, that would never get used.
 * It really needs to be able to handle multiple addresses and multiple credit card details. Only being able to enter one of each is a major limitation.

One other point, which is more of a detail: it's best to avoid dialogs being stacked on top of each other. One way to do this for the preferences would be something like this:

https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/web/wire-autofill-settings.png
Comment 92 Abdullah Alansari 2016-08-14 22:05:49 UTC
OK, now we got this out of the way, I'd like to have a detailed description of the required behavior and functionality, in addition to mockups of course; pseudocode is always welcome too, of course.

This so much bigger than I thought it would be initially; I would be surprised otherwise, given it's software development :).
Comment 93 Abdullah Alansari 2016-08-31 21:45:55 UTC
Hi, just wondering things are going (mockups, a rough schedule, change in plans, etc...).
Comment 94 Michael Catanzaro 2016-09-13 18:56:40 UTC
I would say:

(In reply to Allan Day from comment #91)
>  * It needs to pick up details as they are entered into forms: if you solely
> rely on users finding the autofill setting and manually entering the details
> there, it will never get used.

So, you obviously can already detect which forms can be filled in order to fill them. So what you want to do, when you detect a form submission, open an infobar to ask the user if he would like to save the form details. We already have similar code for this somewhere, to handle password saving; you would just need to adapt it a bit.

It would be good to have a mockup for this, but I think you can proceed without one; just do your best, and the rest will be details. The trick is that we only want to show one infobar, while also showing all of the details to be saved, so fitting all of that information into the infobar might not be simple.

>  * Likewise, it needs to complete the forms as the details are entered,
> (probably using suggestion drop downs): you can't rely on users finding an
> option in a context menu in order to fill the form. Again, that would never
> get used.

This seems clear enough, we have to open a dropdown when the cursor enters an entry that can be autocompleted. I understand you had some trouble figuring out how to do this, so it might be a challenge.

>  * It really needs to be able to handle multiple addresses and multiple
> credit card details. Only being able to enter one of each is a major
> limitation.

The mockup shows how this could work in the preferences dialog.

As for the schedule, that's up to you. ;)
Comment 95 Abdullah Alansari 2016-09-20 13:59:03 UTC
Hi all, sorry for the late response. Seriously, I have no excuse for this :(!

I will be taking some exams (TOEFL, GRE), in the coming 2 months and then applying to universities (statement of interest, CV, etc...). In addition to my senior project, midterms and finals. Long story short, you could say my hands are full.

Anyways, I don't know for sure if I will have time in the coming 3-4 months to finish this. I will try to make a reasonable estimate, though it may be non-trivial given the size of this feature. But still after getting almost every estimate wrong in my programming career, I think I can now make a reasonable-ish estimate :).

Looking at the bright side, I can say with confidence that the issue is tractable, and most likely pretty straight forward for the most part. However, it would still take a non-trivial of time to do get right; For me it's an issue of time not difficulty.

I will try to make an effort to see this through, regardless of time constraints. I will keep you posted.

Finally, thanks for sticking by this far :).
Comment 96 Michael Catanzaro 2016-09-20 15:18:17 UTC
(In reply to Abdullah Alansari from comment #95)
> Hi all, sorry for the late response. Seriously, I have no excuse for this :(!

There's no need to be hard on yourself for taking a week to reply, when I just took two weeks. :P

> I will be taking some exams (TOEFL, GRE), in the coming 2 months and then
> applying to universities (statement of interest, CV, etc...). In addition to
> my senior project, midterms and finals. Long story short, you could say my
> hands are full.

Good luck! The English section of the GRE is quite hard for Americans; it's ridiculous that you're expected to take it. :(
Comment 97 Michael Catanzaro 2016-10-06 19:51:17 UTC
So it turns out that autofill support was added to WebKit earlier this year... now this project gets even more complicated, because we probably want to use that instead of rolling our own. I haven't investigated it much, but I see Source/WebCore/html/Autofill.[h,cpp]; we need to see how that's used on Apple's ports.
Comment 98 Michael Catanzaro 2016-11-10 13:34:03 UTC
Test page:

https://www.roboform.com/filling-test-all-fields
Comment 99 Adam Dingle 2016-11-29 20:34:02 UTC
I appreciate all the work on this feature so far - I know that implementing this may not be so simple and has significant UI ramifications.  I just increased the posted bounty for this work (https://www.bountysource.com/issues/1325823-form-filler) to $2,000.  I hope this will inspire someone (Abdullah?) to continue this work in progress and see it through to a working implementation in master.  Thanks!
Comment 100 Abdullah Alansari 2016-11-30 03:46:06 UTC
Thanks Adam for your generosity. I will continue working on this around Feb 2017. If you find it necessary to do so before then, please do tell me; I am currently working on my senior project. Thanks!
Comment 101 Adam Dingle 2016-11-30 04:12:57 UTC
Abdullah, that sounds reasonable.  Best of luck with your project!
Comment 102 Michael Catanzaro 2017-01-07 16:39:49 UTC
Hm, one more obstacle: autofill phishing.

This example webpage that appears to have just a name/email form actually secretly autofills much more, hiding the elements off the left side of the page. That's pretty bad. The attack is designed to work on Chrome but we're going to be vulnerable unless we think carefully about how to structure our UI to avoid it.
Comment 103 Abdullah Alansari 2017-02-01 16:21:47 UTC
Hi, I've just graduated from college and have no excuse anymore to not work on this :).

To recap, the main requirements of this feature, in no particular order, are the following:
1. Show an info-bar asking the user to save submitted data to be autofilled in the future.
2. Support multiple addresses and credit cards.
3. Show autofill popover when the user clicks on a fillable field and autofill data exists for this field.
4. The content of the popover should consist mainly of saved autofill data corresponding to addresses/credit cards.
5. Clicking on an item in the popover should fill the whole form with the clicked on address/credit card.
6. Autofill is always enabled. And credit card data should be restricted to secure pages (HTTPS).
7. Autofill preferences UI should be in the same window as the main preferences window.
8. Use newly found WebKit autofill powers, if possible.
9. Show the popover to the user, when they start typing and autofill data exists.
10. Better security (e.g: autofill phishing).

A rough sketch of my plan:
1. Install the development environment; Mainly WebKit. This shouldn't take long.
2. Rebase my code against upstream master. My branch is about 1500 commits behind. This shouldn't take long too.
3. Do some simple planning and see how new changes relate to existing changes.
4. Do the rest. This shouldn't take more than a month.

Related mockups:
https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/web/wire-autofill-settings.png
https://bug696867.bugzilla-attachments.gnome.org/attachment.cgi?id=312593

Please do tell me what you think. Mockups are welcome too!

p.s: when you see shouldn't and time in the same phrase, you can safely replace shouldn't with should :).
Comment 104 Michael Catanzaro 2017-02-01 16:34:57 UTC
Looks like a good plan. Congratulations on your graduation!

(In reply to Abdullah Alansari from comment #103)
> 6. Autofill is always enabled. And credit card data should be restricted to
> secure pages (HTTPS).

I think we should restrict all the data to secure pages, or display a security warning otherwise. Maybe like how we do when you focus an insecure password form in 3.23.

> 8. Use newly found WebKit autofill powers, if possible.

This requires some investigation, because I don't know what it's all about, but we definitely need to figure out how WebKit-level autofill works and decide whether to expose it in our API. It probably does some stuff that would be hard to do otherwise from the web extension. The files to investigate are Source/WebCore/html/Autofill.[h,cpp].
Comment 105 Abdullah Alansari 2017-02-01 16:57:32 UTC
> I think we should restrict all the data to secure pages, or display a security warning otherwise. Maybe like how we do when you focus an insecure password form in 3.23.

I agree. Users should have the right to know when their data can be read by third parties. I was actually thinking of disabling autofill in insecure pages altogether. Which maybe a little drastic at the moment; Maybe in a year or so.
Comment 106 Adam Dingle 2017-02-02 07:54:45 UTC
Abdullah, glad to hear you're back in action!  I'm still very much looking forward to seeing this implemented!
Comment 107 Abdullah Alansari 2017-02-03 19:48:43 UTC
Created attachment 344881 [details] [review]
Latest patch rebased against latest upstream

Just finished rebasing autofill against latest upstream. It was mostly straightforward thanks to the great efforts of the maintainers (I see Michael's name a lot).

The patch will change a lot to accommodate the new requirements. I'm posting this only so that you can run it locally, review the code (although it will mostly change in the final patch) or do whatever else you see fit.

At the moment the style of the code is a bit different from the project style guide. I haven't fixed these styling issues, due to the temporary nature of the current patch. However, I will do so if you want to review it.

In the coming days, I will be doing some research and simple planning before starting the first sprint.

Finally, please do feel free if you have any notes, suggestions and even criticism!
Comment 108 Abdullah Alansari 2017-02-28 06:14:18 UTC
I have been going through WebKit's source code, for a while now. And I think we can make use of some of its APIs, with some changes to WebKitGTK+.

As I understand it, the main flow of autofill in WebKit is something like this:
* WebCore::AutofillButtonElement::defaultEventHandler(Event)
* WebCore::{AutofillButtonElementOwner,TextFieldInputType}::autofillButtonElementWasClicked()
* {WebCore::ChromeClient,WebKit::WebChromeClient}::handleAutofillButtonClick(HTMLInputElement)
  * WebKit::InjectedBundlePageUIClient::didClickAutofillButton(WebPage, InjectedBundleNodeHandle, Ref<APIObject>)
    * API::InjectedBundle::PageUIClient::didClickAutofillButton(WebPage, InjectedBundleNodeHandle, Ref<APIObject>)?
  * WebKit::WebPageProxy::handleAutofillButtonClick(UserData)
    * API::UIClient::didClickAutofillButton(WebKit::WebPageProxy, API::Object)

Where the parts under API:: are the interfaces we must implement in WebKitGTK+ (with some other changes). There are, of course, some other parts that support this flow and are mostly in Autofill.h and HTML elements classes.

In conclusion, I think we will be making some changes to the WebKitGTK+ that leverage WebKit's autofill capabilities because some of their implementations will be more reliable than ours. And then use the newly WebKitGTK+ autofill features and complement it with Epiphany's (mostly the UI and storage).

Sorry for the delay, and thanks for your patience continued support!
Comment 109 Michael Catanzaro 2017-02-28 23:36:28 UTC
OK, sounds good. We're the WebKitGTK+ developers too, so if you submit patches there, we'll review them speedily. This will be a lot better than hacking up our own autofill in the Epiphany web extension. :)

Be sure to prefix the bug title with [GTK] select the WebKitGTK+ component when you file the WebKit bug, to ensure that we notice it.
Comment 110 Michael Catanzaro 2017-03-11 16:31:29 UTC
Comment on attachment 344881 [details] [review]
Latest patch rebased against latest upstream

Just removing this from the unreviewed patch tracker, as per the comments above.
Comment 111 Abdullah Alansari 2017-03-17 17:39:35 UTC
I've been experimenting with WebKit (using the mini-browser) for a while now and to my disappointment, I couldn't get it to do anything remotely close to the autofill flow.

I am either missing a key piece of the autofill process/flow/architecture or WebKit just doesn't have that much of a one (that's useful to us) in the first place.

What I need right now is some guidance in the form of a big-picture explanation of the whole WebKit autofill capabilities (including limitations) or whether it's more suitable to just do the whole thing in Epiphany.
Comment 112 Michael Catanzaro 2017-03-18 15:44:16 UTC
I think it's fine to implement it at the Epiphany level.
Comment 113 Abdullah Alansari 2017-03-20 05:12:36 UTC
Created attachment 348294 [details]
Current Preferences Mockup

I plan to start by implementing the whole UI (almost), including preferences and maybe info-bar too. Because I think this will guide the rest of the technical decisions (e.g: storage).

And for this, I need some seriously detailed mockups that not only show the outline of the UI, but also the individual data items (e.g: first name, card holder). In addition, of course, to the whole autofill flow too :).
Comment 114 Adam Dingle 2017-05-13 14:03:05 UTC
(In reply to Michael Catanzaro from comment #102)
> Hm, one more obstacle: autofill phishing.
> 
> This example webpage that appears to have just a name/email form actually
> secretly autofills much more, hiding the elements off the left side of the
> page. That's pretty bad. The attack is designed to work on Chrome but we're
> going to be vulnerable unless we think carefully about how to structure our
> UI to avoid it.

I think you forgot to include a link to the example webpage.  Here it is:

https://github.com/anttiviljami/browser-autofill-phishing
Comment 115 Adam Dingle 2017-05-13 14:29:52 UTC
Abdullah has requested detailed mockups to be able to implement this.  Allan (or anyone else from the design team): is this something you can provide?  Or do you think Abdullah should proceed with an improvised user interface loosely based on the existing mockups, and then we can polish it later?

As mentioned above, the existing mockups seem to be at

https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/web/wire-autofill-settings.png
https://bug696867.bugzilla-attachments.gnome.org/attachment.cgi?id=312593
Comment 116 GNOME Infrastructure Team 2018-08-03 19:56:09 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/epiphany/issues/193.