[Tracking Requested - why for this release]: useless UI regressed by Bug 1035625 See attached screenshot STR 1. Open In-content Preferences 2. Select Content pane 3. Click Pop-ups [Exceptions...] 4. Resize the subdialog as small as possible 5. Close the subdialog 6. Click Fonts & Colors [Advanced...] Actual Results: The fonts subdialog cut off on bottom and right Expected Results: Should not cut off. Changing the size of the subdialog should be independent

STR 1. Open In-content Preferences 2. Select Content pane 3. Click Pop-ups [Exceptions...] 4. Resize the subdialog 5. Close the subdialog 6. Select Security pane 7. Click [Saved Passwords...]

in-content prefs is not shipping in 34. Dropping tracking.

In [0], you'll need to make sure the close function clears the width and height attributes from the dialog. http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/subdialogs.js

Thought to note that in a case like described in bug 1043612 the user might think it'd be worth saving the set dimensions.

Hello Tim, How should I go ahead with this bug? I have setup the source code of Mozilla and is able to run Nightly Debug ny ./mach run.

Hi Tim, I am able to clear the height and width of dialog boxes and the things are working perfectly. I have cleared the height and width to 0 on close. How should I move ahead by submitting the patch?

(In reply to Shubham Jindal from comment #6) > Hi Tim, > I am able to clear the height and width of dialog boxes and the things are > working perfectly. I have cleared the height and width to 0 on close. How > should I move ahead by submitting the patch? Awesome :) To create patches, you can follow the "Working on your first bug" section in [0], or if you prefer written docs, you can check [1]. Also, if you set the height and width to 0, the dialogs won't actually work after you close. We want here to remove the height and width attributes (this._box.removeAttribute("...")). [0] : http://codefirefox.com/ [1] : https://developer.mozilla.org/en-US/docs/Mercurial_Queues#Basic_Commands

I will follow and submit the patch. The dialog boxes are working even if I set them to 0.

Actually, setting attributes to 0 are behaving similar to removing the attributes.

(In reply to Shubham Jindal from comment #9) > Actually, setting attributes to 0 are behaving similar to removing the > attributes. This is because we have a min-width/height on the dialog, so removing the attributes will act the same as setting to 0. But removing the attributes (clearly means reset) is more explicit to understand than setting to 0. People looking at the code in the future should be able to guess what it does.

Yes. I have changed the code now. Removed the attributes instead of setting it to zero.

Comment on attachment 8532836 [details] [diff] [review] Reseted the height and width of sub-dialogs in about:preferences Review of attachment 8532836 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch ! This patch looks good, just a few nits to fix. Since I'm not a peer, I'm gonna ask review from a peer. Also, please change the commit message (using hg qref -e) to the following : Bug 1043346 - In-content prefs - Reset width and height of sub-dialogs when closing. r=jaws ::: browser/components/preferences/in-content/subdialogs.js @@ +89,2 @@ > this._overlay.style.visibility = ""; > // Clear the sizing inline styles. Since we're removing the line below, this comment should be more relevant : // Clear the sizing attributes @@ +89,3 @@ > this._overlay.style.visibility = ""; > // Clear the sizing inline styles. > this._frame.removeAttribute("style"); This line can be removed since no style attribute is set anyway.

Should I submit a new patch with the above mentioned changes? Just to confirm, I should change the commit message to "Bug 1043346 - In-content prefs - Reset width and height of sub-dialogs when closing.". I am getting confused what "r=jaws" means. :P

(In reply to Shubham Jindal from comment #14) > Should I submit a new patch with the above mentioned changes? Yes :) > Just to confirm, I should change the commit message to "Bug 1043346 - In-content > prefs - Reset width and height of sub-dialogs when closing.". I am getting > confused what "r=jaws" means. :P We include the name of the reviewer in each commit message, so it appears in the commit history. r=jaws means the patch has been reviewed by jaws. He's a peer, and I'm not, which is he should review the patch. So yes, you should change it to : "Bug 1043346 - In-content prefs - Reset width and height of sub-dialogs when closing. r=jaws"

(In reply to Tim Nguyen [:ntim] from comment #15) [...] > r=jaws means the patch has been reviewed by jaws. He's a peer, and I'm not, > which is he should review the patch. which is why*

Comment on attachment 8532840 [details] [diff] [review] Reset width and height of sub-dialogs when closing. I tested this patch out and it didn't work for me. I had to change this to remove the attribute from _box, not _frame. With that changed, the bug is fixed, but we will have to figure out what to do about bug 1043612. MattN, are you OK with us fixing this case before the persistence case?

For bug 1043612 , we'll set the dimensions when opening the dialog, so resetting the dimensions when closing shouldn't affect the other bug.

(In reply to Tim Nguyen [:ntim] from comment #19) > For bug 1043612 , we'll set the dimensions when opening the dialog, so > resetting the dimensions when closing shouldn't affect the other bug. Where do you propose we get those dimensions from? Can you put a patch on that bug that implements what you are describing and also works with the patch on this bug?

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #20) > (In reply to Tim Nguyen [:ntim] from comment #19) > > For bug 1043612 , we'll set the dimensions when opening the dialog, so > > resetting the dimensions when closing shouldn't affect the other bug. > > Where do you propose we get those dimensions from? Can you put a patch on > that bug that implements what you are describing and also works with the > patch on this bug? This is just a guess of how it's gonna work. If we're going to set the dimensions of the dialog, it's obviously after opening, because we can't predict what dialog the user is going to click. So resetting the dimensions after closing the dialog shouldn't affect the way the dialog opens.

Comment on attachment 8532840 [details] [diff] [review] Reset width and height of sub-dialogs when closing. Review of attachment 8532840 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/preferences/in-content/subdialogs.js @@ -87,5 @@ > } > > this._overlay.style.visibility = ""; > - // Clear the sizing inline styles. > - this._frame.removeAttribute("style"); Are you sure we don't need to remove @style anymore? I think we could save the value in bug 1043612 just before removing these attributes, right?

(In reply to Tim Nguyen [:ntim] from comment #21) > This is just a guess of how it's gonna work. If we're going to set the > dimensions of the dialog, it's obviously after opening, because we can't > predict what dialog the user is going to click. The point of bug 1043612 is to persist the size that the user resized the dialog to so we need to save on closing.

Hi Shubham, do you feel that you have enough feedback now to put up a final patch on this bug?

(In reply to Matthew N. [:MattN] from comment #23) > (In reply to Tim Nguyen [:ntim] from comment #21) > > This is just a guess of how it's gonna work. If we're going to set the > > dimensions of the dialog, it's obviously after opening, because we can't > > predict what dialog the user is going to click. > > The point of bug 1043612 is to persist the size that the user resized the > dialog to so we need to save on closing. Yes, but we have different dialogs with different sizes to handle, and there's one XUL element for all dialogs, which means the dimensions will be set when the user opens the dialog (since each dialog has a different size, and we can't predict which dialog is going to be opened).

(In reply to Tim Nguyen [:ntim] from comment #25) > (In reply to Matthew N. [:MattN] from comment #23) > > (In reply to Tim Nguyen [:ntim] from comment #21) > > > This is just a guess of how it's gonna work. If we're going to set the > > > dimensions of the dialog, it's obviously after opening, because we can't > > > predict what dialog the user is going to click. > > > > The point of bug 1043612 is to persist the size that the user resized the > > dialog to so we need to save on closing. > > Yes, but we have different dialogs with different sizes to handle, and > there's one XUL element for all dialogs, which means the dimensions will be > set when the user opens the dialog (since each dialog has a different size, > and we can't predict which dialog is going to be opened). Different sizes should be persisted in bug 1043612 based on the ID of the dialog which is how XUL persistence currently works.

> do you feel that you have enough feedback now to put up a final patch on this bug? I actually got confused by going through all the comments. I am not sure what I have to do? whether to reset the dimensions on opening or closing or something else?

(In reply to Shubham Jindal from comment #27) > > do you feel that you have enough feedback now to put up a final patch on this bug? > > I actually got confused by going through all the comments. I am not sure > what I have to do? whether to reset the dimensions on opening or closing or > something else? Just replace this._frame with this._box in the code you added, and restore the this._frame.removeAttribute("style"); you removed.

I missed the comment in the previous patch.

Comment on attachment 8536433 [details] [diff] [review] bug1043346.diff Review of attachment 8536433 [details] [diff] [review]: ----------------------------------------------------------------- Perfect! Thanks for the patch Shubham, congratulations on getting your first bug fixed. The next step is for your patch to get checked in, at which point it may take a day or two before it gets merged to mozilla-central. Once in mozilla-central, it will appear in Firefox Nightly the next day.

Thanks jaws and ntim for helping me through the bug.:)

(In reply to Shubham Jindal from comment #34) > Thanks jaws and ntim for helping me through the bug.:) You're welcome. I would be happy to help you out with another bug :)

Sure. You can always suggest me bugs to work on :) I would love to resolve them.

(In reply to Shubham Jindal [:shubham] from comment #36) > Sure. You can always suggest me bugs to work on :) I would love to resolve > them. Thanks for contributing :) You can try bug 1113096.

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5 using latest Nightly 37.0a1 (buildID: 20141221030204).