related to harmony dialog positioning?

Huh. I think that dialog goes through this function:

https://cs.chromium.org/chromium/src/chrome/browser/chromeos/options/network_config_view.cc?type=cs&sq=package:chromium&l=282

But none of the code nearby has changed since late 2016.

-> jamescook

I don't know too much about Ash but on desktop, if a dialog should be centered in the screen, it's usually enough to specify a null parent window. So if it should *always* center in screen, then NetworkConfigView::ShowDialog(gfx::NativeWindow parent) probably shouldn't have the steps:

  // Attempt to find a fallback parent window.
  if (parent == nullptr)
    parent = GetParentForUnhostedDialog();

since that will center in the fallback parent window.

I suspect something in r428731 "chromeos: Refactor NetworkConfigView to use shared widget parent code for mash" changed the result of GetParentForUnhostedDialog().

(although as #c2 points out, that was a while ago.. maybe a later change to ViewsDelegate::OnBeforeWidgetInit or CreatePlatformNativeWidgetImpl* changed how parent params affect widget creation).

What are the repro steps? (Can we bisect this?)

I'm on the bus home, but I suspect this is bisect-able.

I can repro on a Chromebook by:
* Open a browser window
* Restore the window, make it small and not-centered
* In the status tray, click on the networking item, then the little arc with a plus sign to add a new network by name

The dialog will center on the window, not the display.

I'll bet you would get the same behavior on linux desktop if you built with target_os="chromeos" in gn args.

If it repros and you've got time to bisect it, please do. If not, I can dig in more in the morning (Pacific).

bad 59.0.3055.0
good 52.0.2743.116
good 56.0.2899.0 (426989)
bad 429500 (56.0.2909.0)

I couldn't bisect further than that - the bisect script doesn't start Chrome much earlier than 429500

r428731 still seems the likely culprit.


(and if you're trying to repro, make sure the browser window is at least wide enough to host the dialog, otherwise it seems to reparent anyway).

Removing release block since this regressed ages ago.

Thanks for the bisect information. I didn't know the bisect script worked with chromeos builds these days. I'll take a look today.

Yep, you were right, it's my CL r428731. It's because the old code used to skip the fallback parent window check when the dialog was opened after login. My CL changed both the pre-login and post-login to do the fallback check, which is why this happens.

Thanks for the analysis and pointing me at my own CL. :-)

Ugh, sorry, it's mostly related to the dialog positioning when it is spawned from the webui network settings page. In that case it should parent to the settings window. In the common case it should be centered on the screen. The login stuff is related to the fallback parent, and I suspect we don't need GetParentForUnhostedDialog() at all.

Anyhow, I'll fix it.

Trying to center the dialog on the Settings window when it is visible seems problematic. I would just center the dialog on the display that it was opened from.

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

commit 597a27482d43f2ac88c1c35f94533889845b80ec
Author: James Cook <jamescook@chromium.org>
Date: Thu Jul 13 22:51:57 2017

cros: Fix position of network connect dialog

Always place it in the center of the "root window for new windows", which
is the display where the user last interacted with a window.

This fixes a regression where the dialog could be centered over a tabbed
browser window. It also fixes a couple cases where adding a new Wi-Fi
network from the status tray menu on a secondary monitor would open the
dialog on the primary monitor.

Added some basic test coverage to NetworkConfigView.

Bug:  740316 
Test: chrome browser_tests. Also manually add a new Wi-Fi network during OOBE, at the login screen, and after login on both primary and secondary displays.
Change-Id: I60f2070dea9f39da586c97ff8b178fc34c5fb325
Reviewed-on: https://chromium-review.googlesource.com/569025
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486507}
[modify] https://crrev.com/597a27482d43f2ac88c1c35f94533889845b80ec/chrome/browser/chromeos/options/network_config_view.cc
[modify] https://crrev.com/597a27482d43f2ac88c1c35f94533889845b80ec/chrome/browser/chromeos/options/network_config_view.h
[add] https://crrev.com/597a27482d43f2ac88c1c35f94533889845b80ec/chrome/browser/chromeos/options/network_config_view_browsertest.cc
[modify] https://crrev.com/597a27482d43f2ac88c1c35f94533889845b80ec/chrome/browser/chromeos/status/network_menu.cc
[modify] https://crrev.com/597a27482d43f2ac88c1c35f94533889845b80ec/chrome/browser/chromeos/ui/choose_mobile_network_dialog.h
[modify] https://crrev.com/597a27482d43f2ac88c1c35f94533889845b80ec/chrome/browser/ui/ash/system_tray_client.cc
[modify] https://crrev.com/597a27482d43f2ac88c1c35f94533889845b80ec/chrome/browser/ui/webui/chromeos/login/network_dropdown_handler.cc
[modify] https://crrev.com/597a27482d43f2ac88c1c35f94533889845b80ec/chrome/browser/ui/webui/chromeos/network_ui.cc
[modify] https://crrev.com/597a27482d43f2ac88c1c35f94533889845b80ec/chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
[modify] https://crrev.com/597a27482d43f2ac88c1c35f94533889845b80ec/chrome/browser/ui/webui/settings/chromeos/internet_handler.cc
[modify] https://crrev.com/597a27482d43f2ac88c1c35f94533889845b80ec/chrome/test/BUILD.gn