[Android] Fix accessory bar positioning for "Suggest strong password" [chromium/src : main]
Timofey Chudakov (Gerrit)
Timofey Chudakov added 1 comment![]( "Open in Gerrit")
#if defined(ANDROID)If the fix is about setting correct field bounds, shouldn't we just set them an early return?
Related details
- Christoph Schwering
- Piotr Kotynia
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. |
Piotr Kotynia (Gerrit)
Piotr Kotynia voted and added 1 comment![]( "Open in Gerrit")
Votes added by Piotr Kotynia
| Commit-Queue | +1 |
1 comment
#if defined(ANDROID)If the fix is about setting correct field bounds, shouldn't we just set them an early return?
Unfortunately we set the field bounds much later: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view_impl.cc;l=141;drc=9b3ebe50b523222b93faee798b1e4cc2cf182118
Related details
- Christoph Schwering
- Timofey Chudakov
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. |
Piotr Kotynia (Gerrit)
Piotr Kotynia added 1 comment![]( "Open in Gerrit")
#if defined(ANDROID)Piotr KotyniaIf the fix is about setting correct field bounds, shouldn't we just set them an early return?
Unfortunately we set the field bounds much later: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view_impl.cc;l=141;drc=9b3ebe50b523222b93faee798b1e4cc2cf182118
What is even more interesting - we don't do an early return for an empty suggestions, so we still go through the "Show suggestions..." callstack. This bug only applies to the "Generate strong password" which consumes the event.
Christoph Schwering (Gerrit)
Christoph Schwering added 1 comment![]( "Open in Gerrit")
#if defined(ANDROID)Piotr KotyniaIf the fix is about setting correct field bounds, shouldn't we just set them an early return?
Piotr KotyniaUnfortunately we set the field bounds much later: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view_impl.cc;l=141;drc=9b3ebe50b523222b93faee798b1e4cc2cf182118
What is even more interesting - we don't do an early return for an empty suggestions, so we still go through the "Show suggestions..." callstack. This bug only applies to the "Generate strong password" which consumes the event.
The change looks good to me in the sense that it only affects the experiment, but I don't understand the mechanism yet. A few naive questions for my understanding:
- Does `ShowPasswordGenerationSuggestions()` call `AutofillKeyboardAccessoryControllerImpl::Show()`, but with incorrect coordinates? (Why are they incorrect?)
- IIRC the `autofill_driver->AskForValuesToFill()` call below calls `AutofillKeyboardAccessoryControllerImpl::Show()` with empty suggestions.
- Does that `Show()` call happen before the password-generation suggestion is shown? Or does it re-display the password-generation suggestion in the right place?
- Is there a race condition if `AutofillManager::AskForValuesToFill()` reparses the form asynchronously?
- How does the KA normally know which suggestions to show?
- Does `if (password_agent_handled_request) return;` lead to the same problem?
Related details
- Piotr Kotynia
- Timofey Chudakov
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. |
Piotr Kotynia (Gerrit)
Piotr Kotynia added 1 comment![]( "Open in Gerrit")
#if defined(ANDROID)Piotr KotyniaIf the fix is about setting correct field bounds, shouldn't we just set them an early return?
Piotr KotyniaUnfortunately we set the field bounds much later: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view_impl.cc;l=141;drc=9b3ebe50b523222b93faee798b1e4cc2cf182118
Christoph SchweringWhat is even more interesting - we don't do an early return for an empty suggestions, so we still go through the "Show suggestions..." callstack. This bug only applies to the "Generate strong password" which consumes the event.
The change looks good to me in the sense that it only affects the experiment, but I don't understand the mechanism yet. A few naive questions for my understanding:
- Does `ShowPasswordGenerationSuggestions()` call `AutofillKeyboardAccessoryControllerImpl::Show()`, but with incorrect coordinates? (Why are they incorrect?)
- IIRC the `autofill_driver->AskForValuesToFill()` call below calls `AutofillKeyboardAccessoryControllerImpl::Show()` with empty suggestions.
- Does that `Show()` call happen before the password-generation suggestion is shown? Or does it re-display the password-generation suggestion in the right place?
- Is there a race condition if `AutofillManager::AskForValuesToFill()` reparses the form asynchronously?
- How does the KA normally know which suggestions to show?
- Does `if (password_agent_handled_request) return;` lead to the same problem?
- **"Does ShowPasswordGenerationSuggestions() call AutofillKeyboardAccessoryControllerImpl::Show(), but with incorrect coordinates?"** - No, `AutofillKeyboardAccessoryControllerImpl::Show()` is not called at all. Unfortunately, Keyboard Accessory visibility is not exclusively managed by `AutofillKeyboardAccessoryController`. While centralized management would be simpler, changing it would likely be a difficult and time-consuming task. Instead, visibility is managed by `ManualFillingController`, which is accessible from various classes. However, since only `AutofillKeyboardAccessoryController` receives field bounds from the renderer, it must remain involved in the display logic.
- **IIRC the autofill_driver->AskForValuesToFill() call below calls AutofillKeyboardAccessoryControllerImpl::Show() with empty suggestions.** - yes
- **Does that Show() call happen before the password-generation suggestion is shown? Or does it re-display the password-generation suggestion in the right place?** - technically it may re-display it in some flows (this is probably such flow). It shouldn't be visible though, because there were 2 different calls: for displaying KA [ManualFillingComponentBridge.java] and for changing suggestions vector [AutofillKeyboardAccessoryViewBridge.java], even before that change and it is not visible. Here is the CL that implemented repositioning after getting an updated bounds (https://chromium-review.googlesource.com/c/chromium/src/+/7457307).
- **Is there a race condition if AutofillManager::AskForValuesToFill() reparses the form asynchronously?** - I am not sure / I don't think so, but it shouldn't be a problem if the order is reversed.
- **How does the KA normally know which suggestions to show?** - Normally we clear the vector when user lose the focus from the field [link](https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/features/keyboard_accessory/internal/java/src/org/chromium/chrome/browser/keyboard_accessory/AutofillKeyboardAccessoryViewBridge.java;l=129;drc=b8cb84d2901aeffc0003b15622a3052de7a20023), then `AutofillManager::AskForValuesToFill()` generates the suggestions, passes them to the KA and updates the visibility of the KA [link](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/autofill/autofill_keyboard_accessory_controller_impl.cc;l=618;drc=976a71ef2a509d4391d814b52b9f4700fdf35789).
- **Does if (password_agent_handled_request) return; lead to the same problem?** - I was thinking about it. I didn't observe such issue, probably because `TryShowPasswordSuggestions` goes here [link](https://source.chromium.org/chromium/chromium/src/+/main:components/password_manager/core/browser/password_autofill_manager.cc;l=690;drc=976a71ef2a509d4391d814b52b9f4700fdf35789) and maybe triggers the KA field bounds setting logic, but from different place. I think that this PWM<->Autofill<->KA architecture is unstable, but there is nothing we can do about it in short term.
Related details
- Christoph Schwering
- Timofey Chudakov
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. |
Christoph Schwering (Gerrit)
Christoph Schwering voted and added 2 comments![]( "Open in Gerrit")
Votes added by Christoph Schwering
| Code-Review | +1 |
2 comments
#if defined(ANDROID)Piotr KotyniaIf the fix is about setting correct field bounds, shouldn't we just set them an early return?
Piotr KotyniaUnfortunately we set the field bounds much later: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view_impl.cc;l=141;drc=9b3ebe50b523222b93faee798b1e4cc2cf182118
Christoph SchweringWhat is even more interesting - we don't do an early return for an empty suggestions, so we still go through the "Show suggestions..." callstack. This bug only applies to the "Generate strong password" which consumes the event.
Piotr KotyniaThe change looks good to me in the sense that it only affects the experiment, but I don't understand the mechanism yet. A few naive questions for my understanding:
- Does `ShowPasswordGenerationSuggestions()` call `AutofillKeyboardAccessoryControllerImpl::Show()`, but with incorrect coordinates? (Why are they incorrect?)
- IIRC the `autofill_driver->AskForValuesToFill()` call below calls `AutofillKeyboardAccessoryControllerImpl::Show()` with empty suggestions.
- Does that `Show()` call happen before the password-generation suggestion is shown? Or does it re-display the password-generation suggestion in the right place?
- Is there a race condition if `AutofillManager::AskForValuesToFill()` reparses the form asynchronously?
- How does the KA normally know which suggestions to show?
- Does `if (password_agent_handled_request) return;` lead to the same problem?
- **"Does ShowPasswordGenerationSuggestions() call AutofillKeyboardAccessoryControllerImpl::Show(), but with incorrect coordinates?"** - No, `AutofillKeyboardAccessoryControllerImpl::Show()` is not called at all. Unfortunately, Keyboard Accessory visibility is not exclusively managed by `AutofillKeyboardAccessoryController`. While centralized management would be simpler, changing it would likely be a difficult and time-consuming task. Instead, visibility is managed by `ManualFillingController`, which is accessible from various classes. However, since only `AutofillKeyboardAccessoryController` receives field bounds from the renderer, it must remain involved in the display logic.
- **IIRC the autofill_driver->AskForValuesToFill() call below calls AutofillKeyboardAccessoryControllerImpl::Show() with empty suggestions.** - yes
- **Does that Show() call happen before the password-generation suggestion is shown? Or does it re-display the password-generation suggestion in the right place?** - technically it may re-display it in some flows (this is probably such flow). It shouldn't be visible though, because there were 2 different calls: for displaying KA [ManualFillingComponentBridge.java] and for changing suggestions vector [AutofillKeyboardAccessoryViewBridge.java], even before that change and it is not visible. Here is the CL that implemented repositioning after getting an updated bounds (https://chromium-review.googlesource.com/c/chromium/src/+/7457307).
- **Is there a race condition if AutofillManager::AskForValuesToFill() reparses the form asynchronously?** - I am not sure / I don't think so, but it shouldn't be a problem if the order is reversed.
- **How does the KA normally know which suggestions to show?** - Normally we clear the vector when user lose the focus from the field [link](https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/features/keyboard_accessory/internal/java/src/org/chromium/chrome/browser/keyboard_accessory/AutofillKeyboardAccessoryViewBridge.java;l=129;drc=b8cb84d2901aeffc0003b15622a3052de7a20023), then `AutofillManager::AskForValuesToFill()` generates the suggestions, passes them to the KA and updates the visibility of the KA [link](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/autofill/autofill_keyboard_accessory_controller_impl.cc;l=618;drc=976a71ef2a509d4391d814b52b9f4700fdf35789).
- **Does if (password_agent_handled_request) return; lead to the same problem?** - I was thinking about it. I didn't observe such issue, probably because `TryShowPasswordSuggestions` goes here [link](https://source.chromium.org/chromium/chromium/src/+/main:components/password_manager/core/browser/password_autofill_manager.cc;l=690;drc=976a71ef2a509d4391d814b52b9f4700fdf35789) and maybe triggers the KA field bounds setting logic, but from different place. I think that this PWM<->Autofill<->KA architecture is unstable, but there is nothing we can do about it in short term.
Thanks a lot for the detailed answers!
One more question: What'd happen if we remove the early return also on non-Android?
While centralized management would be simpler, changing it would likely be a difficult and time-consuming task.
Is there a plan to do this (as part of this feature?)? It sounds like a lot of complexity and constant source bugs
Does if (password_agent_handled_request) return; lead to the same problem? - I was thinking about it. I didn't observe such issue, probably because TryShowPasswordSuggestions goes here link and maybe triggers the KA field bounds setting logic, but from different place. I think that this PWM<->Autofill<->KA architecture is unstable, but there is nothing we can do about it in short term.
+1
I'd suspect for the other early returns, there are edge cases where the problem persists.
}Could you add a comment why we don't want to early-return on Android?
If centralizing the KA access is realistic, could you file a bug for it if we don't have one yet and mention it in the comment?
Related details
- Piotr Kotynia
- Timofey Chudakov
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. |
Timofey Chudakov (Gerrit)
Timofey Chudakov added 1 comment![]( "Open in Gerrit")
What'd happen if we remove the early return also on non-Android?
+1 to the question, we should either always return early or always set the correct coordinates.
Related details
- Piotr Kotynia
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. |