After landing Bug 1035540, it is no longer resizable [saved Password] dialog. When turned on all tree columns, content text are truncated. And also, I think that all in-content preferences should be resizable.

> And also, I think that all in-content preferences should be resizable. And also, I think that all in-content preferences dialog should be resizable.

bug 1035625 does not fix the following subdialog 1. Allowed Sites - Add-ons Installation 2. Exceptions - Saved Passwords 3. Saved Passwords

dup of bug 1028174?

I changed subdialog.js' open function to use strict equality due to a subtle bug where if the "resizable" feature was specified with no value, then it would result in a resizable="false" attribute since "" == 0. I also made the same change for the `!= "no"` check for consistency. I went through the preferences and confirmed that there are no other dialogs missing the resizable behavior. Note that two dialogs that are spawned for Advanced > Certificates are using openDialog whereas the windowed-preferences are using openWindow. I added the "resizable" feature to the openDialog to be consistent with the previous behavior.

Marco, can you please add this to the 34.1 iteration?

Added to Iteration 34.1

Comment on attachment 8464213 [details] [diff] [review] Patch Why are you changing many files not related to in-content preferences?

(In reply to Matthew N. [:MattN] from comment #9) > Comment on attachment 8464213 [details] [diff] [review] > Patch > > Why are you changing many files not related to in-content preferences? Because I removed the <resizer> from the dialogs which are shared between in-content and regular preferences. This way if we disable in-content preferences the other dialogs will continue to work.

Comment on attachment 8464213 [details] [diff] [review] Patch Review of attachment 8464213 [details] [diff] [review]: ----------------------------------------------------------------- Generally, have you checked that your change to the features parameter (from "" to "resizable") doesn't defeat the same kind of logic that we have in subdialogs.js ? That is: features = aFeatures || "foo" means that the default features will be "foo", even if you pass "". Passing "resizable" might therefore mean that dialogs no longer get features that they would previously have had. In particular, in the subdialogs code, this will now mean that dialogs no longer get the "modal" and "centerscreen" features (which might not matter much for the in-content "subdialogs"; I'm not familiar enough with them to know). Codewise this generally looks solid to me, but I'd like to actually run it and do some more checks before granting r+ - maybe in the meantime you have time to address these questions? :-) ::: browser/base/content/pageinfo/security.js @@ +142,5 @@ > win.focus(); > } > else > window.openDialog("chrome://passwordmgr/content/passwordManager.xul", > + "Toolkit:PasswordManager", "resizable", Nit: might as well fix the whitespace while we're here. ::: browser/components/preferences/in-content/subdialogs.js @@ +63,2 @@ > open: function(aURL, aFeatures = null, aParams = null, aClosingCallback = null) { > let features = aFeatures || "modal,centerscreen,resizable=no"; Should we really default to resizable=no? @@ +67,5 @@ > this._closingCallback = aClosingCallback.bind(dialog); > } > let featureParams = new URLSearchParams(features.toLowerCase()); > this._box.setAttribute("resizable", featureParams.has("resizable") && > + featureParams.get("resizable") !== "no" && Is this change actually necessary?

From the documentation on MDN: > If the features parameter is a zero-length string, or contains only one or more of the behaviour features > (chrome, dependent, dialog and modal) the chrome features are assumed "OS' choice." This is now no longer the case. That seems worrisome.

Comment on attachment 8464213 [details] [diff] [review] Patch Jared and I had a chat about this patch. Notes: - gSubDialogs is standing in for both window.openDialog (which always gets you resizable windows) and document.documentElement.openSubDialog (which does *not* get you resizable windows). So it's either mess with the arguments, or add a different method to gSubDialogs and mess with the callers... but it's no fun in terms of patch size either way. I think I would prefer the method that would touch the shared dialogs and/or in-separate-window prefs the least. - URLSearchParams only works when you pass it &-separated params. Which we're not. Need to fix that for this code to work. Also, don't pass a "?" because that becomes part of the first param name and therefore messes up all the things, because this is apparently how the API has been designed, which tops today's list of silly API design decisions. - URLSearchParams will otherwise give you the first matching feature, and the openSubDialog code here: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/preferences.xml#1015 appends consumer-provided features *after* the defaults it wants. Checking with window.openDialog("chrome://browser/content/aboutDialog.xul", "_blank", "height=300,height=500", null); and the heights swapped, it seems window.open takes the first provided value, just like URLSearchParams. In other words, if we want the defaults to not override stuff callers provide, we should append the defaults (unlike openSubDialog), and then call URLSearchParams. Clearing review for now. :-)

What's the status here Jared?

I need to work on a new patch that will address Gijs' feedback. It may not be too far from what is here right now, but I will need to make the changes and double-check that each calling site still has the correct resulting parameters. I haven't gotten to this bug as it has been the lowest on my priority list.

Pushed to tryserver, waiting on try results before requesting review. https://tbpl.mozilla.org/?tree=Try&rev=89643082ce7a

Comment on attachment 8475550 [details] [diff] [review] Patch v2 Review of attachment 8475550 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/preferences/in-content/subdialogs.js @@ +67,5 @@ > if (aClosingCallback) { > this._closingCallback = aClosingCallback.bind(dialog); > } > + let featureParams = features.replace(",", "&"); > + featureParams = new URLSearchParams(featureParams.toLowerCase()); uber-nit so I don't just give you a silent r+ ( ;-) ): if you replace the first of these lines with features = features.replace(",", "&");, you don't need to touch the second one. :-)

Verified fixed on Windows 7 64bit, Windows 7 32bit, Ubuntu 13.10 32bit and Mac OSX 10.9.4 using latest Nightly 34.0a1 (buidlID: 20140821030201) and all the in-content preferences subdialogs are resizable.