Calling beforeinstallpromptevent.prompt() on page load makes the ambient mini infobar not work |
||||||
Issue description1. Visit dory.withgoogle.com and sign in 2. The mini infobar prompting you to add to homescreen should show up 3. Tap the infobar What happens: nothing. It should show a prompt for installation. The reason is that dory.withgoogle.com calls beforeinstallpromptevent.prompt() immediately on page load. Since Chrome M68, this method requires a user gesture to succeed. When there is not a user gesture, the method silently fails, BUT it calls InvalidateWeakPtrs() in AppBannerManager(). This inadvertently kills the weak pointer used by InstallableAmbientBadgeInfoBarDelegate to tell AppBannerManager to trigger the add to home screen dialog when the mini infobar is interacted with. The fix is to not unilaterally wipe weak pointers in AppBannerManager::Stop().
,
Aug 9
Requesting merge of #1 to M69. It fixes a fairly commonly triggered case of a mini-infobar not doing what it's supposed to if developers are calling a JS API in a now deprecated manner. The CL has been on Canary, and the large parts of the changes are to test files. It's otherwise quite a small and safe code change.
,
Aug 9
This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 9
Merge approved for branch 3497.
,
Aug 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8fa1bd18f23da65bfd9bf4b6a3903421fc6e9043 commit 8fa1bd18f23da65bfd9bf4b6a3903421fc6e9043 Author: Dominick Ng <dominickn@chromium.org> Date: Fri Aug 10 00:34:59 2018 Enforce beforeinstallpromptevent.prompt's user gesture requirement in the renderer. This addresses a bug where tapping the installable ambient mini-infobar on Chrome for Android may not correctly trigger the add to home screen dialog. The root cause is that the user gesture requirement for calling beforeinstallpromptevent.prompt() is checked in the browser, not the renderer, because some platforms are only just rolling out the requirement. As a result, calling prompt() without a user gesture invalidates all weak pointers when it terminates, including the weak pointer used by the mini-infobar to trigger the add to home screen dialog. This CL fixes the bug by checking the user gesture in the renderer, and not dispatching to the browser unless the gesture is present. This means that the banner pipeline will not invalidate weak pointers when a user gesture is not present. BUG= 869780 TBR=mkwst@chromium.org Change-Id: Iee7dfc76401e7141707b2d7b2e6ac5e24297ac4b Reviewed-on: https://chromium-review.googlesource.com/1158305 Reviewed-by: Dominick Ng <dominickn@chromium.org> Reviewed-by: Matt Giuca <mgiuca@chromium.org> Commit-Queue: Dominick Ng <dominickn@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#580176}(cherry picked from commit eff562302acadcedfa3a981dbb7b9f952a81ff5a) Reviewed-on: https://chromium-review.googlesource.com/1170322 Cr-Commit-Position: refs/branch-heads/3497@{#528} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/8fa1bd18f23da65bfd9bf4b6a3903421fc6e9043/chrome/browser/banners/app_banner_manager.cc [modify] https://crrev.com/8fa1bd18f23da65bfd9bf4b6a3903421fc6e9043/chrome/browser/banners/app_banner_manager.h [modify] https://crrev.com/8fa1bd18f23da65bfd9bf4b6a3903421fc6e9043/chrome/browser/banners/app_banner_manager_browsertest.cc [modify] https://crrev.com/8fa1bd18f23da65bfd9bf4b6a3903421fc6e9043/chrome/browser/installable/installable_logging.cc [modify] https://crrev.com/8fa1bd18f23da65bfd9bf4b6a3903421fc6e9043/content/shell/test_runner/app_banner_service.cc [modify] https://crrev.com/8fa1bd18f23da65bfd9bf4b6a3903421fc6e9043/content/shell/test_runner/app_banner_service.h [modify] https://crrev.com/8fa1bd18f23da65bfd9bf4b6a3903421fc6e9043/third_party/WebKit/LayoutTests/app_banner/app-banner-event-prompt.html [modify] https://crrev.com/8fa1bd18f23da65bfd9bf4b6a3903421fc6e9043/third_party/blink/public/platform/modules/app_banner/app_banner.mojom [modify] https://crrev.com/8fa1bd18f23da65bfd9bf4b6a3903421fc6e9043/third_party/blink/renderer/modules/app_banner/app_banner_controller.cc [modify] https://crrev.com/8fa1bd18f23da65bfd9bf4b6a3903421fc6e9043/third_party/blink/renderer/modules/app_banner/app_banner_controller.h [modify] https://crrev.com/8fa1bd18f23da65bfd9bf4b6a3903421fc6e9043/third_party/blink/renderer/modules/app_banner/before_install_prompt_event.cc [modify] https://crrev.com/8fa1bd18f23da65bfd9bf4b6a3903421fc6e9043/third_party/blink/renderer/modules/app_banner/before_install_prompt_event.h [modify] https://crrev.com/8fa1bd18f23da65bfd9bf4b6a3903421fc6e9043/tools/metrics/histograms/enums.xml
,
Aug 10
Fixed for now and merged to M69. We'll need to do some clean up here once ExperimentalAppBanners is removed everywhere and when user activation v2 ships (we'll consume the user gesture in Blink). |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by bugdroid1@chromium.org
, Aug 2