Closed Bug 1035540 Opened 7 years ago Closed 7 years ago

Convert Security pane dialogs to be in-content

Categories

(Firefox :: Preferences, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 33

People

(Reporter: MattN, Assigned: Paenglab)

References

()

Details

Attachments

(1 file, 3 obsolete files)

Convert security.js to use gSubDialog.
security.js is already in bug 752197 included. What should be done here?
(In reply to Richard Marti (:Paenglab) from comment #1)
> security.js is already in bug 752197 included. What should be done here?

For review purposes the patch in bug 752197 should apparently be split by preferences pane.
Attached patch securityDialogsIncontent.patch (obsolete) — Splinter Review
(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #63)
> Comment on attachment 8448050 [details] [diff] [review]
> inContentDialogs.patch
> 
> Review of attachment 8448050 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/preferences/in-content/advanced.js
> @@ +274,5 @@
> >     */
> >    showConnections: function ()
> >    {
> > +    gSubDialog.open("chrome://browser/content/preferences/connection.xul",
> > +                    "modal=yes", null);
> 
> Omit the last argument when it's null since it's the default value.

I've also removed the "modal=yes" as the dialogs are always modal and don't let change something in the prefs behind them. I hope it's okay like this.
Attachment #8452454 - Flags: review?(MattN+bmo)
Comment on attachment 8452454 [details] [diff] [review]
securityDialogsIncontent.patch

Review of attachment 8452454 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks

::: browser/components/preferences/in-content/security.js
@@ +55,5 @@
>        params.introText = bundlePrefs.getString("addonspermissionstext");
>      }
>  
> +    gSubDialog.open("chrome://browser/content/preferences/permissions.xul",
> +                    params);

I think |params| should be the 3rd argument

@@ +106,5 @@
>     * where passwords are never saved.
>     */
>    showPasswordExceptions: function ()
>    {
> +    gSubDialog.open("chrome://passwordmgr/content/passwordManagerExceptions.xul");

This dialog looks bad (not enough padding, title not aligned with the description, and overly wide) but I suppose that can be fixed in a follow-up or we can leave this sub-dialog out of this patch and get it right in another bug.

@@ +199,5 @@
>     */
>    changeMasterPassword: function ()
>    {
> +    gSubDialog.open("chrome://mozapps/content/preferences/changemp.xul",
> +                    this._initMasterPasswordUI.bind(this));

|this._initMasterPasswordUI.bind(this)| should be the 4th argument

::: browser/themes/shared/incontentprefs/preferences.css
@@ +854,5 @@
> +  padding-right: 6px;
> +  padding-left: 6px;
> +}
> +
> +.title {

Shouldn't this selector be more specific to subdialogs?

@@ +879,5 @@
> +  height: 20em;
> +  width: 66ch;
> +}
> +
> +resizer {

I think this should just target resizers of subdialogs. Or are there others you want to hide?
Attachment #8452454 - Flags: review?(MattN+bmo) → review-
Attached patch securityDialogsIncontent.patch (obsolete) — Splinter Review
(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #4)
> Comment on attachment 8452454 [details] [diff] [review]
> securityDialogsIncontent.patch
> 
> Review of attachment 8452454 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks
> 
> ::: browser/components/preferences/in-content/security.js
> @@ +55,5 @@
> >        params.introText = bundlePrefs.getString("addonspermissionstext");
> >      }
> >  
> > +    gSubDialog.open("chrome://browser/content/preferences/permissions.xul",
> > +                    params);
> 
> I think |params| should be the 3rd argument

I had to add 'null,' only a comma didn't work.

> @@ +106,5 @@
> >     * where passwords are never saved.
> >     */
> >    showPasswordExceptions: function ()
> >    {
> > +    gSubDialog.open("chrome://passwordmgr/content/passwordManagerExceptions.xul");
> 
> This dialog looks bad (not enough padding, title not aligned with the
> description, and overly wide) but I suppose that can be fixed in a follow-up
> or we can leave this sub-dialog out of this patch and get it right in
> another bug.

I forgot to add the preferences.xul change which makes it better. Now included.

> ::: browser/themes/shared/incontentprefs/preferences.css
> @@ +854,5 @@
> > +  padding-right: 6px;
> > +  padding-left: 6px;
> > +}
> > +
> > +.title {
> 
> Shouldn't this selector be more specific to subdialogs?

Now #dialogTitle {}

> @@ +879,5 @@
> > +  height: 20em;
> > +  width: 66ch;
> > +}
> > +
> > +resizer {
> 
> I think this should just target resizers of subdialogs. Or are there others
> you want to hide?

Now

dialog resizer,
window resizer,
prefwindow resizer {}

With bug 1035625 this needs to be removed.
Attachment #8452454 - Attachment is obsolete: true
Attachment #8452588 - Flags: review?(MattN+bmo)
Comment on attachment 8452588 [details] [diff] [review]
securityDialogsIncontent.patch

Review of attachment 8452588 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks. r=me with the two/three fixes.

(In reply to Richard Marti (:Paenglab) from comment #5)
> (In reply to Matthew N. [:MattN] (behind on bugmail) from comment #4)
> > @@ +106,5 @@
> > >     * where passwords are never saved.
> > >     */
> > >    showPasswordExceptions: function ()
> > >    {
> > > +    gSubDialog.open("chrome://passwordmgr/content/passwordManagerExceptions.xul");
> > 
> > This dialog looks bad (not enough padding, title not aligned with the
> > description, and overly wide) but I suppose that can be fixed in a follow-up
> > or we can leave this sub-dialog out of this patch and get it right in
> > another bug.
> 
> I forgot to add the preferences.xul change which makes it better. Now
> included.

The exceptions dialog doesn't look much better TBH. All three problems still apply.

::: browser/themes/shared/incontentprefs/preferences.css
@@ +855,5 @@
> +  padding-left: 6px;
> +}
> +
> +#dialogTitle {
> +  -moz-margin-start: 5px !important;

I'm not sure this is ideal but I guess it's okay for now.

@@ +865,5 @@
>    box-shadow: none;
>    height: 18px;
>    padding: 0;
>    min-width: 18px;
> +  -moz-margin-end: 0;

On OS X this makes it look too close to the right compared to the top offset. Can we move this to a separate bug? (Ideally one that includes more Chameleon styling as it may change again).

@@ +880,5 @@
> +  width: 66ch;
> +}
> +
> +dialog resizer,
> +window resizer,

Add the bug 1035625 in a comment here to indicate it should be adjusted/removed then.

@@ +886,5 @@
> +  display: none;
> +}
> +
> +tree:not(#rejectsTree) {
> +  min-height: 15em;

I'm not a fan of this selector because it includes details of a specific subdialog but I'm not really sure of a better suggestion at the moment.
Attachment #8452588 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8452588 [details] [diff] [review]
securityDialogsIncontent.patch

Review of attachment 8452588 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/shared/incontentprefs/preferences.css
@@ +881,5 @@
> +}
> +
> +dialog resizer,
> +window resizer,
> +prefwindow resizer {

Oh, and you could use -moz-any here since the -moz-any isn't right-most:

-moz-any(dialog, window, prefwindow) resizer {
Attached patch Patch for check-in (obsolete) — Splinter Review
Addressed the comments.
Attachment #8452588 - Attachment is obsolete: true
Attachment #8452618 - Flags: review+
Bugger, a : was missing.
Attachment #8452618 - Attachment is obsolete: true
Attachment #8452638 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ab15179de7d9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Depends on: 1037049
Mentor: MattN+bmo
Duplicate of this bug: 1014059
Verified on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.8.5 using latest Nightly 34.0a1 (buildID: 	20140728030201)and the dialogs from Security pane are in-content. 

Only one mention here: the "Exceptions - Saved Passwords" dialog doesn't look good: title not aligned with the description, not enough padding (those problems were also mentioned in comment #6). Should I file a separately bug for those issues?
Flags: needinfo?(richard.marti)
I think it's not needed and will be done in bug 1036090. I also mentioned it in bug 1036090 comment 4.
Flags: needinfo?(richard.marti)
Thank you for the answer!
Status: RESOLVED → VERIFIED
Depends on: 1055873
You need to log in before you can comment on or make changes to this bug.