In-OS modal dialog should not display a shield underneath them |
||||||
Issue descriptionLinked 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.
,
Nov 2 2017
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?
,
Nov 2 2017
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
,
Nov 2 2017
,
Nov 3 2017
* 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.
,
Nov 16 2017
Making dialogs non-modal sounds good to me.
,
Nov 28 2017
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?
,
Nov 28 2017
* 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?
,
Nov 28 2017
* 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.
,
Nov 28 2017
> Alternatively I could keep these "always on top" without making them modal. That actually sounds like a pretty good solution. Let's try that.
,
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
,
Nov 30 2017
,
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
,
Jul 30
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by steve...@chromium.org
, Nov 2 2017