Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Issue 629964 Project OldSpice Phase 1: Dismiss JavaScript dialogs on switch-away
Starred by 29 users Project Member Reported by a...@chromium.org, Jul 20 2016 Back to list
Status: Fixed
Owner:
Closed: May 31
Cc:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Feature

Blocked on:
issue 682060
issue 716794

Blocking:
issue 629324


Show other hotlists

Hotlists containing this issue:
Interventions


Sign in to add a comment
Project OldSpice is an effort to tame the power of JavaScript dialogs. The design doc is at http://bit.ly/project-oldspice

This bug is to track the implementation of phase 1, as listed in the "Possible steps towards implementation" section, namely:

• Auto-dismissal (dismissing dialogs when switching away from the tabs displaying them) + ignoring background prompt + focus stealing with alert/confirm (though they would be auto-dismissed on switching out of that tab)

 
Project Member Comment 1 by bugdroid1@chromium.org, Jul 21 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/175a82c72429ec5799455abafe70694be6654ebe

commit 175a82c72429ec5799455abafe70694be6654ebe
Author: avi <avi@chromium.org>
Date: Thu Jul 21 18:47:28 2016

Rename the chrome/browser/ui/app_modal directory to be javascript_dialogs.

It will be the future home of the JavaScript dialog tab helper code, which will not be app-modal, so the name is no longer appropriate for use.

In addition, I'm the owner of the existing dialog code (components/app_modal) so I'm going to claim ownership of the new code.

BUG= 629964 
TEST=none

Review-Url: https://codereview.chromium.org/2169593003
Cr-Commit-Position: refs/heads/master@{#406920}

[modify] https://crrev.com/175a82c72429ec5799455abafe70694be6654ebe/chrome/browser/chrome_browser_main.cc
[modify] https://crrev.com/175a82c72429ec5799455abafe70694be6654ebe/chrome/browser/ui/android/javascript_app_modal_dialog_android.cc
[delete] https://crrev.com/2cd2671b8999e8860727401ee1817d52f40c0d5c/chrome/browser/ui/app_modal/chrome_javascript_native_dialog_factory.h
[modify] https://crrev.com/175a82c72429ec5799455abafe70694be6654ebe/chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm
[add] https://crrev.com/175a82c72429ec5799455abafe70694be6654ebe/chrome/browser/ui/javascript_dialogs/OWNERS
[add] https://crrev.com/175a82c72429ec5799455abafe70694be6654ebe/chrome/browser/ui/javascript_dialogs/chrome_javascript_native_dialog_factory.h
[modify] https://crrev.com/175a82c72429ec5799455abafe70694be6654ebe/chrome/browser/ui/views/chrome_javascript_native_dialog_factory_views.cc
[modify] https://crrev.com/175a82c72429ec5799455abafe70694be6654ebe/chrome/chrome_browser_ui.gypi

Comment 3 by a...@chromium.org, Sep 21 2016
Cc: pinkerton@chromium.org ainslie@chromium.org tapted@chromium.org ellyjo...@chromium.org f...@chromium.org hwi@chromium.org emilyschechter@chromium.org a...@chromium.org shrike@chromium.org
 Issue 643355  has been merged into this issue.
Project Member Comment 5 by bugdroid1@chromium.org, Oct 12 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5d3b869bc09c586f076e3d78d8b8966b75dcf6c9

commit 5d3b869bc09c586f076e3d78d8b8966b75dcf6c9
Author: avi <avi@chromium.org>
Date: Wed Oct 12 22:00:46 2016

Simplify the JavaScriptDialogManager.

- This collapses two closely-related calls into just one.
- This allows the WebContents to specify no callbacks, which avoids calling back to it while it's being destructed. (This is a state that is not possible now, but will be possible soon.)

BUG= 629964 
TEST=existing tests

Review-Url: https://codereview.chromium.org/2412843002
Cr-Commit-Position: refs/heads/master@{#424875}

[modify] https://crrev.com/5d3b869bc09c586f076e3d78d8b8966b75dcf6c9/android_webview/browser/aw_javascript_dialog_manager.cc
[modify] https://crrev.com/5d3b869bc09c586f076e3d78d8b8966b75dcf6c9/android_webview/browser/aw_javascript_dialog_manager.h
[modify] https://crrev.com/5d3b869bc09c586f076e3d78d8b8966b75dcf6c9/chrome/browser/extensions/content_script_apitest.cc
[modify] https://crrev.com/5d3b869bc09c586f076e3d78d8b8966b75dcf6c9/chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc
[modify] https://crrev.com/5d3b869bc09c586f076e3d78d8b8966b75dcf6c9/chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h
[modify] https://crrev.com/5d3b869bc09c586f076e3d78d8b8966b75dcf6c9/components/app_modal/app_modal_dialog.cc
[modify] https://crrev.com/5d3b869bc09c586f076e3d78d8b8966b75dcf6c9/components/app_modal/app_modal_dialog.h
[modify] https://crrev.com/5d3b869bc09c586f076e3d78d8b8966b75dcf6c9/components/app_modal/javascript_app_modal_dialog.cc
[modify] https://crrev.com/5d3b869bc09c586f076e3d78d8b8966b75dcf6c9/components/app_modal/javascript_app_modal_dialog.h
[modify] https://crrev.com/5d3b869bc09c586f076e3d78d8b8966b75dcf6c9/components/app_modal/javascript_dialog_manager.cc
[modify] https://crrev.com/5d3b869bc09c586f076e3d78d8b8966b75dcf6c9/components/app_modal/javascript_dialog_manager.h
[modify] https://crrev.com/5d3b869bc09c586f076e3d78d8b8966b75dcf6c9/content/browser/devtools/protocol/devtools_protocol_browsertest.cc
[modify] https://crrev.com/5d3b869bc09c586f076e3d78d8b8966b75dcf6c9/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/5d3b869bc09c586f076e3d78d8b8966b75dcf6c9/content/browser/web_contents/web_contents_impl_browsertest.cc
[modify] https://crrev.com/5d3b869bc09c586f076e3d78d8b8966b75dcf6c9/content/browser/web_contents/web_contents_impl_unittest.cc
[modify] https://crrev.com/5d3b869bc09c586f076e3d78d8b8966b75dcf6c9/content/public/browser/javascript_dialog_manager.h
[modify] https://crrev.com/5d3b869bc09c586f076e3d78d8b8966b75dcf6c9/content/shell/browser/shell_javascript_dialog_manager.cc
[modify] https://crrev.com/5d3b869bc09c586f076e3d78d8b8966b75dcf6c9/content/shell/browser/shell_javascript_dialog_manager.h
[modify] https://crrev.com/5d3b869bc09c586f076e3d78d8b8966b75dcf6c9/extensions/browser/guest_view/web_view/javascript_dialog_helper.cc
[modify] https://crrev.com/5d3b869bc09c586f076e3d78d8b8966b75dcf6c9/extensions/browser/guest_view/web_view/javascript_dialog_helper.h

Project Member Comment 6 by bugdroid1@chromium.org, Oct 19 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7a1b55bb6e44c34caad1913f8ca4ca43b917eb29

commit 7a1b55bb6e44c34caad1913f8ca4ca43b917eb29
Author: avi <avi@chromium.org>
Date: Wed Oct 19 04:18:38 2016

Make JavaScript dialogs auto-dismiss on tab switch.

This is a Views-only implementation for now. This works on the Mac too, but is ugly; a real Mac version is to come.

Behind the feature flag "AutoDismissingDialogs".

BUG= 629964 

Review-Url: https://chromiumcodereview.appspot.com/2421943002
Cr-Commit-Position: refs/heads/master@{#426133}

[modify] https://crrev.com/7a1b55bb6e44c34caad1913f8ca4ca43b917eb29/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/7a1b55bb6e44c34caad1913f8ca4ca43b917eb29/chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc
[modify] https://crrev.com/7a1b55bb6e44c34caad1913f8ca4ca43b917eb29/chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h
[add] https://crrev.com/7a1b55bb6e44c34caad1913f8ca4ca43b917eb29/chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc
[add] https://crrev.com/7a1b55bb6e44c34caad1913f8ca4ca43b917eb29/chrome/browser/ui/javascript_dialogs/javascript_dialog_views.h
[modify] https://crrev.com/7a1b55bb6e44c34caad1913f8ca4ca43b917eb29/chrome/browser/ui/tab_helpers.cc
[modify] https://crrev.com/7a1b55bb6e44c34caad1913f8ca4ca43b917eb29/components/app_modal/javascript_dialog_manager.cc
[modify] https://crrev.com/7a1b55bb6e44c34caad1913f8ca4ca43b917eb29/components/app_modal/javascript_dialog_manager.h

Project Member Comment 7 by bugdroid1@chromium.org, Oct 26 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/713077b86adca2120992f5432afb2b58a1ff0909

commit 713077b86adca2120992f5432afb2b58a1ff0909
Author: avi <avi@chromium.org>
Date: Wed Oct 26 19:44:04 2016

When unblocking a WebContents, don't bring it to the front if it is not already.

BUG= 629964 ,  658772 

Review-Url: https://codereview.chromium.org/2443383002
Cr-Commit-Position: refs/heads/master@{#427776}

[modify] https://crrev.com/713077b86adca2120992f5432afb2b58a1ff0909/chrome/browser/ui/browser.cc

Project Member Comment 9 by bugdroid1@chromium.org, Oct 28 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/49faa64fae69901c98ee47c81a4806b14e0a0e86

commit 49faa64fae69901c98ee47c81a4806b14e0a0e86
Author: tapted <tapted@chromium.org>
Date: Fri Oct 28 08:18:43 2016

Split javascript_dialogs.cc into views and cocoa+views versions.

This allows them both to be compiled on Mac, which is needed for
mac_views_browser (since it doesn't link any Cocoa code).

Compile step regressed at go/macviewsbuilder/builds/16832.

BUG= 629964 ,  425229 

Review-Url: https://codereview.chromium.org/2458903002
Cr-Commit-Position: refs/heads/master@{#428316}

[modify] https://crrev.com/49faa64fae69901c98ee47c81a4806b14e0a0e86/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/49faa64fae69901c98ee47c81a4806b14e0a0e86/chrome/browser/ui/javascript_dialogs/javascript_dialog.cc
[add] https://crrev.com/49faa64fae69901c98ee47c81a4806b14e0a0e86/chrome/browser/ui/javascript_dialogs/javascript_dialog_mac.cc

Project Member Comment 10 by bugdroid1@chromium.org, Oct 31 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/25a3897a86b1a6b8932b92d1a1901789366d841c

commit 25a3897a86b1a6b8932b92d1a1901789366d841c
Author: avi <avi@chromium.org>
Date: Mon Oct 31 18:42:20 2016

Don't do a callback when suppressing a dialog.

Running the callback is already handled by the calling WebContents in that case.

BUG= 629964 

Review-Url: https://codereview.chromium.org/2468473002
Cr-Commit-Position: refs/heads/master@{#428758}

[modify] https://crrev.com/25a3897a86b1a6b8932b92d1a1901789366d841c/chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc

Project Member Comment 11 by bugdroid1@chromium.org, Nov 4 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bfbb4f279b04117cf70c6ae1c1baa5686d9baa5f

commit bfbb4f279b04117cf70c6ae1c1baa5686d9baa5f
Author: avi <avi@chromium.org>
Date: Fri Nov 04 19:56:44 2016

Prevent popunders with the new auto-dismissing dialogs.

BUG= 629964 

Review-Url: https://codereview.chromium.org/2455973006
Cr-Commit-Position: refs/heads/master@{#429986}

[modify] https://crrev.com/bfbb4f279b04117cf70c6ae1c1baa5686d9baa5f/chrome/browser/ui/blocked_content/popup_blocker_browsertest.cc
[modify] https://crrev.com/bfbb4f279b04117cf70c6ae1c1baa5686d9baa5f/chrome/browser/ui/javascript_dialogs/javascript_dialog.cc
[modify] https://crrev.com/bfbb4f279b04117cf70c6ae1c1baa5686d9baa5f/chrome/browser/ui/javascript_dialogs/javascript_dialog.h
[modify] https://crrev.com/bfbb4f279b04117cf70c6ae1c1baa5686d9baa5f/chrome/browser/ui/javascript_dialogs/javascript_dialog_cocoa.mm
[modify] https://crrev.com/bfbb4f279b04117cf70c6ae1c1baa5686d9baa5f/chrome/browser/ui/javascript_dialogs/javascript_dialog_mac.cc
[modify] https://crrev.com/bfbb4f279b04117cf70c6ae1c1baa5686d9baa5f/chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc
[modify] https://crrev.com/bfbb4f279b04117cf70c6ae1c1baa5686d9baa5f/chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h
[modify] https://crrev.com/bfbb4f279b04117cf70c6ae1c1baa5686d9baa5f/chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc
[modify] https://crrev.com/bfbb4f279b04117cf70c6ae1c1baa5686d9baa5f/chrome/common/chrome_features.cc
[modify] https://crrev.com/bfbb4f279b04117cf70c6ae1c1baa5686d9baa5f/chrome/common/chrome_features.h

Project Member Comment 12 by bugdroid1@chromium.org, Nov 11 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7484f1a2039a73e59ae812d6a15e21023c9827c7

commit 7484f1a2039a73e59ae812d6a15e21023c9827c7
Author: avi <avi@chromium.org>
Date: Fri Nov 11 01:59:36 2016

Log to the console when a dialog is suppressed or would be.

BUG= 629964 

Review-Url: https://codereview.chromium.org/2491163002
Cr-Commit-Position: refs/heads/master@{#431457}

[modify] https://crrev.com/7484f1a2039a73e59ae812d6a15e21023c9827c7/chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc

Project Member Comment 13 by bugdroid1@chromium.org, Nov 15 2016
Comment 15 by a...@chromium.org, Nov 30 2016
Labels: Merge-Request-56
I just committed https://codereview.chromium.org/2535043002/ about metrics; can I get a merge to 56 for metrics about dialog closure?
Comment 16 by dimu@chromium.org, Nov 30 2016
Labels: -Merge-Request-56 Merge-Review-56 Hotlist-Merge-Review
[Automated comment] Commit may have occurred before M56 branch point (11/17/2016), needs manual review.
Labels: -Merge-Review-56 Merge-Approved-56
Approving for merge into M56 (build 2924)
Project Member Comment 18 by bugdroid1@chromium.org, Dec 1 2016
Labels: -merge-approved-56 merge-merged-2924
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/511f20e8743d2d5ef2b9ab812fb37428a9c3669c

commit 511f20e8743d2d5ef2b9ab812fb37428a9c3669c
Author: Avi Drissman <avi@chromium.org>
Date: Thu Dec 01 16:11:34 2016

Add UMA metrics to measure the dismissal cause of dialogs.

BUG= 629964 

Review-Url: https://codereview.chromium.org/2535043002
Cr-Commit-Position: refs/heads/master@{#435016}
(cherry picked from commit 70f9ea001b21e3ad255fe0d2bb7822d8e52789b7)

Review URL: https://codereview.chromium.org/2543793003 .

Cr-Commit-Position: refs/branch-heads/2924@{#247}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/511f20e8743d2d5ef2b9ab812fb37428a9c3669c/chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc
[modify] https://crrev.com/511f20e8743d2d5ef2b9ab812fb37428a9c3669c/chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h
[modify] https://crrev.com/511f20e8743d2d5ef2b9ab812fb37428a9c3669c/tools/metrics/histograms/histograms.xml

Cc: durga.behera@chromium.org jochen@chromium.org
 Issue 673166  has been merged into this issue.
Blockedon: 682060
It looks like some site are already trying to adapt to this by throwing the "Do you want to leave this site... changes will not be saved" dialog in combintation with various alert windows and autoplay audio.

Please make sure none of these features can prevent any future attempt to close a tab or window.
 Issue 682855  has been merged into this issue.
Issue 699364 has been merged into this issue.
A better alternative to the proposed auto-dismissal would be to make the dialogs modal to the tab, instead of modal to the window.  Firefox already does it this way.
Please read the design doc at http://bit.ly/project-oldspice . Firefox's approach is something we do not want to do.
Cc: rsesek@chromium.org
 Issue 714459  has been merged into this issue.
Cc: -rsesek@chromium.org
Blockedon: 716794
Project Member Comment 31 by bugdroid1@chromium.org, May 4
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/69b6c44ab3317745f442a91b24b89e4e2f6ca20b

commit 69b6c44ab3317745f442a91b24b89e4e2f6ca20b
Author: avi <avi@chromium.org>
Date: Thu May 04 16:43:54 2017

Enable AutoDismissingDialogs by default.

This allows them to be on for the continuous official Win builders.

BUG= 629964 ,717947

Review-Url: https://codereview.chromium.org/2860093002
Cr-Commit-Position: refs/heads/master@{#469360}

[modify] https://crrev.com/69b6c44ab3317745f442a91b24b89e4e2f6ca20b/chrome/common/chrome_features.cc
[modify] https://crrev.com/69b6c44ab3317745f442a91b24b89e4e2f6ca20b/testing/variations/fieldtrial_testing_config.json

Status: Fixed
Attaching a personal text document tracking CLs involved.
dialog change list.txt
1.9 KB View Download
What's the point of such a breaking change? Can't all the problems be solved simply by making modals non-native and page-modal?
Re. #35: See the design doc. In some cases, multiple pages share a JavaScript environment. When one is showing a blocking dialog, all of the others would be frozen.
Sign in to add a comment