Closed Bug 1037049 Opened 6 years ago Closed 6 years ago

In-content preference subdialogs that contain a <resizer> need to have their <resizer> removed and passed in as a "resizable" window opening parameter

Categories

(Firefox :: Preferences, defect)

33 Branch
x86_64
Windows 7
defect
Not set
normal
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 34
Iteration:
34.3
Tracking Status
firefox33 - affected
firefox34 --- verified

People

(Reporter: alice0775, Assigned: jaws)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1035625
bug 1035625 does not fix the following subdialog


1. Allowed Sites - Add-ons Installation
2. Exceptions - Saved Passwords
3. Saved Passwords
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Summary: In-content preferences dialog should be re-sizable → In-content preference subdialogs that contain a <resizer> need to have their <resizer> removed and passed in as a "resizable" window opening parameter
Duplicate of this bug: 1028174
Assignee: nobody → jaws
Status: REOPENED → ASSIGNED
Iteration: --- → 34.1
Points: --- → 3
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8464213 - Flags: review?(MattN+bmo)
Marco, can you please add this to the 34.1 iteration?
Flags: needinfo?(mmucci)
Added to Iteration 34.1
Flags: needinfo?(mmucci)
Flags: firefox-backlog+
Iteration: 34.1 → 34.2
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.
Attachment #8464213 - Flags: review?(MattN+bmo) → review?(gijskruitbosch+bugs)
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. :-)
Attachment #8464213 - Flags: review?(gijskruitbosch+bugs)
What's the status here Jared?
Flags: needinfo?(jaws)
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.
Flags: needinfo?(jaws)
Iteration: 34.2 → 34.3
QA Whiteboard: [qa+]
Flags: qe-verify+
Attached patch Patch v2Splinter Review
Pushed to tryserver, waiting on try results before requesting review.

https://tbpl.mozilla.org/?tree=Try&rev=89643082ce7a
Attachment #8464213 - Attachment is obsolete: true
Attachment #8475550 - Flags: review?(gijskruitbosch+bugs)
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. :-)
Attachment #8475550 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/integration/fx-team/rev/10fb30af6386
Flags: in-testsuite+
Whiteboard: [fixed in fx-team]
https://hg.mozilla.org/mozilla-central/rev/10fb30af6386
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in fx-team]
Target Milestone: --- → Firefox 34
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.