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

Issue 780994 link

Starred by 2 users

Issue metadata

Status: Archived
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

In-OS modal dialog should not display a shield underneath them

Project Member Reported by sgabr...@chromium.org, Nov 2 2017

Issue description

Linked to  bug 380937 . 

Currently modal dialogs in web-ui settings display a shield on top of the settings UI (and their should keep them).

In the case where we use these dialogs (or any dialogs) in OS, meaning as a floating window with a native top bar, we need to remove all shields.
 
4b43818e-b39f-4ef3-8e8e-796e0efd5e79.png
133 KB View Download
The "shield" is part of being modal (i.e. only the dialog can be interacted with), is the desire to make these dialogs non modal?

ah I see. I would say yes unless you see a reason not to do it. Do you think of any use case where making it non-modal would create issue?
Making any dialogs spawned by the System Tray non modal unless they must be modal for some reason sounds reasonable to me.

This includes internet config, bluetooth pairing, set date/time, and possibly others. I will do an audit and update this with the list.

+kuscher@, +tbuckley@ - Does that sound reasonable?

While we are updating these, a few other things I would like to make consistent:

* Should they have a title bar with just a close icon (current behavior for internet dialogs)? The Bluetooth pairing dialog does not have a title bar, but is also not currently draggable (I'm not sure how difficult it would be to fix that).
* Should there be any sizing guidelines, or should each dialog be sized to match its content?
* Button styling should be as for the specs for the internet spec here?
  https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZZ9oo1_la-_D/files/MCG17nfZaSnyATvK0-zBNL8rwMSJbxAVNpw

Cc: kuscher@chromium.org
* Should they have a title bar with just a close icon (current behavior for internet dialogs)? The Bluetooth pairing dialog does not have a title bar, but is also not currently draggable (I'm not sure how difficult it would be to fix that).
Yes.

* Should there be any sizing guidelines, or should each dialog be sized to match its content?
Let's make them 460 width fixed, 480 height max. Height is adapting if smaller.

* Button styling should be as for the specs for the internet spec here?
  https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZZ9oo1_la-_D/files/MCG17nfZaSnyATvK0-zBNL8rwMSJbxAVNpw
Yes.

Making dialogs non-modal sounds good to me.
Labels: Needs-Feedback
More questions:
* 460 x 480 looks fine for the internet config dialog, but other Settings based dialogs (internet details and bluetooth pairing) work better at 640 wide (which is their natural width in Settings). What do you suggest?

* By default, not-modal dialogs are not always-on-top, which is good for focusing other windows, but allows the dialogs to become "buried" (they do not have a launcher icon since they are dialogs). Thoughts?

* I used the size that I found in the current settings (if I recall properly). Do you mean they should both be (in web-ui and in os) 640px?

* Can we make it non modal but bring it on top and focused when triggered?
Status: Started (was: Assigned)
* As discussed offline, I will try 512 as an intermediate size that is smaller than the Settings width (so that the dialog looks like a dialog when embedded in Settings) but wide enough to fit all 3 UIs (Internet Config, Internet Details, Bluetooth Pairing).

* Every time a stand-alone dialog is "triggered" it opens a new dialog; we do not currently keep track of open dialogs. I could probably modify the code to close any existing dialogs of the same type when a new one of that type is opened so that we do not "loose" a bunch of dialogs, but it will be a bit more work to do so. Alternatively I could keep these "always on top" without making them modal.

> Alternatively I could keep these "always on top" without making them modal. 
That actually sounds like a pretty good solution. Let's try that.
Project Member

Comment 11 by bugdroid1@chromium.org, Nov 30 2017

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

commit 95cd21fddd5f675db0325dfb6f8872d96f086192
Author: Steven Bennetts <stevenjb@chromium.org>
Date: Thu Nov 30 02:45:37 2017

Make SystemWebDialogDelegate non modal and convert bluetooth dialog

This CL:
* Makes SystemWebDialogDelegate non-modal, always-on-top
* Converts BluetoothPairingDialog to use SystemWebDialogDelegate to
  simplify the code and make it consistent with Internet dialogs.

Bug:  780569 ,  780994 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I98aa584b8cfb7a31647e952ab7d2a4939924ba08
Reviewed-on: https://chromium-review.googlesource.com/792636
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520397}
[modify] https://crrev.com/95cd21fddd5f675db0325dfb6f8872d96f086192/chrome/browser/browser_resources.grd
[modify] https://crrev.com/95cd21fddd5f675db0325dfb6f8872d96f086192/chrome/browser/chromeos/BUILD.gn
[delete] https://crrev.com/1a02e4a41a2491f91ef87f3c7752ea79a4d17e5c/chrome/browser/chromeos/bluetooth/bluetooth_pairing_dialog.cc
[delete] https://crrev.com/1a02e4a41a2491f91ef87f3c7752ea79a4d17e5c/chrome/browser/chromeos/bluetooth/bluetooth_pairing_dialog.h
[delete] https://crrev.com/1a02e4a41a2491f91ef87f3c7752ea79a4d17e5c/chrome/browser/resources/chromeos/bluetooth_dialog_host.html
[delete] https://crrev.com/1a02e4a41a2491f91ef87f3c7752ea79a4d17e5c/chrome/browser/resources/chromeos/bluetooth_pair_device.html
[add] https://crrev.com/95cd21fddd5f675db0325dfb6f8872d96f086192/chrome/browser/resources/chromeos/bluetooth_pairing_dialog.html
[rename] https://crrev.com/95cd21fddd5f675db0325dfb6f8872d96f086192/chrome/browser/resources/chromeos/bluetooth_pairing_dialog.js
[modify] https://crrev.com/95cd21fddd5f675db0325dfb6f8872d96f086192/chrome/browser/resources/chromeos/compiled_resources2.gyp
[modify] https://crrev.com/95cd21fddd5f675db0325dfb6f8872d96f086192/chrome/browser/resources/chromeos/internet_config_dialog.html
[modify] https://crrev.com/95cd21fddd5f675db0325dfb6f8872d96f086192/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/95cd21fddd5f675db0325dfb6f8872d96f086192/chrome/browser/ui/ash/system_tray_client.cc
[modify] https://crrev.com/95cd21fddd5f675db0325dfb6f8872d96f086192/chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc
[add] https://crrev.com/95cd21fddd5f675db0325dfb6f8872d96f086192/chrome/browser/ui/webui/chromeos/bluetooth_pairing_dialog.cc
[add] https://crrev.com/95cd21fddd5f675db0325dfb6f8872d96f086192/chrome/browser/ui/webui/chromeos/bluetooth_pairing_dialog.h
[rename] https://crrev.com/95cd21fddd5f675db0325dfb6f8872d96f086192/chrome/browser/ui/webui/chromeos/bluetooth_pairing_dialog_browsertest-inl.h
[rename] https://crrev.com/95cd21fddd5f675db0325dfb6f8872d96f086192/chrome/browser/ui/webui/chromeos/bluetooth_pairing_dialog_browsertest.js
[delete] https://crrev.com/1a02e4a41a2491f91ef87f3c7752ea79a4d17e5c/chrome/browser/ui/webui/chromeos/bluetooth_pairing_ui.cc
[delete] https://crrev.com/1a02e4a41a2491f91ef87f3c7752ea79a4d17e5c/chrome/browser/ui/webui/chromeos/bluetooth_pairing_ui.h
[modify] https://crrev.com/95cd21fddd5f675db0325dfb6f8872d96f086192/chrome/browser/ui/webui/chromeos/internet_config_dialog.cc
[modify] https://crrev.com/95cd21fddd5f675db0325dfb6f8872d96f086192/chrome/browser/ui/webui/chromeos/internet_config_dialog.h
[modify] https://crrev.com/95cd21fddd5f675db0325dfb6f8872d96f086192/chrome/browser/ui/webui/chromeos/internet_detail_dialog.cc
[modify] https://crrev.com/95cd21fddd5f675db0325dfb6f8872d96f086192/chrome/browser/ui/webui/chromeos/internet_detail_dialog.h
[modify] https://crrev.com/95cd21fddd5f675db0325dfb6f8872d96f086192/chrome/browser/ui/webui/chromeos/system_web_dialog_delegate.cc
[modify] https://crrev.com/95cd21fddd5f675db0325dfb6f8872d96f086192/chrome/browser/ui/webui/chromeos/system_web_dialog_delegate.h
[modify] https://crrev.com/95cd21fddd5f675db0325dfb6f8872d96f086192/chrome/test/BUILD.gn
[modify] https://crrev.com/95cd21fddd5f675db0325dfb6f8872d96f086192/chrome/test/data/webui/BUILD.gn
[modify] https://crrev.com/95cd21fddd5f675db0325dfb6f8872d96f086192/ui/webui/resources/cr_components/chromeos/bluetooth_dialog.html
[modify] https://crrev.com/95cd21fddd5f675db0325dfb6f8872d96f086192/ui/webui/resources/cr_components/chromeos/bluetooth_dialog.js

Status: Fixed (was: Started)
Project Member

Comment 13 by bugdroid1@chromium.org, Dec 1 2017

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

commit a1310b22e29a9faa9c693fe41c3132fc6d3f9037
Author: Steven Bennetts <stevenjb@chromium.org>
Date: Fri Dec 01 22:36:39 2017

Settings: Add missing network dialog strings

Bug:  780994 
Change-Id: I549f606839d8fd36facd6a729fd1be27be9a3a09
Reviewed-on: https://chromium-review.googlesource.com/803628
Reviewed-by: Ben Chan <benchan@chromium.org>
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521096}
[modify] https://crrev.com/a1310b22e29a9faa9c693fe41c3132fc6d3f9037/chrome/browser/ui/webui/chromeos/internet_config_dialog.cc
[modify] https://crrev.com/a1310b22e29a9faa9c693fe41c3132fc6d3f9037/chrome/browser/ui/webui/chromeos/internet_detail_dialog.cc

Status: Archived (was: Fixed)

Sign in to add a comment