Alice0775 White Reporter
	Description • 10 years ago

[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

	Alice0775 White Reporter
	Comment 1 • 10 years ago

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

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 2 • 10 years ago

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

	Tim Nguyen :ntim
	Comment 3 • 10 years ago

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

	Albert Scheiner [:alberts]
	Comment 4 • 10 years ago

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

	Shubham Jindal [:shubham] Assignee
	Comment 5 • 10 years ago

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.

	Shubham Jindal [:shubham] Assignee
	Comment 6 • 10 years ago

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?

	Tim Nguyen :ntim
	Comment 7 • 10 years ago

(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

	Shubham Jindal [:shubham] Assignee
	Comment 8 • 10 years ago

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

	Shubham Jindal [:shubham] Assignee
	Comment 9 • 10 years ago

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

	Tim Nguyen :ntim
	Comment 10 • 10 years ago

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

	Shubham Jindal [:shubham] Assignee
	Comment 11 • 10 years ago

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

	Tim Nguyen :ntim
	Comment 13 • 10 years ago

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.

	Shubham Jindal [:shubham] Assignee
	Comment 14 • 10 years ago

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

	Tim Nguyen :ntim
	Comment 15 • 10 years ago

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

	Tim Nguyen :ntim
	Comment 16 • 10 years ago

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

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

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?

	Tim Nguyen :ntim
	Comment 19 • 10 years ago

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

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

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

	Tim Nguyen :ntim
	Comment 21 • 10 years ago

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

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

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?

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

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

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

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

	Tim Nguyen :ntim
	Comment 25 • 10 years ago

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

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

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

	Shubham Jindal [:shubham] Assignee
	Comment 27 • 10 years ago

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

	Tim Nguyen :ntim
	Comment 28 • 10 years ago

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

	Shubham Jindal [:shubham] Assignee
	Comment 30 • 10 years ago

I missed the comment in the previous patch.

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

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 32 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/10bafc4817e3

	Carsten Book [:Tomcat]
	Comment 33 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/10bafc4817e3

	Shubham Jindal [:shubham] Assignee
	Comment 34 • 10 years ago

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

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

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

	Shubham Jindal [:shubham] Assignee
	Comment 36 • 10 years ago

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

	Tim Nguyen :ntim
	Comment 37 • 10 years ago

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

	Camelia Badau [:cbadau], Desktop QA
	Comment 38 • 10 years ago

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