Closed Bug 1043612 Opened 8 years ago Closed 7 years ago

Persist the size of resizable in-content subdialogs

Categories

(Firefox :: Preferences, defect, P2)

defect
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 40
Iteration:
40.1 - 13 Apr
Tracking Status
firefox38 --- verified
firefox39 --- verified
firefox40 --- verified

People

(Reporter: MattN, Assigned: jaws, Mentored)

References

Details

(Keywords: regression)

Attachments

(1 file, 5 obsolete files)

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.
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
Points: --- → 3
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)
Priority: -- → P2
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.
Flags: needinfo?(MattN+bmo)
Whiteboard: [sf-hackweek]
Whiteboard: [sf-hackweek]
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Flags: needinfo?(MattN+bmo)
Attached patch WIP patch (obsolete) — Splinter Review
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
Attached patch Patch (obsolete) — Splinter Review
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...
Attachment #8583191 - Attachment is obsolete: true
Attachment #8589334 - Flags: review?(gijskruitbosch+bugs)
Severity: enhancement → normal
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
Attachment #8589334 - Flags: review?(gijskruitbosch+bugs) → review-
Attached patch Patch v1.1 (obsolete) — Splinter Review
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.
Attachment #8589334 - Attachment is obsolete: true
Attachment #8589810 - Flags: review?(gijskruitbosch+bugs)
Attached patch Patch v1.1 (qref'd) (obsolete) — Splinter Review
Attachment #8589810 - Attachment is obsolete: true
Attachment #8589810 - Flags: review?(gijskruitbosch+bugs)
Attachment #8589843 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8589843 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/integration/fx-team/rev/21d7e9b0c0db
Iteration: --- → 40.1 - 13 Apr
Flags: qe-verify+
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
Attached patch Patch for Aurora 39 and Beta 38 (obsolete) — Splinter Review
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
Attachment #8590442 - Flags: approval-mozilla-aurora?
Attachment #8590442 - Flags: approval-mozilla-beta?
Attachment #8590442 - Flags: approval-mozilla-beta?
Attachment #8590442 - Flags: approval-mozilla-aurora?
Flags: needinfo?(wkocher)
Blocks: 1141031
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 ?
Flags: needinfo?(jaws)
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.
Flags: needinfo?(jaws)
Relanded without the style.height/style.width changes,
https://hg.mozilla.org/integration/fx-team/rev/793c662d0c89
Depends on: 1153403
Attached patch PatchSplinter Review
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
Attachment #8589843 - Attachment is obsolete: true
Attachment #8590442 - Attachment is obsolete: true
Attachment #8591116 - Flags: review+
Attachment #8591116 - Flags: approval-mozilla-beta?
Attachment #8591116 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/793c662d0c89
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Comment on attachment 8591116 [details] [diff] [review]
Patch

Should be in 38 beta 4.
Attachment #8591116 - Flags: approval-mozilla-beta?
Attachment #8591116 - Flags: approval-mozilla-beta+
Attachment #8591116 - Flags: approval-mozilla-aurora?
Attachment #8591116 - Flags: approval-mozilla-aurora+
QA Contact: camelia.badau
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?
Flags: needinfo?(jaws)
(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. :-)
Flags: needinfo?(jaws)
(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.
You need to log in before you can comment on or make changes to this bug.