1901549 - chrome: URLs shouldn't be allowed to open popup windows with the location feature disabled

Status: NEW

(Core :: DOM: Core & HTML, task)

Description

[Dão Gottwald [:dao]]

From https://phabricator.services.mozilla.com/D212724#7314771:

! In D212724#7314771, @Gijs wrote:

! In D212724#7314099, @dao wrote:

! In D212724#7313928, @Gijs wrote:

! In D212724#7306145, @dao wrote:
So it looks like hiding #nav-bar in popups (which is disabled by default but enabled for some tests?)

Can you elaborate on this? Is this a pref that we still support outside of tests, and if so do we need to? Or is this a kiosk mode thing?

Bug 1507375 removed support for the dom.disable_window_open_feature.* prefs, and restricted what web content can do here. I suspect what's happening is some tests (e.g. test_queryCaretRect.html, test_focus.xhtml, test_ime_state_others_in_parent.html) aren't considered web content,

Yes, they are in chrome.toml so they are chrome mochitests and get loaded over chrome: URLs. You can sort of see this in the failing job screenshots at the bottom where it lists URLs. The terrifying thing here is that the window.open path is shared between chrome and content code but the behaviour is... pretty different, generally speaking. CC @nika as we talked about this recently.

and therefore are allowed to open popups with fewer features, although I don't see them explicitly disabling the location feature, so that still seems weird. Here's a screenshot from test_queryCaretRect.html when it failed, with the location bar hidden but the empty toolbar still visible: https://d2mfgivbiy2fiw.cloudfront.net/file/data/roiebqmkd77riysvledv/PHID-FILE-4xpkoobr3jaojgxlyznd/mozilla-test-fail-screenshot_3k2teufo.png

Yeah... so I think if we don't support this for web content we should fix the tests to test something more meaningful (and/or not rely on the chrome bits to open "weird" windows). I'm not sure what motivated this patch; inasmuch as there's some other reason we want to do this cleanup, I'm fine with it landing and then dealing with the test bits and removing support for the "weird" windows as a follow-up -- or perhaps it's appropriate to do it here. I'd expect @arai
to be in a position to comment on what the deal is with the tests.

Comment 1

[Tooru Fujisawa [:arai]]

About the current behavior,
if the features string is not empty, it first defaults to minimal styling.
This was also true for web content's case until bug 1507375 patch, except for that chrome-priv makes it more minimal:

https://searchfox.org/mozilla-central/rev/4c8627a76e2e0a9b49c2b673424da478e08715ad/toolkit/components/windowwatcher/nsWindowWatcher.cpp#1885-1893

if (aFeatures.IsEmpty()) {
  chromeFlags = nsIWebBrowserChrome::CHROME_ALL;
  if (aDialog) {
    chromeFlags |= nsIWebBrowserChrome::CHROME_OPENAS_DIALOG |
                   nsIWebBrowserChrome::CHROME_OPENAS_CHROME;
  }
} else {
  chromeFlags = nsIWebBrowserChrome::CHROME_WINDOW_BORDERS;
}

Then it checks each feature, where most of them defaults to false:

https://searchfox.org/mozilla-central/rev/4c8627a76e2e0a9b49c2b673424da478e08715ad/toolkit/components/windowwatcher/nsWindowWatcher.cpp#1909-1938

if (aFeatures.GetBoolWithDefault("location", false, &presenceFlag)) {
  chromeFlags |= nsIWebBrowserChrome::CHROME_LOCATIONBAR;
}

So, if window.open is called in chrome-priv context, for example with "width=800,height=800" features (that's what test_queryCaretRect.html uses), it results in a very minimal UI which doesn't have location bar.

Then, the 1st question would be what kind of combination is actually used in non-test chrome-priv code, and in what case they're.
If features string isn't actually used, we could just drop the features to control visibility.
If there are small set of combinations, we could reduce the support, for example similar to web content, where the features is just a condition to choose from supported styling.

Then, about the test chrome-priv usage, the question is indeed what the expectation for each testcase is.
If it's intended just to open a new window, then those tests should be either:

• rewritten with BrowserTestUtils.openNewBrowserWindow etc, instead of relying on chrome-priv window.open behavior
• rewritten with mochitest-plain where window.open is performed with content-priv, while all chrome-priv thing replaced with SpecialPowers etc