Track autofillable elements using the TrackedElement framework [chromium/src : main]
Nan Lin (Gerrit)
Nan Lin added 2 comments![]( "Open in Gerrit")
UpdateAutofillableElementTracking();We can't do this in the constructor, as it will call the pure virtual function HTMLFormControlElement::FormControlType(().
Related details
- Khushal Sagar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Khushal Sagar (Gerrit)
Khushal Sagar added 5 comments![]( "Open in Gerrit")
bool HTMLFormControlElement::ShouldTrackAutofillableElement() const {nit: IsEligibleForAutofillableElementTracking? Because this is deciding if the element should be considered for tracking but whether we should actually track depends on the runtime feature check you have below.
return IsAutofillable();Since this depends on autofillable state changing, UpdateAutofillableElementTracking() needs to be called whenever that state changes. Does blink get a callback for it?
All the tests here are manually calling UpdateAutofillableElementTracking() after that bit flips but what will call this in production?
// We only need to track the element if it's rendered.Does it matter? If it's not in the layout it won't be in the painted content anyway. That avoids having to call the update function when the layout state changes.
// TODO(b/469401911): Handle <select> related data as well.What special case do we need for select element? Is there a scenario where it should be redacted even if its not autofillable?
UpdateAutofillableElementTracking();I'm not following why this needs to be called here...
Related details
- Nan Lin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nan Lin (Gerrit)
Nan Lin added 6 comments![]( "Open in Gerrit")
bool HTMLFormControlElement::ShouldTrackAutofillableElement() const {nit: IsEligibleForAutofillableElementTracking? Because this is deciding if the element should be considered for tracking but whether we should actually track depends on the runtime feature check you have below.
This was to align with ShouldTrackPassword(), I'll rename that as well. Thanks.
return IsAutofillable();Since this depends on autofillable state changing, UpdateAutofillableElementTracking() needs to be called whenever that state changes. Does blink get a callback for it?
All the tests here are manually calling UpdateAutofillableElementTracking() after that bit flips but what will call this in production?
`InAutofillable()` eventually calls [`GetAutofillFormControlType()`](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/content/renderer/form_autofill_util.cc;drc=5acb55861d6ffcd18e43831faccba49abbfef0b7;l=2364?q=IsAutofillable&ss=chromium) which depends on the input type. The bit is flipped manually in the unit tests as the autofill client is not available. On production,`UpdateAutofillableElementTracking()` will be called whenever the form control type may change.
// We only need to track the element if it's rendered.Does it matter? If it's not in the layout it won't be in the painted content anyway. That avoids having to call the update function when the layout state changes.
It doesn't really matter. But as noted in https://chromium-review.git.corp.google.com/c/chromium/src/+/7877684/comment/2e09035f_61fb95be/, unfortunately we can't do this in the constructor, as it will call the pure virtual function `HTMLFormControlElement::FormControlType()`. Or is there a better code path to trigger this?
// TODO(b/469401911): Handle <select> related data as well.What special case do we need for select element? Is there a scenario where it should be redacted even if its not autofillable?
This is mostly to align with the APC redaction which currently doesn't redact the select element. https://source.chromium.org/chromium/chromium/src/+/main:components/optimization_guide/content/browser/page_content_proto_util.cc;l=691?q=page_content_proto_util.cc&ss=chromium
To be consistent, the redaction of APC and screenshot should be in sync.
UpdateAutofillableElementTracking();I'm not following why this needs to be called here...
Since `IsAutofillable()` depends on the form control type. We call this here as single and multiple selects have different form control types. Although selects are not actually tracked yet, I think we should still call it here to avoid future errors.
Related details
- Khushal Sagar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Khushal Sagar (Gerrit)
Khushal Sagar added 4 comments![]( "Open in Gerrit")
UpdateAutofillableElementTracking();Khushal SagarWe can't do this in the constructor, as it will call the pure virtual function HTMLFormControlElement::FormControlType(().
It should be handled by the code that changes `IsAutofillable()` since the element is going to start off as not autofillable anyway. So we shouldn't need to call it at creation at all.
return IsAutofillable();Nan LinSince this depends on autofillable state changing, UpdateAutofillableElementTracking() needs to be called whenever that state changes. Does blink get a callback for it?
All the tests here are manually calling UpdateAutofillableElementTracking() after that bit flips but what will call this in production?
`InAutofillable()` eventually calls [`GetAutofillFormControlType()`](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/content/renderer/form_autofill_util.cc;drc=5acb55861d6ffcd18e43831faccba49abbfef0b7;l=2364?q=IsAutofillable&ss=chromium) which depends on the input type. The bit is flipped manually in the unit tests as the autofill client is not available. On production,`UpdateAutofillableElementTracking()` will be called whenever the form control type may change.
Can you add the production logic for dispatching UpdateAutofillableElementTracking() within this CL?
I see that whether an element is autofillable is outside Blink [here](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/content/renderer/form_autofill_util.cc;l=2364;drc=5dcb514d2b27639844ebb3acffd720651faa435d). It's based on the element's type but that's an internal detail of the autofill code. It's not a good idea to bake any assumptions about that here. So I like the idea of blink just getting a callback when autofill state has been invalidated.
// TODO(b/469401911): Handle <select> related data as well.Nan LinWhat special case do we need for select element? Is there a scenario where it should be redacted even if its not autofillable?
This is mostly to align with the APC redaction which currently doesn't redact the select element. https://source.chromium.org/chromium/chromium/src/+/main:components/optimization_guide/content/browser/page_content_proto_util.cc;l=691?q=page_content_proto_util.cc&ss=chromium
To be consistent, the redaction of APC and screenshot should be in sync.
What's final data structure being sent via TrackedElement? I can see a couple of options:
1. The renderer sends all information necessary to make the decision (including that its a select element), and the browser combines it with its own information to make the final decision. This would align closer to how APC based redaction is working, in case we shouldn't need to special-case select elements here. That logic can live in one place in the browser.
2. The renderer makes the redaction decision and only tracks elements which must be redacted if in the screenshot.
I think you're going with #1? Do you mind putting up a CL which uses the tracked element data to redact in the browser code. That'll help me see the full picture.
UpdateAutofillableElementTracking();Nan LinI'm not following why this needs to be called here...
Since `IsAutofillable()` depends on the form control type. We call this here as single and multiple selects have different form control types. Although selects are not actually tracked yet, I think we should still call it here to avoid future errors.
See comment above, it's brittle to have this assumption in blink code: IsAutofillable depends on form control type. That detail is hidden behind the chrome_client API and blink shouldn't know when that state could have changed.
Does the autofill code already get a callback when the form control type has changed? Ideally autofill code should get calls about events it needs to know about to track autofill state. And Blink should get one callback saying, "autofillability for this element has changed".
Related details
- Nan Lin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nan Lin (Gerrit)
Nan Lin added 1 comment![]( "Open in Gerrit")
UpdateAutofillableElementTracking();Nan LinI'm not following why this needs to be called here...
Khushal SagarSince `IsAutofillable()` depends on the form control type. We call this here as single and multiple selects have different form control types. Although selects are not actually tracked yet, I think we should still call it here to avoid future errors.
See comment above, it's brittle to have this assumption in blink code: IsAutofillable depends on form control type. That detail is hidden behind the chrome_client API and blink shouldn't know when that state could have changed.
Does the autofill code already get a callback when the form control type has changed? Ideally autofill code should get calls about events it needs to know about to track autofill state. And Blink should get one callback saying, "autofillability for this element has changed".
@schw...@google.com Would you mind chiming in here? Is there a way for blink to get a callback when `IsAutofillableElement()` changes on the autofill side? Thanks.
Related details
- Christoph Schwering
- Khushal Sagar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nan Lin (Gerrit)
Nan Lin added 1 comment![]( "Open in Gerrit")
UpdateAutofillableElementTracking();Nan LinI'm not following why this needs to be called here...
Khushal SagarSince `IsAutofillable()` depends on the form control type. We call this here as single and multiple selects have different form control types. Although selects are not actually tracked yet, I think we should still call it here to avoid future errors.
Nan LinSee comment above, it's brittle to have this assumption in blink code: IsAutofillable depends on form control type. That detail is hidden behind the chrome_client API and blink shouldn't know when that state could have changed.
Does the autofill code already get a callback when the form control type has changed? Ideally autofill code should get calls about events it needs to know about to track autofill state. And Blink should get one callback saying, "autofillability for this element has changed".
@schw...@google.com Would you mind chiming in here? Is there a way for blink to get a callback when `IsAutofillableElement()` changes on the autofill side? Thanks.
Based on code search, I don't find a dedicated callback from Blink within the autofill component for when a form control's type attribute changes. @schw...@google.com may know this better and hopefully can give some suggestions here.