Credit card/payment options autofull popup does not show for forms inside OOPIFs |
|||||||||||||||
Issue descriptionChrome Version: 57.0.2987.98 OS: All What steps will reproduce the problem? (1) Start chrome with --site-per-process (or enable it in chrome://flags). (2) Navigate to http://csreis.github.io/tests/cross-site-iframe.html. Also try with https://csreis.github.io/tests/cross-site-iframe.html. (3) Inspect main page and type: navFrame("https://krfilters.com/contact-us/secure-payment-form/") in devtools. (4) Mouse click into the input field for credit card (or start typing). What is the expected result? For http:// page, A pop-up should appear to warn against non-secure content. For https:// page, an autofill pop-up with registered cards on file appears. What happens instead? Nothing happens on either of https:// and http:// pages.
,
Mar 21 2017
I did some digging into this. I found that PageClickTracker::Legacy is to be removed. The Legacy class is a RenderViewObserver while PageClickTracker is a RenderFrameObserver (perhaps for all frames or just the local root). Now the process to show the popup could go through RenderWidget::FocusChangeComplete which for main frame ends up at RenderViewImpl::RenderWidgetFocusChangeComplete and calls the observers. A quick fix for this which I have tried and works is to make RenderWidget call RenerFrames and notify focus change and RenderFrame call the RenderView. This will resolve the credit card issue more or less. There is some issues though. There are other code patch which show the popup such as ChromeClientImpl::onMouseDown which goes to RenderView again. I am also making this blocked on issue 676037 . The reason is that one of the codepaths for RenderWidget::FocusChangeComplete seems to come from ScrollFocusedEditableElement which is not implemented for OOPIFs (this means the feature might not work with touch screens). I will investigate this a bit more and log it here.
,
Mar 21 2017
Following comment #2: ChromeClientImpl::onMouseDown works fine for OOPIFs but it only works on Android. This CL is a temporary fix for this problem (minus the potential case in issue 676037 ): https://codereview.chromium.org/2766053002/
,
Mar 22 2017
Actually I might go for the better solution and remove PageClickTracker altogether following estade@'s comment: https://cs.chromium.org/chromium/src/components/autofill/content/renderer/page_click_tracker.h?rcl=561597e7145baa77888bdfa707154ce440062472&l=37 cc-ing estade@ to the bug.
,
Mar 22 2017
Correction re: comment #4: s/remove PageClickTracker/remove PageClickTracker::Legacy. The same thing will be done for AutofillAgent::LegacyAutofillAgent. cc-ing vabr@ who owns issue 433486 .
,
Mar 23 2017
,
Mar 24 2017
I would love to see LegacyAutofillAgent gone! :) Thanks for digging into this.
,
Apr 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/27ca69b1e049f9f86e15f34420eeef224335f075 commit 27ca69b1e049f9f86e15f34420eeef224335f075 Author: ekaramad <ekaramad@chromium.org> Date: Thu Apr 20 18:34:29 2017 [refactor] Fix autofill features for payments when the form is inside an OOPIF Currently, clicking into a credit card information <input> field inside an OOPIF does not pop up the autofill window. The root cause is simply because the call to RenderWidget::FocusChangeComplete() is dropped for OOPIFs. Moreover, for the main frame, this call is forwarded throught RenderViewObserver calls to the corresponding PageClickTracker. On the other hand, both PageClickTracker and AutofillAgent use a internal Legacy class to observe RenderView for updates in element focus and mouse down down inside a node. This is unfortunate given that the actual classes are RenderFrameObservers. This CL will make the following changes: A) FocusChangeComplete: This call is now forwarded to the observers of the RenderWidget which are RenderFrames. They will then notify their own observers, e.g., AutofillAgent and PageClickTracker. In line with this, the Legacy classes inside these classes will be removed. Also, there is no longer a need for RenderViewImpl::RenderWidgetFocusChangeComplete. B) MouseDown: Currently, in response to a left mouse button down or a gesture tap, ChromeClient notifies the WebViewClient (RenderViewImpl). This is solely used in PageClickTracker. This CL will remove the call from WebViewClient to the WebWidgetClient (RenderWidget) which similarly to FocusChangeComplete, will be forwarded to the observer RenderFrames and eventually the RenderFrameObservers. The change in A) will also fix a bug with credit card payment autofill in OOPIFs. BUG=712754, 703800 , 583347, 433486 Review-Url: https://codereview.chromium.org/2766053002 Cr-Commit-Position: refs/heads/master@{#466078} [modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/chrome/browser/autofill/autofill_interactive_uitest.cc [modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/chrome/renderer/autofill/page_click_tracker_browsertest.cc [modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/components/autofill/content/renderer/autofill_agent.cc [modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/components/autofill/content/renderer/autofill_agent.h [modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/components/autofill/content/renderer/page_click_tracker.cc [modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/components/autofill/content/renderer/page_click_tracker.h [modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/content/public/renderer/render_view_observer.h [modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/content/renderer/render_view_impl.cc [modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/content/renderer/render_view_impl.h [modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/content/renderer/render_widget.cc [modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/content/renderer/render_widget_owner_delegate.h [modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/third_party/WebKit/Source/core/input/EventHandler.cpp [modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/third_party/WebKit/Source/core/input/GestureManager.cpp [modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/third_party/WebKit/Source/core/page/ChromeClient.h [modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/third_party/WebKit/Source/web/ChromeClientImpl.cpp [modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/third_party/WebKit/Source/web/ChromeClientImpl.h [modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/third_party/WebKit/public/web/WebAutofillClient.h [modify] https://crrev.com/27ca69b1e049f9f86e15f34420eeef224335f075/third_party/WebKit/public/web/WebViewClient.h
,
Apr 21 2017
Fix in comment #8 is not in Canary yet. I will close this bug once verified on Canary. creis@: Given the SiteIsolation component to the bug (and potential regression from 56 I guess), do we need to merge this to 59?
,
Apr 21 2017
ekaramad@: I'm testing this on Mac Canary 60.0.3077.0 (cut from r466199, and thus has your r466078), but the bug still seems to be there? I don't see either the "not secure" popup or the autofill popup on the HTTP and HTTPS pages, respectively. Maybe I'm testing it incorrectly, though, since I don't see them when running without --site-per-process either. Can you double check on a Mac Canary? (This does seem like something worth merging to M59 if we can verify that it works.)
,
Apr 21 2017
csreis@: Thanks for the update! That sounds a bit strange. In guest mode and for the https:// version I don't see popups but that is because there are no cards associated with the profile. But event for the guest mode in the http:// case I can see the warning popup. I have attached a verification video to this bug. So I will be marking it as fixed. I also agree that the fix is worth merging (after it has been tested on Canary for a few days of course). This bug is perhaps a regression since 56 for extensions.
,
Apr 28 2017
Following comments #10 and #11 adding labels for merge request.
,
Apr 28 2017
Your change meets the bar and is auto-approved for M59. Please go ahead and merge the CL to branch 3071 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb commit ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb Author: EhsanK <ekaramad@chromium.org> Date: Fri Apr 28 21:46:12 2017 [refactor] Fix autofill features for payments when the form is inside an OOPIF Currently, clicking into a credit card information <input> field inside an OOPIF does not pop up the autofill window. The root cause is simply because the call to RenderWidget::FocusChangeComplete() is dropped for OOPIFs. Moreover, for the main frame, this call is forwarded throught RenderViewObserver calls to the corresponding PageClickTracker. On the other hand, both PageClickTracker and AutofillAgent use a internal Legacy class to observe RenderView for updates in element focus and mouse down down inside a node. This is unfortunate given that the actual classes are RenderFrameObservers. This CL will make the following changes: A) FocusChangeComplete: This call is now forwarded to the observers of the RenderWidget which are RenderFrames. They will then notify their own observers, e.g., AutofillAgent and PageClickTracker. In line with this, the Legacy classes inside these classes will be removed. Also, there is no longer a need for RenderViewImpl::RenderWidgetFocusChangeComplete. B) MouseDown: Currently, in response to a left mouse button down or a gesture tap, ChromeClient notifies the WebViewClient (RenderViewImpl). This is solely used in PageClickTracker. This CL will remove the call from WebViewClient to the WebWidgetClient (RenderWidget) which similarly to FocusChangeComplete, will be forwarded to the observer RenderFrames and eventually the RenderFrameObservers. The change in A) will also fix a bug with credit card payment autofill in OOPIFs. BUG=712754, 703800 , 583347, 433486 Review-Url: https://codereview.chromium.org/2766053002 Cr-Commit-Position: refs/heads/master@{#466078} (cherry picked from commit 27ca69b1e049f9f86e15f34420eeef224335f075) Review-Url: https://codereview.chromium.org/2853623002 . Cr-Commit-Position: refs/branch-heads/3071@{#304} Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641} [modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/chrome/browser/autofill/autofill_interactive_uitest.cc [modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/chrome/renderer/autofill/page_click_tracker_browsertest.cc [modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/components/autofill/content/renderer/autofill_agent.cc [modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/components/autofill/content/renderer/autofill_agent.h [modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/components/autofill/content/renderer/page_click_tracker.cc [modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/components/autofill/content/renderer/page_click_tracker.h [modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/content/public/renderer/render_view_observer.h [modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/content/renderer/render_view_impl.cc [modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/content/renderer/render_view_impl.h [modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/content/renderer/render_widget.cc [modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/content/renderer/render_widget_owner_delegate.h [modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/third_party/WebKit/Source/core/input/EventHandler.cpp [modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/third_party/WebKit/Source/core/input/GestureManager.cpp [modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/third_party/WebKit/Source/core/page/ChromeClient.h [modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/third_party/WebKit/Source/web/ChromeClientImpl.cpp [modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/third_party/WebKit/Source/web/ChromeClientImpl.h [modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/third_party/WebKit/public/web/WebAutofillClient.h [modify] https://crrev.com/ca19e7c02d4e92fbf88d3d52b25d1e5760e408eb/third_party/WebKit/public/web/WebViewClient.h
,
Sep 12 2017
,
Nov 29
|
|||||||||||||||
►
Sign in to add a comment |
|||||||||||||||
Comment 1 by ekaramad@chromium.org
, Mar 21 2017