Address autofill API [chromium/src : main]
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
This is identical to https://chromium-review.googlesource.com/c/chromium/src/+/6200613, with the following differences:
- The AutofillEvent now has a browser test
- Removed the extra `Lift` from ContentAutofillDriver
- Brought back `WebString key = control_element.NameForAutofill();` in form_autofill_util and removed the TODO (which was incorrect)
- Bring back the direct call to `RetriggerAutofillImpl` from BrowserAutofillManager (instead of `ScheduleRefill`), as we don't need to schedule a refill in the future, but to trigger it immediately (as a response to the promise resolving).
- Add a null check to web_form_element.
I'll add comments in the code itself to make the above clearer and easier to review.
Related details
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) voted and added 3 comments![]( "Open in Gerrit")
Votes added by Yoav Weiss (@Shopify)
| Commit-Queue | +1 |
3 comments
WebString key = control_element.NameForAutofill();@schw...@google.com - I brought back the name here, as this is the fields that are web-exposed to developers, and they should have meaningful names.
form_filler_->RetriggerAutofillImpl(@schw...@google.com - This calls RetriggerAutofillImpl rather than ScheduleRefill as I believe we need to trigger it immediately, not in some point in the future.
if (!element) {@schw...@google.com - Added this null check, as some tests arrive here without an HTMLFormElement.
Related details
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering added 10 comments![]( "Open in Gerrit")
// Retrigger an autofill after the onautofill event fired and the Promise it
// passed to `waitForIt` was resolved.I suspect most readers won't know what this is referring to, especially what `waitForIt` is. (At least I don't 😊.)
bool waiting_for_autofill_retrigger_ = false;How many times may JS delay the autofill? Once, right?
bool DispatchAutofillEvent(const blink::WebDocument& document,Please document what this does.
WebString key = control_element.NameForAutofill();@schw...@google.com - I brought back the name here, as this is the fields that are web-exposed to developers, and they should have meaningful names.
I see! There will a specification for what the keys should be, right? I think we don't guarantee stability of `NameForAutofill()` so perhaps we should implement our own fallback from `name` to `id` or vice versa, if we want such a fallback at all.
.TriggerAutofillEvent(is_undo, field_values);std::move() or make the parameter a const ref?
void RetriggerAutofillImpl(const FormData& form,Keep `TriggerRefill()`?
if (!element) {@schw...@google.com - Added this null check, as some tests arrive here without an HTMLFormElement.
It seems odd to me to have a meaningful operation on null `WebFormElement`s. Analogous with [this](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/content/renderer/form_autofill_util.cc;l=1882-1891;drc=48d6f7175422b2c969c14258f9f8d5b196c28d18), I think the null case should be handled by `WebDocument`.
void waitForIt(ScriptState*, ScriptPromise<IDLUndefined>);What is "it"?
class AutofillEvent : public Event {Should this be documented?
std::unordered_map<String, String> converted_form_values;Do we really want `std::unordered_map` here? base/containers/README.md suggests it for no scenario. And we already take the `std::map` by value here; couldn't we just pass it on?
Related details
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 4 comments![]( "Open in Gerrit")
// Retrigger an autofill after the onautofill event fired and the Promise it
// passed to `waitForIt` was resolved.I suspect most readers won't know what this is referring to, especially what `waitForIt` is. (At least I don't 😊.)
It's the function you can call inside the event, to pass a promise that tells the browser autofill can refill. I need to rename it (both here and in the proposal). This was a placeholder..
bool waiting_for_autofill_retrigger_ = false;How many times may JS delay the autofill? Once, right?
Indeed
WebString key = control_element.NameForAutofill();Christoph Schwering@schw...@google.com - I brought back the name here, as this is the fields that are web-exposed to developers, and they should have meaningful names.
I see! There will a specification for what the keys should be, right? I think we don't guarantee stability of `NameForAutofill()` so perhaps we should implement our own fallback from `name` to `id` or vice versa, if we want such a fallback at all.
Yeah, we probably want to create a more thought-through mapping..
void waitForIt(ScriptState*, ScriptPromise<IDLUndefined>);What is "it"?
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering added 2 comments![]( "Open in Gerrit")
const FormFieldData& trigger_field = form.fields()[0];That's a security issue because the triggering field's origin matters when there are multiple frames involved.
In `BrowserAutofillManager`, that issue is handled by the refill logic, which remembers the origin in the `RefillContex`.
In `AndroidAutofillManager` the closest to that I know is [`current_field_`](https://source.chromium.org/chromium/chromium/src/+/main:components/android_autofill/browser/android_autofill_provider.h;l=327-333;drc=524850a07d7849a97763c3b848063d3dfdbd653d), but I guess just using `current_field_` isn't sufficent.
// Retrigger an autofill after the onautofill event fired and the Promise it
// passed to `waitForIt` was resolved.Yoav Weiss (@Shopify)I suspect most readers won't know what this is referring to, especially what `waitForIt` is. (At least I don't 😊.)
It's the function you can call inside the event, to pass a promise that tells the browser autofill can refill. I need to rename it (both here and in the proposal). This was a placeholder..
I see. I guess we should abstract from this here in the mojo documentation.
Related details
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 3 comments![]( "Open in Gerrit")
// Retrigger an autofill after the onautofill event fired and the Promise it
// passed to `waitForIt` was resolved.Yoav Weiss (@Shopify)I suspect most readers won't know what this is referring to, especially what `waitForIt` is. (At least I don't 😊.)
Christoph SchweringIt's the function you can call inside the event, to pass a promise that tells the browser autofill can refill. I need to rename it (both here and in the proposal). This was a placeholder..
I see. I guess we should abstract from this here in the mojo documentation.
Renamed the function to make things clearer. Let me know if more changes are needed
void waitForIt(ScriptState*, ScriptPromise<IDLUndefined>);Yoav Weiss (@Shopify)What is "it"?
I'll rename this (in the proposal and code..)
Renamed
std::unordered_map<String, String> converted_form_values;Do we really want `std::unordered_map` here? base/containers/README.md suggests it for no scenario. And we already take the `std::map` by value here; couldn't we just pass it on?
Nuked this conversion in favor of conversion in web_form_element (where it belongs)..
Related details
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
WebString key = control_element.NameForAutofill();Christoph Schwering@schw...@google.com - I brought back the name here, as this is the fields that are web-exposed to developers, and they should have meaningful names.
Yoav Weiss (@Shopify)I see! There will a specification for what the keys should be, right? I think we don't guarantee stability of `NameForAutofill()` so perhaps we should implement our own fallback from `name` to `id` or vice versa, if we want such a fallback at all.
Yeah, we probably want to create a more thought-through mapping..
I'm hesitating on the best approach here.. IIUC, we have the "autocomplete" attribute (translated to HTMLFieldType), as well as `NameForAutofill` which is essentially the "name" and "id".
Should we try to map all three to the HTML autocomplete tokens? Something else?
Related details
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) voted and added 4 comments![]( "Open in Gerrit")
Votes added by Yoav Weiss (@Shopify)
| Commit-Queue | +1 |
4 comments
std::move() or make the parameter a const ref?
moved
void RetriggerAutofillImpl(const FormData& form,Yoav Weiss (@Shopify)Keep `TriggerRefill()`?
Done
Christoph Schwering@schw...@google.com - Added this null check, as some tests arrive here without an HTMLFormElement.
It seems odd to me to have a meaningful operation on null `WebFormElement`s. Analogous with [this](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/content/renderer/form_autofill_util.cc;l=1882-1891;drc=48d6f7175422b2c969c14258f9f8d5b196c28d18), I think the null case should be handled by `WebDocument`.
Handled this at the call site!
class AutofillEvent : public Event {Yoav Weiss (@Shopify)Should this be documented?
Done
Related details
- Christoph Schwering
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
bool DispatchAutofillEvent(const blink::WebDocument& document,Please document what this does.
Done
Related details
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
// Retrigger an autofill after the onautofill event fired and the Promise it
// passed to `waitForIt` was resolved.Yoav Weiss (@Shopify)I suspect most readers won't know what this is referring to, especially what `waitForIt` is. (At least I don't 😊.)
Christoph SchweringIt's the function you can call inside the event, to pass a promise that tells the browser autofill can refill. I need to rename it (both here and in the proposal). This was a placeholder..
Yoav Weiss (@Shopify)I see. I guess we should abstract from this here in the mojo documentation.
Renamed the function to make things clearer. Let me know if more changes are needed
Resolving
Related details
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yoav Weiss (@Shopify)
| Commit-Queue | +1 |
1 comment
const FormFieldData& trigger_field = form.fields()[0];That's a security issue because the triggering field's origin matters when there are multiple frames involved.
In `BrowserAutofillManager`, that issue is handled by the refill logic, which remembers the origin in the `RefillContex`.
In `AndroidAutofillManager` the closest to that I know is [`current_field_`](https://source.chromium.org/chromium/chromium/src/+/main:components/android_autofill/browser/android_autofill_provider.h;l=327-333;drc=524850a07d7849a97763c3b848063d3dfdbd653d), but I guess just using `current_field_` isn't sufficent.
Revamped to use current_field_'s ID and origin. Let me know if this is not sufficient..
Related details
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
WebString key = control_element.NameForAutofill();@schw...@google.com - I brought back the name here, as this is the fields that are web-exposed to developers, and they should have meaningful names.
Yoav Weiss (@Shopify)I see! There will a specification for what the keys should be, right? I think we don't guarantee stability of `NameForAutofill()` so perhaps we should implement our own fallback from `name` to `id` or vice versa, if we want such a fallback at all.
Yoav Weiss (@Shopify)Yeah, we probably want to create a more thought-through mapping..
I'm hesitating on the best approach here.. IIUC, we have the "autocomplete" attribute (translated to HTMLFieldType), as well as `NameForAutofill` which is essentially the "name" and "id".
Should we try to map all three to the HTML autocomplete tokens? Something else?
Added a TODO to resolve this in an issue on the spec repo
Related details
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 3 comments![]( "Open in Gerrit")
auto* provider = GetAutofillProvider();It's unclear what this would do in terms of UI when retriggering autofill in WebViews.
bool waiting_for_autofill_retrigger_ = false;Yoav Weiss (@Shopify)How many times may JS delay the autofill? Once, right?
Indeed
Resolving
form_filler_->RetriggerAutofillImpl(Yoav Weiss (@Shopify)@schw...@google.com - This calls RetriggerAutofillImpl rather than ScheduleRefill as I believe we need to trigger it immediately, not in some point in the future.
Resolving
Cathy Li (Gerrit)
Cathy Li added 2 comments![]( "Open in Gerrit")
auto current_field_info = provider->GetCurrentFieldIdAndOrigin();In an ideal world, we can also cache the last-triggered field from the last OnAutofillAvailable call in the provider, and use that. We'd need to cache that information when we cache the form info (see other comment)
My personal playing around with shopify's sites in this space previously showed that the form manipulation usually changes the autofill session (which doesn't matter in C++, but *does matter a lot* if we notify the Java side - aka option A in the other comment), and the last-modified form field ends up becoming the "current_field". For shopify sites I've played around with, this always ends up being the telephone number country code field for me.
That means by the time you are trying to retrigger the autofill, the current_field is probably already updated to telephone number - hence, you want to cache the original trigger field inside autofillProvider when OnAutofillAvailable happens, and use that trigger field, if it exists, as the re-fill trigger
provider->OnAskForValuesToFill(What kind of flow do you want to trigger with the retrigger autofill?
By this time, presumably the autofill service (e.g. Meta's custom autofill or OnePass) would have:
1. already processed the form
2. prompted the user for whether they consent to autofill
3. the user has already consented
4. sent back a list of IDs with values for setting values.
Your choices at this point are:
A. potentially notify the autofill service that the form is ready for re-fill (meaning an Android change to update the Android AutofillManager API to notify AutofillService/client implementations)
B. silently trigger a refill based on the information (id, value pairs) that the service provided with #4 above.
If A, this is a much bigger change.
If B, you'll need to make some changes inside AndroidAutofillProvider to cache the last-filled form information from OnAutofillAvailable method (https://source.chromium.org/chromium/chromium/src/+/main:components/android_autofill/browser/android_autofill_provider.cc;l=337), create a new method (maybe called "retriggerAutofill" inside AndroidAutofillProvider which takes the cached last-filled form information, map it into the new form, and call FillOrPreviewForm with that newly mapped information.
From API standpoint, A is going to be much better, where autofill services (like onepass) can then retrigger fills with updated information.
From fast experiment standpoint, B is going to be better. I can custom-hookup this into meta's custom autofill APIs when I land this change into our custom webview
Related details
- Christoph Schwering
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 2 comments![]( "Open in Gerrit")
auto current_field_info = provider->GetCurrentFieldIdAndOrigin();In an ideal world, we can also cache the last-triggered field from the last OnAutofillAvailable call in the provider, and use that. We'd need to cache that information when we cache the form info (see other comment)
My personal playing around with shopify's sites in this space previously showed that the form manipulation usually changes the autofill session (which doesn't matter in C++, but *does matter a lot* if we notify the Java side - aka option A in the other comment), and the last-modified form field ends up becoming the "current_field". For shopify sites I've played around with, this always ends up being the telephone number country code field for me.
That means by the time you are trying to retrigger the autofill, the current_field is probably already updated to telephone number - hence, you want to cache the original trigger field inside autofillProvider when OnAutofillAvailable happens, and use that trigger field, if it exists, as the re-fill trigger
Revamped to cache the last-triggered field. PTAL?
provider->OnAskForValuesToFill(What kind of flow do you want to trigger with the retrigger autofill?
By this time, presumably the autofill service (e.g. Meta's custom autofill or OnePass) would have:
1. already processed the form
2. prompted the user for whether they consent to autofill
3. the user has already consented
4. sent back a list of IDs with values for setting values.Your choices at this point are:
A. potentially notify the autofill service that the form is ready for re-fill (meaning an Android change to update the Android AutofillManager API to notify AutofillService/client implementations)
B. silently trigger a refill based on the information (id, value pairs) that the service provided with #4 above.If A, this is a much bigger change.
If B, you'll need to make some changes inside AndroidAutofillProvider to cache the last-filled form information from OnAutofillAvailable method (https://source.chromium.org/chromium/chromium/src/+/main:components/android_autofill/browser/android_autofill_provider.cc;l=337), create a new method (maybe called "retriggerAutofill" inside AndroidAutofillProvider which takes the cached last-filled form information, map it into the new form, and call FillOrPreviewForm with that newly mapped information.From API standpoint, A is going to be much better, where autofill services (like onepass) can then retrigger fills with updated information.
From fast experiment standpoint, B is going to be better. I can custom-hookup this into meta's custom autofill APIs when I land this change into our custom webview
Do I understand correctly and A would require a change on the Android OS side, as well as an eco-system change with various autofill providers?
If so, I think we should probably start with B.
Related details
- Cathy Li
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering added 14 comments![]( "Open in Gerrit")
"//net:test_support",Move this upwards?
#if BUILDFLAG(IS_ANDROID)
return chrome_test_utils::GetActiveWebContents(this);
#else
return browser()->tab_strip_model()->GetActiveWebContents();
#endif
}Dosen't `chrome_test_utils::GetActiveWebContents(this);` work on Desktop?
// Enable the AddressAutofillAPI feature for this testnitty nit: `.` (also below)
scoped_feature_list_.InitAndEnableFeature(
blink::features::kAddressAutofillAPI);nit: Do this at the test fixture's construction time?
```
base::test::ScopedFeatureList scoped_feature_list_{
blink::features::kAddressAutofillAPI};
```
should suffice
// === Verify autofill event was fired ===nit: remove (and below)
const FormFieldData& trigger_field = form.fields()[0];That's a security issue because the triggering field's origin matters when there are multiple frames involved.
In `BrowserAutofillManager`, that issue is handled by the refill logic, which remembers the origin in the `RefillContex`.
In `AndroidAutofillManager` the closest to that I know is [`current_field_`](https://source.chromium.org/chromium/chromium/src/+/main:components/android_autofill/browser/android_autofill_provider.h;l=327-333;drc=524850a07d7849a97763c3b848063d3dfdbd653d), but I guess just using `current_field_` isn't sufficent.
Christoph SchweringRevamped to use current_field_'s ID and origin. Let me know if this is not sufficient..
Thanks
provider->OnAskForValuesToFill(Yoav Weiss (@Shopify)What kind of flow do you want to trigger with the retrigger autofill?
By this time, presumably the autofill service (e.g. Meta's custom autofill or OnePass) would have:
1. already processed the form
2. prompted the user for whether they consent to autofill
3. the user has already consented
4. sent back a list of IDs with values for setting values.Your choices at this point are:
A. potentially notify the autofill service that the form is ready for re-fill (meaning an Android change to update the Android AutofillManager API to notify AutofillService/client implementations)
B. silently trigger a refill based on the information (id, value pairs) that the service provided with #4 above.If A, this is a much bigger change.
If B, you'll need to make some changes inside AndroidAutofillProvider to cache the last-filled form information from OnAutofillAvailable method (https://source.chromium.org/chromium/chromium/src/+/main:components/android_autofill/browser/android_autofill_provider.cc;l=337), create a new method (maybe called "retriggerAutofill" inside AndroidAutofillProvider which takes the cached last-filled form information, map it into the new form, and call FillOrPreviewForm with that newly mapped information.From API standpoint, A is going to be much better, where autofill services (like onepass) can then retrigger fills with updated information.
From fast experiment standpoint, B is going to be better. I can custom-hookup this into meta's custom autofill APIs when I land this change into our custom webview
Do I understand correctly and A would require a change on the Android OS side, as well as an eco-system change with various autofill providers?
If so, I think we should probably start with B.
For my understanding: B does not support added fields, does it? If so, isn't A necessary?
// Properties of the last-focused field of the current session for `form_`
// (queried input or changed select box).
CurrentFieldInfo current_field_;
// Properties of the original trigger field that initiated the autofill
// session. This is cached in OnAutofillAvailable to ensure retrigger
// operations use the original field even if form manipulation changes the
// current field.
CurrentFieldInfo original_trigger_field_;I guess `Reset()` should reset this, too.
Can we collect the session-specific state in one `struct`? Maybe there should be a member `std::optional<SessionState>`.
I find it rather difficult to understand what a session is in AndroidAutofillProvider (when does it start, when does it end, and what's its state).
// session. This is cached in OnAutofillAvailable to ensure retriggernit: `()`
// AutofillProvider:
std::optional<std::pair<FieldGlobalId, url::Origin>>
GetOriginalTriggerFieldIdAndOrigin() const override;nit: Move up to the other overrides?
private:Should this be public or should `GetCurrentFieldInfo()` be private??
// Cache the original trigger field that initiated this autofill session.I find no information what the event means and when it's fired (I didn't look beyond the C++ code). There's a `StartNewSession()` function. The comment sounds to me like `original_trigger_field_` should be set there. WDYT?
bool waiting_for_autofill_retrigger_ = false;Like you mention below, we need a timer that fires `RetriggerAutofill()` if JS hasn't responded fast enough. I guess we can just replace the `bool` with a `base::OneShotTimer`?
bool waiting_for_autofill_retrigger_ = false;Suppose `waiting_for_autofill_retrigger` is true. How will we deal with `ApplyFieldsAction()` messages coming in?
One option could be to ignore them. Then we should also not send any `AskForValuesToFill()` while `waiting_for_autufill_retrigger`. Otherwise, the user could see Autofill suggestions and maybe even the preview, but filling would do nothing.
We could try to refine the skipping and only block `AskForValuesToFill()` and `ApplyFieldsAction()` on fields that are touched by ongoing fill operations.
I'm also wondering how things behave when we have a frame-transcending form. Suppose we have a frame-transcending form in two documents A and B and
- A and B both observe `onautofill` and
- Autofill fills the fields in A and B
- A adds some field X to its form (--> the browser process is informed about the change)
- B adds some field Y to its form (--> the browser process is informed about the change)
- A resolves the promise
- the browser process refills the frame-transcending form, including the new fields X and Y.
What should B do? Should it fill the new data into Y or ignore it?
I guess it's OK for B to ignore it. Because B can and will still resolve the promise, which would trigger another refill that again includes X and Y, and B would not ignore that second refill.
Related details
- Cathy Li
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 6 comments![]( "Open in Gerrit")
"//net:test_support",Yoav Weiss (@Shopify)Move this upwards?
Done
#if BUILDFLAG(IS_ANDROID)
return chrome_test_utils::GetActiveWebContents(this);
#else
return browser()->tab_strip_model()->GetActiveWebContents();
#endif
}Dosen't `chrome_test_utils::GetActiveWebContents(this);` work on Desktop?
It does!!
// Enable the AddressAutofillAPI feature for this testnitty nit: `.` (also below)
Removed
scoped_feature_list_.InitAndEnableFeature(
blink::features::kAddressAutofillAPI);nit: Do this at the test fixture's construction time?
```
base::test::ScopedFeatureList scoped_feature_list_{
blink::features::kAddressAutofillAPI};
```
should suffice
Done
// === Verify autofill event was fired ===Yoav Weiss (@Shopify)nit: remove (and below)
Done
// session. This is cached in OnAutofillAvailable to ensure retriggerYoav Weiss (@Shopify)nit: `()`
Done
Related details
- Cathy Li
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) voted and added 3 comments![]( "Open in Gerrit")
Votes added by Yoav Weiss (@Shopify)
| Commit-Queue | +1 |
3 comments
provider->OnAskForValuesToFill(Yoav Weiss (@Shopify)What kind of flow do you want to trigger with the retrigger autofill?
By this time, presumably the autofill service (e.g. Meta's custom autofill or OnePass) would have:
1. already processed the form
2. prompted the user for whether they consent to autofill
3. the user has already consented
4. sent back a list of IDs with values for setting values.Your choices at this point are:
A. potentially notify the autofill service that the form is ready for re-fill (meaning an Android change to update the Android AutofillManager API to notify AutofillService/client implementations)
B. silently trigger a refill based on the information (id, value pairs) that the service provided with #4 above.If A, this is a much bigger change.
If B, you'll need to make some changes inside AndroidAutofillProvider to cache the last-filled form information from OnAutofillAvailable method (https://source.chromium.org/chromium/chromium/src/+/main:components/android_autofill/browser/android_autofill_provider.cc;l=337), create a new method (maybe called "retriggerAutofill" inside AndroidAutofillProvider which takes the cached last-filled form information, map it into the new form, and call FillOrPreviewForm with that newly mapped information.From API standpoint, A is going to be much better, where autofill services (like onepass) can then retrigger fills with updated information.
From fast experiment standpoint, B is going to be better. I can custom-hookup this into meta's custom autofill APIs when I land this change into our custom webview
Christoph SchweringDo I understand correctly and A would require a change on the Android OS side, as well as an eco-system change with various autofill providers?
If so, I think we should probably start with B.
For my understanding: B does not support added fields, does it? If so, isn't A necessary?
That's a great point. Cathy - thoughts?
// AutofillProvider:
std::optional<std::pair<FieldGlobalId, url::Origin>>
GetOriginalTriggerFieldIdAndOrigin() const override;Yoav Weiss (@Shopify)nit: Move up to the other overrides?
Done
Should this be public or should `GetCurrentFieldInfo()` be private??
It's actually dead code. Removed!
Related details
- Cathy Li
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering added 1 comment![]( "Open in Gerrit")
Nothing needed from my side at the moment, right?
Related details
- Cathy Li
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
Nothing needed from my side at the moment, right?
- Cathy Li
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) voted and added 3 comments![]( "Open in Gerrit")
Votes added by Yoav Weiss (@Shopify)
| Commit-Queue | +1 |
3 comments
// Properties of the last-focused field of the current session for `form_`
// (queried input or changed select box).
CurrentFieldInfo current_field_;
// Properties of the original trigger field that initiated the autofill
// session. This is cached in OnAutofillAvailable to ensure retrigger
// operations use the original field even if form manipulation changes the
// current field.
CurrentFieldInfo original_trigger_field_;I guess `Reset()` should reset this, too.
Can we collect the session-specific state in one `struct`? Maybe there should be a member `std::optional<SessionState>`.
I find it rather difficult to understand what a session is in AndroidAutofillProvider (when does it start, when does it end, and what's its state).
Did that as part of https://chromium-review.googlesource.com/c/chromium/src/+/6998451 as it seemed like a large non-functional change. Once that lands, I can fold original_trigger_field_ into SessionState.
// Cache the original trigger field that initiated this autofill session.I find no information what the event means and when it's fired (I didn't look beyond the C++ code). There's a `StartNewSession()` function. The comment sounds to me like `original_trigger_field_` should be set there. WDYT?
Done
bool waiting_for_autofill_retrigger_ = false;Like you mention below, we need a timer that fires `RetriggerAutofill()` if JS hasn't responded fast enough. I guess we can just replace the `bool` with a `base::OneShotTimer`?
This adds a bit of complexity so I thought it's best to do this in a followup. But I can integrate it to this CL if this is better from your perspective
Related details
- Cathy Li
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yoav Weiss (@Shopify)
| Commit-Queue | +1 |
1 comment
bool waiting_for_autofill_retrigger_ = false;Suppose `waiting_for_autofill_retrigger` is true. How will we deal with `ApplyFieldsAction()` messages coming in?
One option could be to ignore them. Then we should also not send any `AskForValuesToFill()` while `waiting_for_autufill_retrigger`. Otherwise, the user could see Autofill suggestions and maybe even the preview, but filling would do nothing.
We could try to refine the skipping and only block `AskForValuesToFill()` and `ApplyFieldsAction()` on fields that are touched by ongoing fill operations.
I'm also wondering how things behave when we have a frame-transcending form. Suppose we have a frame-transcending form in two documents A and B and
- A and B both observe `onautofill` and
- Autofill fills the fields in A and B
- A adds some field X to its form (--> the browser process is informed about the change)
- B adds some field Y to its form (--> the browser process is informed about the change)
- A resolves the promise
- the browser process refills the frame-transcending form, including the new fields X and Y.
What should B do? Should it fill the new data into Y or ignore it?
I guess it's OK for B to ignore it. Because B can and will still resolve the promise, which would trigger another refill that again includes X and Y, and B would not ignore that second refill.
Suppose waiting_for_autofill_retrigger is true. How will we deal with ApplyFieldsAction() messages coming in?
One option could be to ignore them. Then we should also not send any AskForValuesToFill() while waiting_for_autufill_retrigger. Otherwise, the user could see Autofill suggestions and maybe even the preview, but filling would do nothing.
We could try to refine the skipping and only block AskForValuesToFill() and ApplyFieldsAction() on fields that are touched by ongoing fill operations.
Implemented the blocking behavior. Currently it can block autofill forever, but once we add a timeout blocking makes more sense.
> I'm also wondering how things behave when we have a frame-transcending form. Suppose we have a frame-transcending form in two documents A and B and
A and B both observe onautofill and
Autofill fills the fields in A and B
A adds some field X to its form (--> the browser process is informed about the change)
B adds some field Y to its form (--> the browser process is informed about the change)
A resolves the promise
the browser process refills the frame-transcending form, including the new fields X and Y.
What should B do? Should it fill the new data into Y or ignore it?
I guess it's OK for B to ignore it. Because B can and will still resolve the promise, which would trigger another refill that again includes X and Y, and B would not ignore that second refill.
Yeah, I think it's fine for each form to fill once it resolves the promise. That way if it moves fields around we won't get an inconsistent outcome.
Related details
- Cathy Li
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
// Properties of the last-focused field of the current session for `form_`
// (queried input or changed select box).
CurrentFieldInfo current_field_;
// Properties of the original trigger field that initiated the autofill
// session. This is cached in OnAutofillAvailable to ensure retrigger
// operations use the original field even if form manipulation changes the
// current field.
CurrentFieldInfo original_trigger_field_;Yoav Weiss (@Shopify)I guess `Reset()` should reset this, too.
Can we collect the session-specific state in one `struct`? Maybe there should be a member `std::optional<SessionState>`.
I find it rather difficult to understand what a session is in AndroidAutofillProvider (when does it start, when does it end, and what's its state).
Did that as part of https://chromium-review.googlesource.com/c/chromium/src/+/6998451 as it seemed like a large non-functional change. Once that lands, I can fold original_trigger_field_ into SessionState.
Done
Related details
- Cathy Li
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cathy Li (Gerrit)
Cathy Li added 1 comment![]( "Open in Gerrit")
provider->OnAskForValuesToFill(Yoav Weiss (@Shopify)What kind of flow do you want to trigger with the retrigger autofill?
By this time, presumably the autofill service (e.g. Meta's custom autofill or OnePass) would have:
1. already processed the form
2. prompted the user for whether they consent to autofill
3. the user has already consented
4. sent back a list of IDs with values for setting values.Your choices at this point are:
A. potentially notify the autofill service that the form is ready for re-fill (meaning an Android change to update the Android AutofillManager API to notify AutofillService/client implementations)
B. silently trigger a refill based on the information (id, value pairs) that the service provided with #4 above.If A, this is a much bigger change.
If B, you'll need to make some changes inside AndroidAutofillProvider to cache the last-filled form information from OnAutofillAvailable method (https://source.chromium.org/chromium/chromium/src/+/main:components/android_autofill/browser/android_autofill_provider.cc;l=337), create a new method (maybe called "retriggerAutofill" inside AndroidAutofillProvider which takes the cached last-filled form information, map it into the new form, and call FillOrPreviewForm with that newly mapped information.From API standpoint, A is going to be much better, where autofill services (like onepass) can then retrigger fills with updated information.
From fast experiment standpoint, B is going to be better. I can custom-hookup this into meta's custom autofill APIs when I land this change into our custom webview
Christoph SchweringDo I understand correctly and A would require a change on the Android OS side, as well as an eco-system change with various autofill providers?
If so, I think we should probably start with B.
For my understanding: B does not support added fields, does it? If so, isn't A necessary?
Yoav: yes, A would probably require eco-system change, which you may be pushing for anyway. It's the only way webviews can notify system autofill services that form has changed so they can re-examine all of the fields and issue a re-fill request without potentially triggering yet another UI prompt.
Yoav/Christoph: I think for long-term we probably want A. For an initial implementation where we just want to see if this idea actually works, we can maybe start with B?
Additional context: we fork the chromium webview code and package it into Meta's in-app-browser x Meta's autofill. We were intending to run a trial test between Meta and Shopify for this API to see how it affects autofill "in the wild". I can make custom changes in our forked webview to make the connections necessary for this initial trial test, while we work on what A would look like
Related details
- Christoph Schwering
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: kin...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): kin...@chromium.org
Reviewer source(s):
kin...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Christoph Schwering
- Jan Keitel
- Kinuko Yasuda
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cathy Li (Gerrit)
Cathy Li added 1 comment![]( "Open in Gerrit")
void RetriggerAutofill(const FormData&) override;note - this doesn't compile. you're missing the name for this parameter
e.g. const FormData& form
Christoph Schwering (Gerrit)
Christoph Schwering added 11 comments![]( "Open in Gerrit")
base::WeakPtr<AndroidAutofillManager> manager_;Seems to be unused -- remove?
content::GlobalRenderFrameHostId last_queried_field_rfh_id_;Seems to be unused -- remove?
CurrentFieldInfo original_trigger_field;Can it be `const`?
// session. This is cached in OnAutofillAvailable() to ensure retrigger
// operations use the original field even if form manipulation changes the
// current field.I don't understand this, perhaps because I don't know `OnAutofillAvailable()` means.
I'd assume that the "original" trigger field is set when a `SessionState` is created and remains unchanged. Is that not the case?
void RetriggerAutofill(const FormData&) override;note - this doesn't compile. you're missing the name for this parameter
e.g. const FormData& form
Parameters don't need names (https://en.cppreference.com/w/cpp/language/function.html#Parameter_list), but I also think we should have one for consistency.
RetriggerAutofill(FormData form);How about naming this `RequestRefill()`?
"Refill" sounds more familiar to those who who are familiar with refills, and IIUC for `BrowserAutofillManager` it's the the behaviour that we trigger.
And "request" makes it sound less binding. For example, if the originally filled field is gone, Chrome won't refill the form (at least now).
bool waiting_for_autofill_retrigger_ = false;Yoav Weiss (@Shopify)Like you mention below, we need a timer that fires `RetriggerAutofill()` if JS hasn't responded fast enough. I guess we can just replace the `bool` with a `base::OneShotTimer`?
This adds a bit of complexity so I thought it's best to do this in a followup. But I can integrate it to this CL if this is better from your perspective
Hmm I think we need to move this to the browser process. Otherwise, it wouldn't work properly for frame-transcending forms.
Otherwise, if frame A observes `autofill` and frame B does not, the user could still trigger an autofill before frame A has fulfilled the promise.
I imagine something like this: every frame reports when a promise is pending and when a promise is fulfilled. The browser process (probably `AutofillClient` or `AutofillDriverFactory`) have a counter. The browser process only shows suggestions -- and maybe also only fills forms -- if this counter is zero.
Moving this to a separate CL sgtm, but I think we should have a concrete idea how it'll work already. WDYT?
if (autofill_retrigger_state_ == AutofillRetriggerState::kNotWaiting &&
base::FeatureList::IsEnabled(blink::features::kAddressAutofillAPI) &&
form_util::DispatchAutofillEvent(document, is_undo, fields) ==
AutofillEventResult::DELAY) {
autofill_retrigger_state_ = AutofillRetriggerState::kWaiting;
// We need to delay autofill triggering until the AutofillEvent
// prepareForm promise is resolved.
// TODO: Kick off a timer here that will retrigger the autofill on the
// same form if the promise is not resolved in that time.
return;
}
autofill_retrigger_state_ = AutofillRetriggerState::kNotWaiting;A suggestion to reduce overall complexity:
- Chrome supports only address and credit card refills.
- WebView supports no refills at all.
How about we add a parameter to `ApplyFieldsAction()` (say, `bool fire_js_event`), which we always set to `false` except for non-refill address or credit card fils ([here](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/filling/form_filler.cc;l=920-926;drc=30914745e420f4e9b1e95dd8dffe37441f7c779e)).
virtual void RetriggerAutofill(const FormData& form);nit: `On...`
void NoOp(AutofillManager&) {}What does this mean?
.Then(NotifyObserversCallback(&Observer::NoOp)));If we don't want an `Observer` event, just remove this?
Otherwise, there should be an `OnBeforeFoo()` and an `OnAfterFoo()` event.
Related details
- Jan Keitel
- Kinuko Yasuda
- Yoav Weiss (@Shopify)
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 9 comments![]( "Open in Gerrit")
base::WeakPtr<AndroidAutofillManager> manager_;Seems to be unused -- remove?
Done
content::GlobalRenderFrameHostId last_queried_field_rfh_id_;Seems to be unused -- remove?
Done
CurrentFieldInfo original_trigger_field;Can it be `const`?
I don't think it can without further changes to how SessionState is constructed
// session. This is cached in OnAutofillAvailable() to ensure retrigger
// operations use the original field even if form manipulation changes the
// current field.I don't understand this, perhaps because I don't know `OnAutofillAvailable()` means.
I'd assume that the "original" trigger field is set when a `SessionState` is created and remains unchanged. Is that not the case?
Stale comment. Fixed
Christoph Schweringnote - this doesn't compile. you're missing the name for this parameter
e.g. const FormData& form
Parameters don't need names (https://en.cppreference.com/w/cpp/language/function.html#Parameter_list), but I also think we should have one for consistency.
It built for me.. added the parameter name
How about naming this `RequestRefill()`?
"Refill" sounds more familiar to those who who are familiar with refills, and IIUC for `BrowserAutofillManager` it's the the behaviour that we trigger.
And "request" makes it sound less binding. For example, if the originally filled field is gone, Chrome won't refill the form (at least now).
Renamed
virtual void RetriggerAutofill(const FormData& form);Yoav Weiss (@Shopify)nit: `On...`
Done
void NoOp(AutofillManager&) {}Yoav Weiss (@Shopify)What does this mean?
Removed
.Then(NotifyObserversCallback(&Observer::NoOp)));If we don't want an `Observer` event, just remove this?
Otherwise, there should be an `OnBeforeFoo()` and an `OnAfterFoo()` event.
- Cathy Li
- Christoph Schwering
- Jan Keitel
- Kinuko Yasuda
Christoph Schwering (Gerrit)
Christoph Schwering added 1 comment![]( "Open in Gerrit")
if (autofill_retrigger_state_ == AutofillRetriggerState::kNotWaiting &&
base::FeatureList::IsEnabled(blink::features::kAddressAutofillAPI) &&
form_util::DispatchAutofillEvent(document, is_undo, fields) ==
AutofillEventResult::DELAY) {
autofill_retrigger_state_ = AutofillRetriggerState::kWaiting;
// We need to delay autofill triggering until the AutofillEvent
// prepareForm promise is resolved.
// TODO: Kick off a timer here that will retrigger the autofill on the
// same form if the promise is not resolved in that time.
return;
}
autofill_retrigger_state_ = AutofillRetriggerState::kNotWaiting;A suggestion to reduce overall complexity:
- Chrome supports only address and credit card refills.
- WebView supports no refills at all.
How about we add a parameter to `ApplyFieldsAction()` (say, `bool fire_js_event`), which we always set to `false` except for non-refill address or credit card fils ([here](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/filling/form_filler.cc;l=920-926;drc=30914745e420f4e9b1e95dd8dffe37441f7c779e)).
I left that comment half-finished:
In the `AutofillAgents::ApplyFieldsAction()`, we'd only fire the JavaScript event if `fire_js_event` is true.
That way, we won't need to extend Chrome's refill mechanism and build a refill mechanism for WebView, both of which may be significant pieces of work. We can of course still implement them in the future.
The spec may need to be relaxed though to tolerate this, i.e., to allow the browser not to fill the event. I think currently it does not allow that, right?
Related details
- Cathy Li
- Jan Keitel
- Kinuko Yasuda
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering added 1 comment![]( "Open in Gerrit")
bool waiting_for_autofill_retrigger_ = false;Suppose `waiting_for_autofill_retrigger` is true. How will we deal with `ApplyFieldsAction()` messages coming in?
Could you add a test for that two frames requesting a refill works?
I just thought it doesn't because it I thought the refill mechanism has a limitation to at most 1 refill, but I can't find such a limit so I guess I was wrong.
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
if (autofill_retrigger_state_ == AutofillRetriggerState::kNotWaiting &&
base::FeatureList::IsEnabled(blink::features::kAddressAutofillAPI) &&
form_util::DispatchAutofillEvent(document, is_undo, fields) ==
AutofillEventResult::DELAY) {
autofill_retrigger_state_ = AutofillRetriggerState::kWaiting;
// We need to delay autofill triggering until the AutofillEvent
// prepareForm promise is resolved.
// TODO: Kick off a timer here that will retrigger the autofill on the
// same form if the promise is not resolved in that time.
return;
}
autofill_retrigger_state_ = AutofillRetriggerState::kNotWaiting;Christoph SchweringA suggestion to reduce overall complexity:
- Chrome supports only address and credit card refills.
- WebView supports no refills at all.
How about we add a parameter to `ApplyFieldsAction()` (say, `bool fire_js_event`), which we always set to `false` except for non-refill address or credit card fils ([here](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/filling/form_filler.cc;l=920-926;drc=30914745e420f4e9b1e95dd8dffe37441f7c779e)).
I left that comment half-finished:
In the `AutofillAgents::ApplyFieldsAction()`, we'd only fire the JavaScript event if `fire_js_event` is true.
That way, we won't need to extend Chrome's refill mechanism and build a refill mechanism for WebView, both of which may be significant pieces of work. We can of course still implement them in the future.
The spec may need to be relaxed though to tolerate this, i.e., to allow the browser not to fill the event. I think currently it does not allow that, right?
If the browser doesn't trigger a refill, that would create a problem from the web developer's perpective. Right now developers have all kinda of hacks to deal with browsers that don't refill. They'd need to create more hacks to deal with browsers that support the autofill event, yet don't refill after the promise is resolved..
Related details
- Cathy Li
- Christoph Schwering
- Jan Keitel
- Kinuko Yasuda
Yoav Weiss (@Shopify) (Gerrit)
To be clear, I'm all for the "support only address and credit card refills" part. But I think we need to support refills (at least to some extent) on all platforms that support the autofill event.
Christoph Schwering (Gerrit)
Christoph Schwering added 1 comment![]( "Open in Gerrit")
They'd need to create more hacks to deal with browsers that support the autofill event, yet don't refill after the promise is resolved..
I agree. I think the `fire_js_event` addresses that by not firing the `autofill` event in the first place.
Let me add some details:
---
Re refilling other non-addresses, non-credit-card things: To clarify, Chrome currently also supports or is building support for: usernames, passwords, OTPs, Autocomplete (single-field data without specific semantics), "Help me write", IBANs, passports, driver's licenses, ... (code pointers are [this](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/filling/form_filler.h;l=45-49;drc=9a326bd4b652f9c3f55103c1fd902a1a468ee37f) and [this](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/single_field_fillers/single_field_fill_router.h;l=70-82;drc=639b11a3b1c98212e81a9e49ad03672fd5569043), but that's not a complete list). Implementing refills for them isn't trivial for two reasons:
(1) Proper refilling isn't trivial for security reasons. For example, refills are limited to the same `FieldTypeGroup`s that were filled in the original fill ([code](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/filling/form_filler.cc;l=436-437;drc=30914745e420f4e9b1e95dd8dffe37441f7c779e)). But there's no meaningful `FieldTypeGroup` that'd distinguish between the different `AttributeType`s of an `EntityType`, e.g., between a driver's license number and the name on the license. There are no plans to build this at the moment.
(2) Simply sending the same filling data to the renderer a second time won't work: while we do [remember the `FormData`](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/filling/form_filler.cc;l=441-443;drc=ae249e81ef263546d4b6d1cd5fbb5101e6f94374) in some cases, deriving the refill data from that alone isn't trivial either. It'd be hacky at least, and in cases where the data-to-fill was determined outside of the core Autofill code (like with "Help me write"), I don't see any practical way of doing that.
So I think the browser must be able to skip the `autofill` event. Adding a `bool fire_js_event` parameter for `ApplyFieldsAction()` that skips the `autofill` event seems like the simplest way to do that.
---
Re WebView: I agree it'd be great to support the `onautofill` on WebView and refills on WebView. But option A from Cathy's comment looks quite complex to me. So it'd be good if we can decouple WebView from the CL and to unblock project, right?
I'm not suggesting to never implement support for WebView.
---
So I thought: We need something like a `bool fire_js_event` parameter to disable the mechanism. Since we need it anyway, we can use it to decouple WebView.
WDYT?
Related details
- Cathy Li
- Jan Keitel
- Kinuko Yasuda
- Yoav Weiss (@Shopify)
Cathy Li (Gerrit)
Cathy Li added 1 comment![]( "Open in Gerrit")
I'm fine with decoupling this from WebView and have a separate WebView change to at least call the refill.
My thought for WebView is a 2-step process, but the overall code could be a bit challenging:
Step 1: We can have some necessary methods that hook up all the way to the Java side of AutofillProvider. The Java implementation can be stubbed out for something like NotifyRefillReady() or something. This step allows me to run potential public tests on WebView through Facebook's integration, and potentially help with overall direction.
Step 1.5: If we want to implement a potentially throw-away implementation where the webview itself intrinsically triggers the refill, my thinking is we can store the virtual ID -> values array from the autofill() call on the Java side and translate it to a FieldGlobalId -> values list to C++. Refill would happen on all fields whose global id still exists. Imperfect, but should still be no worse than current implementation.
Step 2: Based on findings from 1, we can write a proposal for how to get other Autofill services onboarded with this API through Android's AutofillManager integration and make the API changes on Android with the understanding that it's a bigger, more complicated process. We'd have at least one data point that the API is sound.
Related details
Cathy Li (Gerrit)
Thinking it'd be good to verify my assumptions first:
The proposed API is to get a list of fields -> values so JS developers can pre-prepare the form and send back a notification that the form is ready. The browser can either re-examine the form for re-fill, or re-initiate the previously paused fill.
For compatibility:
If the browser fires the js event, then it should wait for the callback:
1. if browser gets a callback, it should notify the autofill service (Chrome's internal autofill for Chrome browser, or the registered AutofillService for WebViews).
1.1 If the service ignores the re-fill call, we should re-initiate the previously paused fill
1.2 If the service does not ignore by calling TriggerRefill(), we use the refill values (form or otherwise) to trigger the fill.
2. If browser does not get a callback after a set period, it will do 1.1 above
And this thread is whether to extend Chrome's refill capabilities for chrome browser in case of 1.2, and how to have that work in WebViews (probably fork to separate CL)
An alternate question is whether 1.1 (re-initiating the previously paused fill) should happen on a deeper level (e.g. autofill_agent/autofill_driver) or at the autofill_provider (webview) / browser_autofill_manager level?
But Christoph's thought is if we are triggering the JS event, maybe we completely skip the autofill altogether, and rely on autofill service to know/detect that their autofill was skipped, and to re-issue... I think this works for Chrome browser because everything is in-house. It doesn't work well for WebView because with the AutofillManager as a middle-man between the WebView and the AutofillService, there's almost no way to tell the Service that their last fill was just ignored, and may cause system autofill service for WebViews to just break down completely.
This makes me think we should bring the refill mechanism to the autofill_provider/browser_autofill_manager level, where chrome can ignore, but autofill_provider issues a refill with last known fill data using either cached FormData or a FieldGlobalId -> values map.