Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2754033002:
Fix the position of AutofillClient for PasswordAutofillManager when the focused form element is ins… (Closed)
|
Created:
3 years, 9 months ago by EhsanK Modified:
3 years, 9 months ago CC:
chromium-reviews, jam, gcasto+watchlist_chromium.org, darin-cc_chromium.org, vabr+watchlistpasswordmanager_chromium.org, site-isolation-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix the position of AutofillClient for PasswordAutofillManager when the focused form element is inside OOPIF
When a form element is inside an OOPIF, the coordinates which are sent from
renderer are with respect to the WebFrameWidget's coordinates and need to be
transformed to root coordinate on the browser side. Without this change the
position of the autofill pop-up is incorrect.
This CL transforms the element bound to root view's coordinate before showing
the autofill pop-up.
BUG=686129
Review-Url: https://codereview.chromium.org/2754033002
Cr-Commit-Position: refs/heads/master@{#458593}
Committed: https://chromium.googlesource.com/chromium/src/+/6f15a2dc3a3b9bd111352a58c40cf1a1ea0e9652
Patch Set 1 #
Total comments: 9
Patch Set 2 : Addressing comments and adding a test #
Total comments: 11
Patch Set 3 : Addressing creis@'s comments #
Total comments: 2
Patch Set 4 : Fixing a compile error #
Messages
Total messages: 39 (21 generated)
ekaramad@chromium.org changed reviewers: + creis@chromium.org, estark@chromium.org, vabr@chromium.org
Hello all, Please take a look at this change. Thanks. estark@: who first landed the method in https://codereview.chromium.org/2604453003 creis@: OOPIF vabr@: Owner and reviewer of estark@ CL. https://codereview.chromium.org/2754033002/diff/1/components/password_manager... File components/password_manager/content/browser/content_password_manager_driver.cc (right): https://codereview.chromium.org/2754033002/diff/1/components/password_manager... components/password_manager/content/browser/content_password_manager_driver.cc:297: password_autofill_manager_.OnShowNotSecureWarning(text_direction, I looked to me better to apply the transform here as opposed to inside AutofillClient. If I understand correctly, AutofillClient is page level so we have to submit transformed points to it instead. WDYT?
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2754033002/diff/1/components/password_manager... File components/password_manager/content/browser/content_password_manager_driver.cc (right): https://codereview.chromium.org/2754033002/diff/1/components/password_manager... components/password_manager/content/browser/content_password_manager_driver.cc:294: content::RenderWidgetHostView* rwhv = render_frame_host_->GetView(); Not sure yet if |rwhv| can be nullptr.
Thanks for the fix! I have two comments in addition to the code-bound ones: (1) Is there a way this could be tested? (2) nit: "showing the autofill client" in the description is not clear to me. Did you mean: "showing the autofill pop-up"? This LGTM once you add a test. Cheers, Vaclav https://codereview.chromium.org/2754033002/diff/1/components/password_manager... File components/password_manager/content/browser/content_password_manager_driver.cc (right): https://codereview.chromium.org/2754033002/diff/1/components/password_manager... components/password_manager/content/browser/content_password_manager_driver.cc:294: content::RenderWidgetHostView* rwhv = render_frame_host_->GetView(); On 2017/03/16 07:06:58, EhsanK wrote: > Not sure yet if |rwhv| can be nullptr. Looking at ContentAutofillDriver::TransformBoundingBoxToViewportCoordinates, the null case should be handled. https://codereview.chromium.org/2754033002/diff/1/components/password_manager... components/password_manager/content/browser/content_password_manager_driver.cc:297: password_autofill_manager_.OnShowNotSecureWarning(text_direction, On 2017/03/16 06:47:33, EhsanK wrote: > I looked to me better to apply the transform here as opposed to inside > AutofillClient. If I understand correctly, AutofillClient is page level so we > have to submit transformed points to it instead. WDYT? The browser process and the driver in particular seem like a good place to do the transformation in, and it matches the non-password autofill (ContentAutofillDriver::TransformBoundingBoxToViewportCoordinates).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks for the quick response! https://codereview.chromium.org/2754033002/diff/1/components/password_manager... File components/password_manager/content/browser/content_password_manager_driver.cc (right): https://codereview.chromium.org/2754033002/diff/1/components/password_manager... components/password_manager/content/browser/content_password_manager_driver.cc:288: key, text_direction, typed_username, options, bounds); This likely has the same bug, right?
Description was changed from ========== Fix the position of AutofillClient for PasswordAutofillManager when the focused form element is inside OOPIF When a form element is inside an OOPIF, the coordinates which are sent from renderer are with respect to the WebFrameWidget's coordinates and need to be transformed to root coordinate on the browser side. Without this change the position of the autofill client is incorrect. This CL transforms the element bound to root view's coordinate before showing the autofill client. BUG=686129 ========== to ========== Fix the position of AutofillClient for PasswordAutofillManager when the focused form element is inside OOPIF When a form element is inside an OOPIF, the coordinates which are sent from renderer are with respect to the WebFrameWidget's coordinates and need to be transformed to root coordinate on the browser side. Without this change the position of the autofill client is incorrect. This CL transforms the element bound to root view's coordinate before showing the autofill client. BUG=686129 ==========
Thanks! lgtm modulo creis's and vabr's comments https://codereview.chromium.org/2754033002/diff/1/components/password_manager... File components/password_manager/content/browser/content_password_manager_driver.cc (right): https://codereview.chromium.org/2754033002/diff/1/components/password_manager... components/password_manager/content/browser/content_password_manager_driver.cc:288: key, text_direction, typed_username, options, bounds); On 2017/03/16 16:51:49, Charlie Reis (slow) wrote: > This likely has the same bug, right? +1, in my testing I saw this happen on regular autofill dropdowns too (https://screenshot.googleplex.com/RV4eiq1nNo3). I wonder if this is a problem for non-password dropdowns like credit cards too?
On 2017/03/16 17:14:38, estark wrote: ... > > I wonder if this is a problem for non-password dropdowns like credit cards too? Seb, do you know where credit card filling drop-downs get their coordinates from? Things which go through AutofillAgent::QueryAutofillSuggestions seem to use ContentAutofillDriver::TransformBoundingBoxToViewportCoordinates, which does the correct thing. Are there any other paths in non-password autofill to display dropdowns? Cheers, Vaclav
Thank you all for the reviews! PTAL. https://codereview.chromium.org/2754033002/diff/1/components/password_manager... File components/password_manager/content/browser/content_password_manager_driver.cc (right): https://codereview.chromium.org/2754033002/diff/1/components/password_manager... components/password_manager/content/browser/content_password_manager_driver.cc:288: key, text_direction, typed_username, options, bounds); On 2017/03/16 17:14:38, estark wrote: > On 2017/03/16 16:51:49, Charlie Reis (slow) wrote: > > This likely has the same bug, right? > > +1, in my testing I saw this happen on regular autofill dropdowns too > (https://screenshot.googleplex.com/RV4eiq1nNo3). > > I wonder if this is a problem for non-password dropdowns like credit cards too? Implemented the same code. Would it be worthwhile to copy the same test for this feature? https://codereview.chromium.org/2754033002/diff/1/components/password_manager... components/password_manager/content/browser/content_password_manager_driver.cc:294: content::RenderWidgetHostView* rwhv = render_frame_host_->GetView(); On 2017/03/16 08:25:49, vabr (Chromium) wrote: > On 2017/03/16 07:06:58, EhsanK wrote: > > Not sure yet if |rwhv| can be nullptr. > > Looking at ContentAutofillDriver::TransformBoundingBoxToViewportCoordinates, the > null case should be handled. Agreed. We should check. But not sure how this can happen. Maybe when we navigate right after this is called by the renderer? https://codereview.chromium.org/2754033002/diff/1/components/password_manager... components/password_manager/content/browser/content_password_manager_driver.cc:297: password_autofill_manager_.OnShowNotSecureWarning(text_direction, On 2017/03/16 08:25:49, vabr (Chromium) wrote: > On 2017/03/16 06:47:33, EhsanK wrote: > > I looked to me better to apply the transform here as opposed to inside > > AutofillClient. If I understand correctly, AutofillClient is page level so we > > have to submit transformed points to it instead. WDYT? > > The browser process and the driver in particular seem like a good place to do > the transformation in, and it matches the non-password autofill > (ContentAutofillDriver::TransformBoundingBoxToViewportCoordinates). Acknowledged. https://codereview.chromium.org/2754033002/diff/20001/chrome/browser/chrome_s... File chrome/browser/chrome_site_per_process_browsertest.cc (right): https://codereview.chromium.org/2754033002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:509: namespace autofill { not sure if this is better on top.
Thanks! LGTM with nits. https://codereview.chromium.org/2754033002/diff/20001/chrome/browser/chrome_s... File chrome/browser/chrome_site_per_process_browsertest.cc (right): https://codereview.chromium.org/2754033002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:509: namespace autofill { On 2017/03/16 20:39:00, EhsanK wrote: > not sure if this is better on top. Yes, please move to the top. https://codereview.chromium.org/2754033002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:602: AutofillClientPoisitonWhenInsideOOPIF) { nit: typo in Position https://codereview.chromium.org/2754033002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:620: // Navigate the <iframe> to a page with <form> nit: End with period, as above. https://codereview.chromium.org/2754033002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:624: // Insert a password <input> inside the form. The comments are suggesting that there's an existing form, but that's not the case for title1.html. https://codereview.chromium.org/2754033002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:643: EXPECT_GT(autofill_client().last_element_bounds().origin().y() /* top */, This would miss cases that the popup is too low or too far right, but I guess we don't know exactly where it will be. I guess I'm ok with it, unless there's a better approach in other autofill popup tests.
Description was changed from ========== Fix the position of AutofillClient for PasswordAutofillManager when the focused form element is inside OOPIF When a form element is inside an OOPIF, the coordinates which are sent from renderer are with respect to the WebFrameWidget's coordinates and need to be transformed to root coordinate on the browser side. Without this change the position of the autofill client is incorrect. This CL transforms the element bound to root view's coordinate before showing the autofill client. BUG=686129 ========== to ========== Fix the position of AutofillClient for PasswordAutofillManager when the focused form element is inside OOPIF When a form element is inside an OOPIF, the coordinates which are sent from renderer are with respect to the WebFrameWidget's coordinates and need to be transformed to root coordinate on the browser side. Without this change the position of the autofill pop-up is incorrect. This CL transforms the element bound to root view's coordinate before showing the autofill pop-up. BUG=686129 ==========
On 2017/03/16 18:54:28, vabr (Chromium) wrote: > On 2017/03/16 17:14:38, estark wrote: > ... > > > > I wonder if this is a problem for non-password dropdowns like credit cards > too? > > Seb, do you know where credit card filling drop-downs get their coordinates > from? Things which go through AutofillAgent::QueryAutofillSuggestions seem to > use ContentAutofillDriver::TransformBoundingBoxToViewportCoordinates, which does > the correct thing. Are there any other paths in non-password autofill to display > dropdowns? > > Cheers, > Vaclav Hi Vaclav, I not super familiar with that part of the code but Roger did touch it not too long ago. Adding it here.
On 2017/03/16 18:54:28, vabr (Chromium) wrote: > On 2017/03/16 17:14:38, estark wrote: > ... > > > > I wonder if this is a problem for non-password dropdowns like credit cards > too? > > Seb, do you know where credit card filling drop-downs get their coordinates > from? Things which go through AutofillAgent::QueryAutofillSuggestions seem to > use ContentAutofillDriver::TransformBoundingBoxToViewportCoordinates, which does > the correct thing. Are there any other paths in non-password autofill to display > dropdowns? > > Cheers, > Vaclav AutofillManager::OnQueryFormFieldAutofill() handles both address and CC autofill and uses ContentAutofillDriver::TransformBoundingBoxToViewportCoordinates much as this CL does for passwords.
rogerm@chromium.org changed reviewers: + rogerm@chromium.org
lgtm
ekaramad@chromium.org changed reviewers: + kenrb@chromium.org
Thank you all for reviews. Charlie, I made some changes to the test to use a tighter bound at the end. PTAL. + kenrb@. Thanks! https://codereview.chromium.org/2754033002/diff/20001/chrome/browser/chrome_s... File chrome/browser/chrome_site_per_process_browsertest.cc (right): https://codereview.chromium.org/2754033002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:509: namespace autofill { On 2017/03/17 18:43:07, Charlie Reis (slow) wrote: > On 2017/03/16 20:39:00, EhsanK wrote: > > not sure if this is better on top. > > Yes, please move to the top. Done. https://codereview.chromium.org/2754033002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:602: AutofillClientPoisitonWhenInsideOOPIF) { On 2017/03/17 18:43:07, Charlie Reis (slow) wrote: > nit: typo in Position Done. https://codereview.chromium.org/2754033002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:620: // Navigate the <iframe> to a page with <form> On 2017/03/17 18:43:07, Charlie Reis (slow) wrote: > nit: End with period, as above. Done. https://codereview.chromium.org/2754033002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:624: // Insert a password <input> inside the form. On 2017/03/17 18:43:07, Charlie Reis (slow) wrote: > The comments are suggesting that there's an existing form, but that's not the > case for title1.html. Yes I forgot to fix the comments after changing my test a bit. Thanks! Changed some of the comments. https://codereview.chromium.org/2754033002/diff/20001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:643: EXPECT_GT(autofill_client().last_element_bounds().origin().y() /* top */, On 2017/03/17 18:43:07, Charlie Reis (slow) wrote: > This would miss cases that the popup is too low or too far right, but I guess we > don't know exactly where it will be. I guess I'm ok with it, unless there's a > better approach in other autofill popup tests. I am now using the notification about focused node change which contains bounds in screen coordinates. https://codereview.chromium.org/2754033002/diff/40001/chrome/browser/chrome_s... File chrome/browser/chrome_site_per_process_browsertest.cc (right): https://codereview.chromium.org/2754033002/diff/40001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:714: EXPECT_LT(error.Length(), 1.4143f); The conversion logic on lines 704-706 is identical to that of WebContentsImpl::OnFocusedElementChangedInFrame(), so the error here should be much smaller than this perhaps. But that being said I thought with this approach of testing we should not rely on how the exact conversion happens there and hence allow fo some room for error (1 unit in each coordinate in this case)?
Thanks! Still LGTM. https://codereview.chromium.org/2754033002/diff/40001/chrome/browser/chrome_s... File chrome/browser/chrome_site_per_process_browsertest.cc (right): https://codereview.chromium.org/2754033002/diff/40001/chrome/browser/chrome_s... chrome/browser/chrome_site_per_process_browsertest.cc:714: EXPECT_LT(error.Length(), 1.4143f); On 2017/03/20 16:45:28, EhsanK wrote: > The conversion logic on lines 704-706 is identical to that of > WebContentsImpl::OnFocusedElementChangedInFrame(), so the error here should be > much smaller than this perhaps. But that being said I thought with this approach > of testing we should not rely on how the exact conversion happens there and > hence allow fo some room for error (1 unit in each coordinate in this case)? Thanks-- I'm ok with that.
Thanks for the reviews! I will try to CQ now.
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by ekaramad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org, estark@chromium.org, rogerm@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2754033002/#ps60001 (title: "Fixing a compile error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490134433115850,
"parent_rev": "09067d4bab0caeb8e1409148c9d6beabf762fd39", "commit_rev":
"6f15a2dc3a3b9bd111352a58c40cf1a1ea0e9652"}
Message was sent while issue was closed.
Description was changed from ========== Fix the position of AutofillClient for PasswordAutofillManager when the focused form element is inside OOPIF When a form element is inside an OOPIF, the coordinates which are sent from renderer are with respect to the WebFrameWidget's coordinates and need to be transformed to root coordinate on the browser side. Without this change the position of the autofill pop-up is incorrect. This CL transforms the element bound to root view's coordinate before showing the autofill pop-up. BUG=686129 ========== to ========== Fix the position of AutofillClient for PasswordAutofillManager when the focused form element is inside OOPIF When a form element is inside an OOPIF, the coordinates which are sent from renderer are with respect to the WebFrameWidget's coordinates and need to be transformed to root coordinate on the browser side. Without this change the position of the autofill pop-up is incorrect. This CL transforms the element bound to root view's coordinate before showing the autofill pop-up. BUG=686129 Review-Url: https://codereview.chromium.org/2754033002 Cr-Commit-Position: refs/heads/master@{#458593} Committed: https://chromium.googlesource.com/chromium/src/+/6f15a2dc3a3b9bd111352a58c40c... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/6f15a2dc3a3b9bd111352a58c40c...
Message was sent while issue was closed.
The new test by this CL is flaky. Do you mind investigating and fixing it? https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWV...
Message was sent while issue was closed.
On 2017/03/22 04:37:54, stgao(ping after 24h) wrote: > The new test by this CL is flaky. Do you mind investigating and fixing it? > > https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWV... Thanks! Commenting here since there is no associated bugs for now. I cannot repro this locally. Still, if the flakiness persists, I could try moving this to interactive tests. I will keep monitoring the flakiness dashboard to see if this is still occurring. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
