Closed Bug 1035625 Opened 5 years ago Closed 5 years ago

Support "resizable" in-content preference subdialogs

Categories

(Firefox :: Preferences, defect)

defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 34
Iteration:
34.1

People

(Reporter: MattN, Assigned: jaws)

References

(Depends on 3 open bugs)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
Duplicate of this bug: 1037049
Flags: firefox-backlog+
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
I had to move the width and height to min-width and min-height or else the dialogs could be resized too small.
Attachment #8455838 - Flags: review?(MattN+bmo)
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.
Attachment #8455838 - Flags: review?(MattN+bmo) → feedback+
Attached patch Patch v2Splinter Review
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.
Attachment #8455838 - Attachment is obsolete: true
Attachment #8455858 - Flags: review?(MattN+bmo)
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
Attachment #8455858 - Flags: review?(MattN+bmo) → review+
Whiteboard: [fixed-in-fx-team]
(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.
Hi Jared, can you provide a point value and if the bug should be marked as [qa+] or [qa-]
Iteration: --- → 34.1
QA Whiteboard: [qa?]
Flags: needinfo?(jaws)
Points: --- → 3
QA Whiteboard: [qa?] → [qa+]
Flags: needinfo?(jaws)
https://hg.mozilla.org/mozilla-central/rev/43aa05ddc80a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
Hi Florin, can a QA contact be assigned for verification of this bug.
Flags: needinfo?(florin.mezei)
QA Contact: camelia.badau
Flags: needinfo?(florin.mezei)
Depends on: 1043346
> (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.
Depends on: 1043509
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?
Flags: needinfo?(jaws)
I forgot to mention that I used latest Nightly 34.0a1 (buildID: 20140728030201).
(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?
Flags: needinfo?(jaws) → needinfo?(MattN+bmo)
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.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
(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?
Flags: needinfo?(MattN+bmo)
Flags: needinfo?(camelia.badau)
I've already filed Bug 1043346
Thanks!
Flags: needinfo?(camelia.badau)
Depends on: 1056478
Depends on: 1322955
You need to log in before you can comment on or make changes to this bug.