1035540 - Convert Security pane dialogs to be in-content

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Matthew N. [:MattN]]

Convert security.js to use gSubDialog.

Comment 1

[Richard Marti (:Paenglab)]

security.js is already in bug 752197 included. What should be done here?

Comment 2

[u428464]

(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.

Comment 3

[Richard Marti (:Paenglab)]

Attachment: securityDialogsIncontent.patch

(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.

Comment 4

[Matthew N. [:MattN]]

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?

Comment 5

[Richard Marti (:Paenglab)]

Attachment: securityDialogsIncontent.patch

(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.

Comment 6

[Matthew N. [:MattN]]

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.

Comment 7

[Matthew N. [:MattN]]

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 {

Comment 8

[Richard Marti (:Paenglab)]

Attachment: Patch for check-in

Addressed the comments.

Comment 9

[Richard Marti (:Paenglab)]

Attachment: Patch for check-in

Bugger, a : was missing.

Comment 10

[Richard Marti (:Paenglab)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=02a4571934f4

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/ab15179de7d9

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ab15179de7d9

Comment 14

[Camelia Badau [:cbadau], Desktop Test Engineering]

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?

Comment 15

[Richard Marti (:Paenglab)]

I think it's not needed and will be done in bug 1036090. I also mentioned it in bug 1036090 comment 4.

Comment 16

[Camelia Badau [:cbadau], Desktop Test Engineering]

Thank you for the answer!