Matthew N. [:MattN] Reporter
	Description • 10 years ago

e.g. if I resize the saved password dialog because I toggle the two optional columns on, I don't want to have to resize the sub-diialog to make it larger every time.

We can hopefully still use XUL persistence for this so we don't have to re-implement storage.

	Matthew N. [:MattN] Reporter
	Comment 1 • 10 years ago

In case it wasn't clear, different dialogs should have different persisted sizes like how XUL persistence uses the ID of the documentElement.

	Tim Nguyen :ntim
	Comment 2 • 10 years ago

(In reply to Matthew N. [:MattN] from comment #1)
> In case it wasn't clear, different dialogs should have different persisted
> sizes like how XUL persistence uses the ID of the documentElement.

The problem is we have one id for all dialog boxes, which is #dialogBox. We can't just persist the same size for all dialog boxes.

	Matthew N. [:MattN] Reporter
	Comment 4 • 10 years ago

(In reply to Tim Nguyen [:ntim] from comment #2)
> (In reply to Matthew N. [:MattN] from comment #1)
> > In case it wasn't clear, different dialogs should have different persisted
> > sizes like how XUL persistence uses the ID of the documentElement.
> 
> The problem is we have one id for all dialog boxes, which is #dialogBox. We
> can't just persist the same size for all dialog boxes.

That's not true. See https://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/content/passwordManager.xul for example. The ID is SignonViewerDialog.

	Tim Nguyen :ntim
	Comment 5 • 10 years ago

(In reply to Matthew N. [:MattN] from comment #4)
> (In reply to Tim Nguyen [:ntim] from comment #2)
> > (In reply to Matthew N. [:MattN] from comment #1)
> > > In case it wasn't clear, different dialogs should have different persisted
> > > sizes like how XUL persistence uses the ID of the documentElement.
> > 
> > The problem is we have one id for all dialog boxes, which is #dialogBox. We
> > can't just persist the same size for all dialog boxes.
> 
> That's not true. See
> https://mxr.mozilla.org/mozilla-central/source/toolkit/components/
> passwordmgr/content/passwordManager.xul for example. The ID is
> SignonViewerDialog.

This is the frame inside it,not the actual element being resized.

	Matthew N. [:MattN] Reporter
	Comment 6 • 10 years ago

Right, we can continue to persist the size of that element (the documentElement)

	:Gijs (he/him)
	Comment 7 • 9 years ago

But there are already dialogs that have persisted sizes (e.g. http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/content/passwordManagerExceptions.xul ) that don't work in-content, so I don't think persisting things on the document element of the dialog is enough...

	Tim Nguyen :ntim
	Comment 8 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #7)
> But there are already dialogs that have persisted sizes (e.g.
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/
> content/passwordManagerExceptions.xul ) that don't work in-content, so I
> don't think persisting things on the document element of the dialog is
> enough...

Once the dialog frame loads, it could read the width and height attribute inside the frame.

	:Gijs (he/him)
	Comment 9 • 9 years ago

Matt, are you able to take this yourself? It's been mentored for a while with no takers, and we'd like to have this in for shipping these.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 10 • 9 years ago

To do:
1, treeviews and similar style elements are not shrinking when the subdialog shrinks
2, the persisted size needs to have the subdialog padding, caption, and borders removed

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 11 • 9 years ago

We need frame.style.width and frame.style.height to be set when the dialog is opening so that the subdialog's size will be pulled in from the persisted attributes, but we can't keep those values around or it causes flex items to not shrink when the dialog shrinks. I worked around this by removing the style properties if the dialog gets resized.

Also, if you test out this patch, it will (maybe obviously) only work on dialogs that are set to persist their width and height. You can test it with Content > Pop-ups > Exceptions... and Security > Saved Passwords...

	:Gijs (he/him)
	Comment 12 • 9 years ago

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

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

Close, but I got questions... r- for now, I think the next will be r+. Sorry!

::: browser/components/preferences/in-content/subdialogs.js
@@ +184,5 @@
>      let boxHorizontalPadding = 2 * parseFloat(getComputedStyle(groupBoxBody).paddingLeft);
> +    let frameMinWidth = docEl.style.width || docEl.scrollWidth + "px";
> +    let frameWidth = docEl.getAttribute("width") ? docEl.getAttribute("width") + "px" :
> +                     frameMinWidth;
> +    let frameMinHeight = docEl.style.height || docEl.scrollHeight + "px";

Seeing as we're here... I don't suppose you've spotted any problems with removing docEl.style.height || and always using scrollHeight/scrollWidth here?

@@ +233,5 @@
> +    let frame = gSubDialog._frame;
> +    // The width and height styles are needed for the initial
> +    // layout of the frame, but afterward they need to be removed
> +    // or their presence will restrict the contents of the <browser>
> +    // from resizing to a smaller size.

Don't we need to do this for non-persisted dialogs as well?

@@ +240,5 @@
> +
> +    let docEl = frame.contentDocument.documentElement;
> +    for (let mutation of mutations) {
> +      if (mutation.attributeName == "width") {
> +        docEl.setAttribute(mutation.attributeName, docEl.scrollWidth);

could just hardcode the name of the thing you're setting here.

@@ +242,5 @@
> +    for (let mutation of mutations) {
> +      if (mutation.attributeName == "width") {
> +        docEl.setAttribute(mutation.attributeName, docEl.scrollWidth);
> +      } else if (mutation.attributeName == "height") {
> +        docEl.setAttribute(mutation.attributeName, docEl.scrollHeight);

ditto

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 13 • 9 years ago

Through manual testing I confirmed that we don't need to use docEl.style.width/docEl.style.height. scrollWidth/scrollHeight achieve what we are looking for there anyways.

I've made the other changes you requested and retested.

	:Gijs (he/him)
	Comment 15 • 9 years ago

Comment on attachment 8589843 [details] [diff] [review]
Patch v1.1 (qref'd)

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

I expect this may also fix bug 1141031. We should check after this lands.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 16 • 9 years ago

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

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 17 • 9 years ago

Follow-up to fix browser_subdialogs.js,
https://hg.mozilla.org/integration/fx-team/rev/f0f90206af12

rs=Gijs over IRC

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 18 • 9 years ago

And the second-half of the follow-up,
https://hg.mozilla.org/integration/fx-team/rev/1831a8cf4116

rs=Gijs over IRC

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 19 • 9 years ago

Approval Request Comment
[Feature/regressing bug #]: new feature of in-content prefs
[User impact if declined]: some dialogs such as Saved Passwords will not remember their adjusted size
[Describe test coverage new/current, TreeHerder]: manual testing
[Risks and why]: none expected
[String/UUID change made/needed]: none

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 20 • 9 years ago

Original patch and the two followups backed out because bc1 is still orange on at least OSX:
remote:   https://hg.mozilla.org/integration/fx-team/rev/53f0959b3713
remote:   https://hg.mozilla.org/integration/fx-team/rev/81f9b3088233
remote:   https://hg.mozilla.org/integration/fx-team/rev/a1e6f8f259dc

https://treeherder.mozilla.org/logviewer.html#?job_id=2627548&repo=fx-team

	:Gijs (he/him)
	Comment 21 • 9 years ago

I feel bad that this has now bounced because of a suggested fix for bug 1141031 that I encouraged you to include here. If you want to (trypush and) reland without that change, that wfm, assuming it isn't actually necessary for this bug (which I think it is not) and I we figure out the issues + oranges in bug 1141031 ?

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 22 • 9 years ago

Yeah, that sounds like a good change to make. I was gonna start investigating the failure but it's a rat-hole unrelated to this bug really.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 23 • 9 years ago

Relanded without the style.height/style.width changes,
https://hg.mozilla.org/integration/fx-team/rev/793c662d0c89

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 24 • 9 years ago

Approval Request Comment
[Feature/regressing bug #]: new feature of in-content prefs
[User impact if declined]: some dialogs such as Saved Passwords will not remember their adjusted size
[Describe test coverage new/current, TreeHerder]: manual testing
[Risks and why]: none expected
[String/UUID change made/needed]: none

	Carsten Book [:Tomcat]
	Comment 25 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/793c662d0c89

	Sylvestre Ledru [:Sylvestre]
	Comment 26 • 9 years ago

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

Should be in 38 beta 4.

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 9 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/86d473ce84f5

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 9 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/9740c1d817f1

	Camelia Badau [:cbadau], Desktop QA
	Comment 29 • 9 years ago

Verified on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5 using latest Nightly 40.0a1 (buildID: 20150414130911), latest Aurora 39.0a2 (buildID: 20150414004003) and Firefox 38 Beta 4 (buildID: 20150413143743): 

- the following dialogs remember their adjusted size: General -> "Set Home Page" dialog; Content -> Pop-ups -> Exceptions ; Privacy -> History (Use custom settings for history) -> Exceptions and Cookies dialogs; Security -> General -> Exceptions (Allowed Sites - Add-ons Installation); Security -> Passwords -> "Exceptions-Saved Passwords" and "Saved Passwords" dialogs

- one mention should be done here: if I change the size of the Exceptions dialog from Content -> Pop-ups ("Allowed Sites - Pop-ups"),all the Exceptions dialogs take over this size (Exceptions from Privacy -> History and Exceptions from Security -> General) - it is expected?

	:Gijs (he/him)
	Comment 30 • 9 years ago

(In reply to Camelia Badau, QA [:cbadau] from comment #29)
> - one mention should be done here: if I change the size of the Exceptions
> dialog from Content -> Pop-ups ("Allowed Sites - Pop-ups"),all the
> Exceptions dialogs take over this size (Exceptions from Privacy -> History
> and Exceptions from Security -> General) - it is expected?

Yes. :-)

	Camelia Badau [:cbadau], Desktop QA
	Comment 31 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #30)
> (In reply to Camelia Badau, QA [:cbadau] from comment #29)
> > - one mention should be done here: if I change the size of the Exceptions
> > dialog from Content -> Pop-ups ("Allowed Sites - Pop-ups"),all the
> > Exceptions dialogs take over this size (Exceptions from Privacy -> History
> > and Exceptions from Security -> General) - it is expected?
> 
> Yes. :-)

Marking this bug as VERIFIED FIXED.