New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 687010 link

Starred by 5 users

Issue metadata

Status: Closed
Owner:
Closed: Sep 26
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug

Blocked on:
issue 798891
issue 798893
issue 812443
issue 812452
issue 812453
issue 817461
issue 873236
issue 874537

Blocking:
issue 799334



Sign in to add a comment

OldSpice on Clank: Make JS dialogs tab-modal instead of app-modal and prevent focus stealing from prompts

Project Member Reported by k...@chromium.org, Jan 31 2017

Issue description

In the original Project OldSpice bug ( crbug.com/629964 ), we made JS dialogs modal to the tab instead of the window, enabling users to dismiss the alerts by switching tabs. We also prevented focus from being stolen by prompts.

We would like to bring this functionality to Chrome on Android.

To make the JS dialog tab-modal:
- A toolbar scrolls in if offscreen
- Scrim/shim/overlay extends to the bottom of the toolbar
- Dialog can sit up to 8dp over the toolbar, and is centered within that and the scrim space
- The overflow menu is muted/inaccessible

Mocks: https://folio.googleplex.com/chrome-ux/mocks/418-clank-dialogs/011317_TabModalClankDialogs#%2F04_PermissionFlow.png%3Fz=width
UI Review Thread: https://groups.google.com/a/google.com/forum/#!topic/chrome-ui-review/H93Jt4CsxjQ
 

Comment 1 by rolfe@chromium.org, Mar 22 2017

Cc: -rolfe@chromium.org hannahs@chromium.org
+hannahs as systems/standards overlap

Comment 2 by rolfe@chromium.org, Mar 31 2017

Cc: k...@chromium.org
Implementation deck started for when ready to go:
https://docs.google.com/a/google.com/presentation/d/1Vv595_25SJ7f7ed8JIVJqPOkzbXRByepnqwvd7bKMR8/edit?usp=sharing
Owner: huayinz@chromium.org
Status: Assigned (was: Untriaged)
Labels: Hotlist-UX-Backlog-Hannahs
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/99c228041e1e693cd28661dda4422e963cfde70a

commit 99c228041e1e693cd28661dda4422e963cfde70a
Author: Becky Zhou <huayinz@chromium.org>
Date: Thu Oct 19 05:09:27 2017

[TabModal] Import JavaScript dialog tab helper code to Android

Create a dialog class that is managed by JavaScriptDialogTabHelper
for Android and show dummy "app modal" AlertDialog for now.

Bug:  687010 
Change-Id: Iea8a5ff759f5975db4d67c1090082cfbe341c0bc
Reviewed-on: https://chromium-review.googlesource.com/702925
Commit-Queue: Becky Zhou <huayinz@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510002}
[add] https://crrev.com/99c228041e1e693cd28661dda4422e963cfde70a/chrome/android/java/src/org/chromium/chrome/browser/JavascriptTabModalDialog.java
[modify] https://crrev.com/99c228041e1e693cd28661dda4422e963cfde70a/chrome/android/java_sources.gni
[modify] https://crrev.com/99c228041e1e693cd28661dda4422e963cfde70a/chrome/browser/BUILD.gn
[modify] https://crrev.com/99c228041e1e693cd28661dda4422e963cfde70a/chrome/browser/about_flags.cc
[modify] https://crrev.com/99c228041e1e693cd28661dda4422e963cfde70a/chrome/browser/android/chrome_feature_list.cc
[modify] https://crrev.com/99c228041e1e693cd28661dda4422e963cfde70a/chrome/browser/android/chrome_feature_list.h
[modify] https://crrev.com/99c228041e1e693cd28661dda4422e963cfde70a/chrome/browser/android/tab_web_contents_delegate_android.cc
[modify] https://crrev.com/99c228041e1e693cd28661dda4422e963cfde70a/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/99c228041e1e693cd28661dda4422e963cfde70a/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/99c228041e1e693cd28661dda4422e963cfde70a/chrome/browser/ui/BUILD.gn
[add] https://crrev.com/99c228041e1e693cd28661dda4422e963cfde70a/chrome/browser/ui/android/javascript_dialog_android.cc
[add] https://crrev.com/99c228041e1e693cd28661dda4422e963cfde70a/chrome/browser/ui/android/javascript_dialog_android.h
[modify] https://crrev.com/99c228041e1e693cd28661dda4422e963cfde70a/chrome/browser/ui/javascript_dialogs/javascript_dialog.h
[modify] https://crrev.com/99c228041e1e693cd28661dda4422e963cfde70a/chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc
[modify] https://crrev.com/99c228041e1e693cd28661dda4422e963cfde70a/chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.h
[modify] https://crrev.com/99c228041e1e693cd28661dda4422e963cfde70a/chrome/browser/ui/tab_helpers.cc
[modify] https://crrev.com/99c228041e1e693cd28661dda4422e963cfde70a/tools/metrics/histograms/enums.xml

hannahs@ - I have a few questions regarding to how JS dialogs should be handled in various cases:

1) Can url bar be focused? If the url bar is focused and user doesn't navigate to a different url, should we dismiss the dialog or keep it shown?

2) Should the dialog be dismissed when user goes to tab switcher but chooses that same tab again?

3) For Chrome Home, how should we handle swipe up to open bottom sheet when dialog is showing? Should we just disable it?
Hi! Some quick answers in line:

1) Can url bar be focused? If the url bar is focused and user doesn't navigate to a different url, should we dismiss the dialog or keep it shown?
I assume the original designer allowed for this for security reasons (can the user dismiss the dialog by tapping the scrim?) The pattern should probably be the same as the translate infobar -when the url bar is focused but the user returns the dialog reappears. 

2) Should the dialog be dismissed when user goes to tab switcher but chooses that same tab again?
Lets follow the same pattern as above where the dialog reappears when the tab is re-selected.

3) For Chrome Home, how should we handle swipe up to open bottom sheet when dialog is showing? Should we just disable it?
Lets use the same pattern as above again, where it gets replaced by the bottom sheet but reappears if the user swipes back down.
Thanks for the response!

Re #8:
1) The user cannot dismiss the dialog by tapping the scrim. Currently I made the dialog stays at behind the url bar and the url suggestions when url bar is focused. That's the same behavior on desktop. Is that okay? If we decide not to follow desktop, the behavior you suggest is okay too.

2) Yes I think we can do that.

3) Again the dialog is currently made behind the bottom sheet if bottom sheet is opened. When bottom sheet is closed, the dialog is brought to the front again.

Some more questions:
4) If bottom sheet is showing before dialog is shown, should we immediately show the dialog and close the bottom sheet for the user, or, show the dialog behind bottom sheet and bring it up front once bottom sheet is closed by user (or show the dialog after bottom sheet is closed)?

5) Should we consider overlapping toolbar more for multi-window mode on small screens since the dialog is really small, or having the toolbar accessible is a must in all situations?
1-3 lgtm!

4) If bottom sheet is showing before dialog is shown, should we immediately show the dialog and close the bottom sheet for the user, or, show the dialog behind bottom sheet and bring it up front once bottom sheet is closed by user (or show the dialog after bottom sheet is closed)?
Lets show it after the bottom sheet is closed so it doesn't seem like the site is taking action while the user is in CH.

5) Should we consider overlapping toolbar more for multi-window mode on small screens since the dialog is really small, or having the toolbar accessible is a must in all situations?
Would I be able to ask for a quick screenshot/video here? Since the user can't dismiss the dialog by tapping the scrim the toolbar is the only way out if the user doesn't want to take any action which is why (I believe) it was kept in. Can anyone confirm if this is true?

Comment 11 by k...@chromium.org, Nov 16 2017

Sorry - just jumping on the backlog of emails. This LGTM. For small screens, what happens if we overflow the screen size? do we just force the content to scroll?
Re #10: Screenshot attached is showing the multi-window mode javascript prompt dialog. Since there is a text editting box, the message box becomes really small and nearly unreadable.
Multi-window prompt dialog.png
408 KB View Download
Currently, it seems like dialogs either resize to a smaller width (as in the case with add to home) or it does what you've screenshot'd above (except that the scroll bar is visible + div line above the buttons to let the user know you there's more content on scroll. 

Can we do the same here? (resize when there's no image, and scroll bar + div line when there are?)
Screenshot_20171206-102021 (1).png
162 KB View Download
There is actually a scrollbar for the description part on the screenshot in #12, but seems only visible when you scroll it. In #13, do you mean that we want to make the title also scrollable with the message?
Sorry I meant that the scrollbar should always be visible when the dialog is cut off (not only on scroll). I think it also makes sense to make the title scrollable (I believe it'a also what we did for the data saver promo so lets do that). Thanks!
Project Member

Comment 16 by bugdroid1@chromium.org, Jan 3 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f2ff06878397a4e7b76a5ed21e6f81b1606e6271

commit f2ff06878397a4e7b76a5ed21e6f81b1606e6271
Author: Becky Zhou <huayinz@chromium.org>
Date: Wed Jan 03 17:30:16 2018

[TabModal] Add dialog view to tab modal javascript dialogs

Browser controls are accessible if the browser controls are not hidden
before the tab modal javascript dialog is shown. The app modal dialog
is also using the same dialog view now for consistency.

Bug:  687010 
Change-Id: I6f36e8ef06129dd220d75d08878480672993fc72
Reviewed-on: https://chromium-review.googlesource.com/757166
Commit-Queue: Becky Zhou <huayinz@chromium.org>
Reviewed-by: Ted Choc (back but slow, ping me) <tedchoc@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526726}
[modify] https://crrev.com/f2ff06878397a4e7b76a5ed21e6f81b1606e6271/chrome/android/java/res/layout/main.xml
[add] https://crrev.com/f2ff06878397a4e7b76a5ed21e6f81b1606e6271/chrome/android/java/res/layout/modal_dialog_container.xml
[add] https://crrev.com/f2ff06878397a4e7b76a5ed21e6f81b1606e6271/chrome/android/java/res/layout/modal_dialog_view.xml
[modify] https://crrev.com/f2ff06878397a4e7b76a5ed21e6f81b1606e6271/chrome/android/java/res/layout/promo_dialog_layout.xml
[modify] https://crrev.com/f2ff06878397a4e7b76a5ed21e6f81b1606e6271/chrome/android/java/res/values-v17/styles.xml
[modify] https://crrev.com/f2ff06878397a4e7b76a5ed21e6f81b1606e6271/chrome/android/java/res/values/dimens.xml
[modify] https://crrev.com/f2ff06878397a4e7b76a5ed21e6f81b1606e6271/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
[modify] https://crrev.com/f2ff06878397a4e7b76a5ed21e6f81b1606e6271/chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
[add] https://crrev.com/f2ff06878397a4e7b76a5ed21e6f81b1606e6271/chrome/android/java/src/org/chromium/chrome/browser/jsdialog/JavascriptModalDialogView.java
[add] https://crrev.com/f2ff06878397a4e7b76a5ed21e6f81b1606e6271/chrome/android/java/src/org/chromium/chrome/browser/modaldialog/AppModalPresenter.java
[add] https://crrev.com/f2ff06878397a4e7b76a5ed21e6f81b1606e6271/chrome/android/java/src/org/chromium/chrome/browser/modaldialog/ModalDialogManager.java
[add] https://crrev.com/f2ff06878397a4e7b76a5ed21e6f81b1606e6271/chrome/android/java/src/org/chromium/chrome/browser/modaldialog/ModalDialogView.java
[add] https://crrev.com/f2ff06878397a4e7b76a5ed21e6f81b1606e6271/chrome/android/java/src/org/chromium/chrome/browser/modaldialog/TabModalLifetimeHandler.java
[add] https://crrev.com/f2ff06878397a4e7b76a5ed21e6f81b1606e6271/chrome/android/java/src/org/chromium/chrome/browser/modaldialog/TabModalPresenter.java
[modify] https://crrev.com/f2ff06878397a4e7b76a5ed21e6f81b1606e6271/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
[modify] https://crrev.com/f2ff06878397a4e7b76a5ed21e6f81b1606e6271/chrome/android/java/src/org/chromium/chrome/browser/tab/TabStateBrowserControlsVisibilityDelegate.java
[modify] https://crrev.com/f2ff06878397a4e7b76a5ed21e6f81b1606e6271/chrome/android/java_sources.gni
[add] https://crrev.com/f2ff06878397a4e7b76a5ed21e6f81b1606e6271/chrome/android/javatests/src/org/chromium/chrome/browser/modaldialog/ModalDialogManagerTest.java

Blockedon: 798891
Blockedon: 798893

Comment 19 by k...@chromium.org, Jan 5 2018

Blocking: 799334
Project Member

Comment 20 by bugdroid1@chromium.org, Jan 6 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/903fd6842f256f39a5ecba231ade9551b16107a8

commit 903fd6842f256f39a5ecba231ade9551b16107a8
Author: Becky Zhou <huayinz@chromium.org>
Date: Sat Jan 06 00:21:17 2018

[TabModal] Use modal dialog views for javascript tab modal dialogs

The JavascriptTabModalDialog was using an Android dialog for display
before. This CL will make the javascript tab modal dialog looks "tab
modal".

Bug:  687010 
Change-Id: I9b3039914b02b330959010777796f02cb1c16145
Reviewed-on: https://chromium-review.googlesource.com/849479
Commit-Queue: Becky Zhou <huayinz@chromium.org>
Reviewed-by: Ted Choc (back but slow, ping me) <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527450}
[delete] https://crrev.com/03e8c4238c96d15283d5673ff934518579586c3c/chrome/android/java/src/org/chromium/chrome/browser/JavascriptTabModalDialog.java
[add] https://crrev.com/903fd6842f256f39a5ecba231ade9551b16107a8/chrome/android/java/src/org/chromium/chrome/browser/jsdialog/JavascriptTabModalDialog.java
[modify] https://crrev.com/903fd6842f256f39a5ecba231ade9551b16107a8/chrome/android/java/src/org/chromium/chrome/browser/modaldialog/ModalDialogManager.java
[modify] https://crrev.com/903fd6842f256f39a5ecba231ade9551b16107a8/chrome/android/java_sources.gni
[add] https://crrev.com/903fd6842f256f39a5ecba231ade9551b16107a8/chrome/android/javatests/src/org/chromium/chrome/browser/jsdialog/JavascriptTabModalDialogTest.java
[modify] https://crrev.com/903fd6842f256f39a5ecba231ade9551b16107a8/chrome/browser/BUILD.gn

Blockedon: 812453
Blockedon: 812452
Blockedon: 812443

Comment 24 by k...@chromium.org, Feb 15 2018

Cc: -k...@chromium.org
Project Member

Comment 25 by bugdroid1@chromium.org, Feb 23 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/008b4ee236811270f8d5d430dd5de9e79f978235

commit 008b4ee236811270f8d5d430dd5de9e79f978235
Author: Becky Zhou <huayinz@chromium.org>
Date: Fri Feb 23 22:13:52 2018

[TabModal] Move custom view initialization in JavascriptModalDialogView

Moved the initialization for params to be only in the static create()
method.
Discussed here: https://chromium-review.googlesource.com/915105

Bug:  687010 
Change-Id: I1b8da988ab9879d3d4fe9f4567ef2d809a32c4ee
Reviewed-on: https://chromium-review.googlesource.com/917624
Reviewed-by: Joy Ming <jming@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Commit-Queue: Becky Zhou <huayinz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538918}
[modify] https://crrev.com/008b4ee236811270f8d5d430dd5de9e79f978235/chrome/android/java/src/org/chromium/chrome/browser/jsdialog/JavascriptModalDialogView.java
[modify] https://crrev.com/008b4ee236811270f8d5d430dd5de9e79f978235/chrome/android/java/src/org/chromium/chrome/browser/modaldialog/ModalDialogView.java

Blockedon: 817461
Project Member

Comment 27 by bugdroid1@chromium.org, Apr 7 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/508412496c3ee709169805984348b19cfdb7f67a

commit 508412496c3ee709169805984348b19cfdb7f67a
Author: Becky Zhou <huayinz@chromium.org>
Date: Sat Apr 07 01:28:27 2018

[TabModal] Update foremost logic for JS dialog to match desktop behavior

User interactability is too strict for deciding whether we should
auto dismiss the dialog (test case described in  bug 682060 ).

Instead, we check whether the current web contents is active, and let
the Android side hold on to the dialog until user becomes interactable.

Bug:  687010 , 828621
Change-Id: Ib40ceb9a0d71538e1832bda71860ca4622818a2c
Reviewed-on: https://chromium-review.googlesource.com/988733
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Commit-Queue: Becky Zhou <huayinz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549008}
[modify] https://crrev.com/508412496c3ee709169805984348b19cfdb7f67a/chrome/android/java/src/org/chromium/chrome/browser/browseractions/BrowserActionsTabModelSelector.java
[modify] https://crrev.com/508412496c3ee709169805984348b19cfdb7f67a/chrome/android/java/src/org/chromium/chrome/browser/modaldialog/TabModalLifetimeHandler.java
[modify] https://crrev.com/508412496c3ee709169805984348b19cfdb7f67a/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/EmptyTabModel.java
[modify] https://crrev.com/508412496c3ee709169805984348b19cfdb7f67a/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/IncognitoTabModel.java
[modify] https://crrev.com/508412496c3ee709169805984348b19cfdb7f67a/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/SingleTabModel.java
[modify] https://crrev.com/508412496c3ee709169805984348b19cfdb7f67a/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java
[modify] https://crrev.com/508412496c3ee709169805984348b19cfdb7f67a/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelDelegate.java
[modify] https://crrev.com/508412496c3ee709169805984348b19cfdb7f67a/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelImpl.java
[modify] https://crrev.com/508412496c3ee709169805984348b19cfdb7f67a/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelJniBridge.java
[modify] https://crrev.com/508412496c3ee709169805984348b19cfdb7f67a/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java
[modify] https://crrev.com/508412496c3ee709169805984348b19cfdb7f67a/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/DocumentTabModelImpl.java
[modify] https://crrev.com/508412496c3ee709169805984348b19cfdb7f67a/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/DocumentTabModelSelector.java
[modify] https://crrev.com/508412496c3ee709169805984348b19cfdb7f67a/chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorObserverTestRule.java
[modify] https://crrev.com/508412496c3ee709169805984348b19cfdb7f67a/chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabPersistentStoreTest.java
[modify] https://crrev.com/508412496c3ee709169805984348b19cfdb7f67a/chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/document/MockDocumentTabModel.java
[modify] https://crrev.com/508412496c3ee709169805984348b19cfdb7f67a/chrome/browser/ui/android/tab_model/tab_model.h
[modify] https://crrev.com/508412496c3ee709169805984348b19cfdb7f67a/chrome/browser/ui/android/tab_model/tab_model_jni_bridge.cc
[modify] https://crrev.com/508412496c3ee709169805984348b19cfdb7f67a/chrome/browser/ui/android/tab_model/tab_model_jni_bridge.h
[modify] https://crrev.com/508412496c3ee709169805984348b19cfdb7f67a/chrome/browser/ui/android/tab_model/tab_model_list_unittest.cc
[modify] https://crrev.com/508412496c3ee709169805984348b19cfdb7f67a/chrome/browser/ui/android/tab_model/tab_model_unittest.cc
[modify] https://crrev.com/508412496c3ee709169805984348b19cfdb7f67a/chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc

Project Member

Comment 28 by bugdroid1@chromium.org, Aug 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2cb9efe882557d814a9973cd8c0448831f36fc30

commit 2cb9efe882557d814a9973cd8c0448831f36fc30
Author: Becky Zhou <huayinz@chromium.org>
Date: Fri Aug 10 16:19:03 2018

[TabModal] Enable tab modal javascript dialog by default

Bug:  687010 
Change-Id: I304d68c8ade4e6cddb2017881b80b9300d0e512d
Reviewed-on: https://chromium-review.googlesource.com/1170188
Reviewed-by: Theresa <twellington@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Becky Zhou <huayinz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582191}
[modify] https://crrev.com/2cb9efe882557d814a9973cd8c0448831f36fc30/chrome/browser/android/chrome_feature_list.cc

Blockedon: 873236
Blockedon: 874537
Status: Closed (was: Assigned)
We don't need this umbrella bug anymore. Closing this issue.
Issue 341126 has been merged into this issue.

Sign in to add a comment