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.

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

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

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

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

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

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

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

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.

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

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

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

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.

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.

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

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

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

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

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 ?

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.

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

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

Comment on attachment 8591116 [details] [diff] [review] Patch Should be in 38 beta 4.

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?

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

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