1035625 - Support "resizable" in-content preference subdialogs

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Matthew N. [:MattN]]

The "resizable" option of the features argument to gSubDialog.open should be respected and allow resizing of subdialogs. This is useful for the password manager dialog where additional columns can be added.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

I had to move the width and height to min-width and min-height or else the dialogs could be resized too small.

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8455838 [details] [diff] [review]
Patch

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

Can you remove https://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/incontentprefs/preferences.css?rev=c274ab1b4086#887 now?

::: browser/themes/shared/incontentprefs/preferences.css
@@ +848,5 @@
>  
>  #dialogBox {
>    border: 1px solid #666;
>    display: -moz-box;
> +  resize: both;

You should only make the dialog resizable when the resizable feature is specified for the subdialog. You probably need to set an attribute somewhere on the dialog when that's the case and have the CSS apply only then.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

This patch checks for resizable=yes in the features. I noticed that the Content > Exceptions dialog has two resizers but that looks to be due to the Exceptions dialog having its own <resizer> element that needs to be removed.

I'd rather that get taken care of in a separate bug since more investigation may be necessary and this patch is a general fix.

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8455858 [details] [diff] [review]
Patch v2

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

r=me with the regex change to handle "1" along with no "=".

We'll need a new bug for the resizer visibility then.

::: browser/components/preferences/in-content/subdialogs.js
@@ +65,5 @@
>      let dialog = window.openDialog(aURL, "dialogFrame", features, aParams);
>      if (aClosingCallback) {
>        this._closingCallback = aClosingCallback.bind(dialog);
>      }
> +    this._box.setAttribute("resizable", /resizable=yes/i.test(features));

resisable without the "=" is also truthy (it seems[1]) so I think you need something like (untested):
/resizable(=yes|=1|,|$)/i

[1] https://developer.mozilla.org/en-US/docs/Web/API/Window.open#Toolbar_and_chrome_features

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to [Away for most of July] (please needinfo? me) Jared Wein [:jaws] from comment #4)
> I noticed that the
> Content > Exceptions dialog has two resizers but that looks to be due to the
> Exceptions dialog having its own <resizer> element that needs to be removed.
> 
> I'd rather that get taken care of in a separate bug since more investigation
> may be necessary and this patch is a general fix.

(In reply to Matthew N. [:MattN] from comment #5)
> We'll need a new bug for the resizer visibility then.

The resizer visibility has been taken care of between the time that I made the changeset that my patch was previously based on and current hg tip.

Comment 8

[Marco Mucci [:MarcoM]]

Hi Jared, can you provide a point value and if the bug should be marked as [qa+] or [qa-]

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/43aa05ddc80a

Comment 10

[Marco Mucci [:MarcoM]]

Hi Florin, can a QA contact be assigned for verification of this bug.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

> (In reply to Matthew N. [:MattN] from comment #5)
> > We'll need a new bug for the resizer visibility then.
> 
> The resizer visibility has been taken care of between the time that I made
> the changeset that my patch was previously based on and current hg tip.

My above comment was erroneous. Bug 1037049 will handle moving that <resizer> out of the dialog and to a window opening parameter.

Comment 12

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

Verified on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.8.5 and the following mentions should be done: 
- only the "Exceptions" subdialog from Content tab is resizable (because only this has a resizer)
- if I resize the "Exceptions" subdialog (from Content tab), all other subdialogs will have the new size of the "Expections" subdialog

It is ok? Is this the behaviour intended?

Comment 13

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

I forgot to mention that I used latest Nightly 34.0a1 (buildID: 20140728030201).

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Camelia Badau, QA [:cbadau] from comment #12)
> Verified on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.8.5 and the
> following mentions should be done: 
> - only the "Exceptions" subdialog from Content tab is resizable (because
> only this has a resizer)

This is bug 1037049.

> - if I resize the "Exceptions" subdialog (from Content tab), all other
> subdialogs will have the new size of the "Expections" subdialog

I'm pretty sure this is bug 1043612. MattN, can you confirm?

> It is ok? Is this the behaviour intended?

Comment 15

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Seems like there's no more testing work to do here, but wait for Matt's confirmation on the second issue. Marking as Verified since remaining issues are tracked separately.

Comment 16

[Matthew N. [:MattN]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #14)
> (In reply to Camelia Badau, QA [:cbadau] from comment #12)
> > - if I resize the "Exceptions" subdialog (from Content tab), all other
> > subdialogs will have the new size of the "Expections" subdialog
> 
> I'm pretty sure this is bug 1043612. MattN, can you confirm?

The size of one subdialog should have no impact on subsequent ones shown as we try to clear attributes set on the frame upon closing of a dialog. Bug 1043612 is supposed to make it so we remember the sizes for certain dialogs. Can you please file a new bug blocking bug 1014201 if you are saying one dialog size is affecting others?

Comment 17

[Alice0775 White]

I've already filed Bug 1043346

Comment 18

[Matthew N. [:MattN]]

Thanks!