357725 - Top-level XUL windows should support size constraints

Status: RESOLVED FIXED

(Core :: Widget, defect, P3)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Right now specifying minwidth/minheight/maxwidth/maxheight attributes in a <xul:window> has no effect. This isn't a huge problem for normal chrome windows with OS-supplied window decorations, but it is a problem for hidechrome windows. If a hidechrome window has a resizer element, for instance, you can resize the window to *nothing* and lose the window forever.

I originally set out to fix the hidechrome problem only, but after chatting with a few folks it seemed that being able to specify size constraints in a XUL file might be a nice feature.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Incomplete patch

This is still in progress and I'm posting it mainly to get some feedback on the approach.

This works almost perfectly in windows as is. The only remaining issue has to do with mouse tracking on the resizer element. Right now you can shrink the window to it's minimum size and then expand again before the mouse has returned to the resizer element.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: XUL test window

Here's a test window. The patch works with and without the hidechrome attribute (on windows).

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch

Pretty darn close this time. Just posting to get it out of my tree.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: HideChrome test

Test XUL window with 10px wide resizers on all sides. Kinda like what Songbird does at the moment.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Normal test

Test XUL window with a normal frame and a "bottomright" resizer. This is similar to what Firefox uses at the moment.

Comment 6

[Neil Deakin]

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

You should be able to declare the unimplemented functions in nsBaseWidget, rather than add these for every platform.

Comment 7

[Boris Zbarsky [:bzbarsky]]

And the nsIWidget change needs document the coordinates used.  If they're pixels, make sure to document which pixels.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Oh, and what happens for non-top-level "windows" (do you mean widgets?) should be documented.  Does it throw?  Is it just ignored?  Will calling GetSizeConstraints after SetSizeConstraint work or not?

Not to mention that there needs to be documentation as to what a "top level window" is.  Especially in a non-XUL embedding situation (think camino).

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch v1.0

Here's an updated patch that takes those comments into account.

bz, are those comments acceptable?

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 245631 [details] [diff] [review]
Patch v1.0

Hmm, somehow I got some nsStringGlue changes in there. Those aren't necessary.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yeah, those comments cover all my issues except the "which pixels" one.  I assume you want CSS pixels here, but you might want to check with a widget peer.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

IMHO the size constraints to nsIWidget::Get/SetSizeConstraints should be in device pixels (like the rest of nsIWidget coordinates), and the size constraints in XUL should be in CSS pixels. This is consistent with what the rest of the world will look like when we land the units patch. But for now I don't think we have a way to implement that difference, we have to assume that they're 1:1. We can still document in comments how it *should* work.

Ben, if none of that makes any sense, find me and I'll explain it.

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch v1.1

Fixed a typo and now using NSCoordToInt just to be safe. Also fixed up the CSS/device pixel comments after roc explained the situation to me.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks good to me!

Comment 15

[Neil Deakin]

Comment on attachment 245701 [details] [diff] [review]
Patch v1.1

>+           nsCOMPtr<nsIWidget> tempWidget = mainWidget->GetParent();
>+           while (tempWidget) {
>+             mainWidget = tempWidget;
>+             tempWidget = tempWidget->GetParent();
>+           }

Does this mean a resizer in a child window will resize the parent? That shouldn't be the case.

I think that the constraint code in ResizerFrame would be better in the window widget's Resize method. Was it added to resizer frame because you need to know the direction in which the window is resized? If so, that's OK.

> 
>+#define MINIMUM_WIDTH  120
>+#define MINIMUM_HEIGHT 60
>+

Why these values? Web site popup windows for instance can be a minimum of 100x100. We should set a fairly small default minimum I think.

>+  // Handle minimum width case
>+  if (mConstrainedMinWidth != -1) {
>+    // Make sure we can do the calculations
>+    if (aInfo->cx) {
>+      // Reset the width if we're shrinking below the minimum
>+      if (aInfo->cx < mConstrainedMinWidth) {
>+        // Make sure that we don't accidentally move the window while trying
>+        // to enforce the minimum size
>+        nscoord lastLeftX = mLastBounds.x;

Shouldn't mLastBounds be set whenever the window moves and resizes? The EnsureSizeConstraints function is only called when the window resizes.

>+
>+#define DEFAULT_MINIMUM_WIDTH  60
>+#define DEFAULT_MINIMUM_HEIGHT 60
>+

Why are these values different that those defined earlier?

It would be nice to add some comments to the sizing algorithms, as it took me a while to figure out what it was doing, as I kept getting confused.

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch v2.0

Okay, Neil, don't hate me for this but here's a better way to do things!

Most of this patch is identical to the others. The differences:

1. Use WM_GETMINMAXINFO to handle the windows-specific size constraints. That's what the message is for, after all, and it means that I don't have to do all that math in the widget code.
2. I spent some time fixing maximization issues. Now the window behaves exactly like the windows console (cmd.exe) when maximized/restored, including the animated rects madness.
3. Resizers disable themselves if their parent window is maximized. This prevents some rather ugly repaint issues (if a resizer is enabled on a maximized window then sizing causes the window to revert to 'normal' mode which calls the standard animation mess again).
4. Since I needed a way to disable the resizer I went ahead and implemented the fix for bug 360890 as it was only a few extra lines.
5. And since I was fixing the MINMAX stuff anyway I went ahead and fixed bug 321595. That was only about two extra lines as well.
6. I addressed your initial review comments, but let me address them more thoroughly:

(In reply to comment #15)
> (From update of attachment 245701 [details] [diff] [review] [edit])
> Does this mean a resizer in a child window will resize the parent? That
> shouldn't be the case.

Fixed.

> I think that the constraint code in ResizerFrame would be better in the window
> widget's Resize method. Was it added to resizer frame because you need to know
> the direction in which the window is resized? If so, that's OK.

It's actually because of the moving problem. Resizing from the top or left actually causes the window to move and that will not get trapped by the widget's  size constraint code. A pleasant side-effect (and how often do those occur?) is that resizers will obey size constraints on all platforms, regardless of whether or not the platform-specific code has been implemented. That won't fix native window decorations, but oh well.

> Why these values? Web site popup windows for instance can be a minimum of
> 100x100. We should set a fairly small default minimum I think.

I now let Windows decide based on the minimum default size set for all new popup windows. This may bite us someday if someone desperately wants a 5x5 window or something (I can't think of a decent use case), but for now it seems reasonable.

> Shouldn't mLastBounds be set whenever the window moves and resizes? The
> EnsureSizeConstraints function is only called when the window resizes.

All that code is gone now (MINMAX takes care of all that)

> It would be nice to add some comments to the sizing algorithms, as it took me a
> while to figure out what it was doing, as I kept getting confused.

Done. I hope that's enough, let me know if it's still unclear.

Comment 17

[Neil Deakin]

Comment on attachment 245991 [details] [diff] [review]
Patch v2.0

Index: layout/xul/base/src/nsResizerFrame.cpp
>  nsIFrame*
>  NS_NewResizerFrame(nsIPresShell* aPresShell, nsStyleContext* aContext)
>  {
>    return new (aPresShell) nsResizerFrame(aPresShell, aContext);
>  } // NS_NewResizerFrame
>  
> +

Don't add extra blank lines in parts of the code you haven't changed. There's a few places where this happens.

> +PRBool
> +nsResizerFrame::GetDisabledAttr()
> +{

This is a confusing function name as it returns a boolean but the return value isn't the disabled state. Either call the function CacheDisabledAttr, or have it return the disabled state.

> +nsresult
> +nsResizerFrame::GetWindowFromPresContext(nsPresContext* aPresContext,
> +                                         nsIBaseWindow** _result)
> +{
> +  nsPIDOMWindow* domWindow =
> +    aPresContext->PresShell()->GetDocument()->GetWindow();

Add an NS_ENSURE_TRUE for the document too to be safe.

> +nsResizerFrame::IsDisabled(nsPresContext* aPresContext)
...
> +  if (sizeMode == nsSizeMode_Maximized)
> +    return PR_TRUE;
> +
> +  return PR_FALSE;

Just use return (sizeMode == nsSizeMode_Maximized);

Index: widget/src/windows/nsWindow.cpp
       DispatchStandardEvent(NS_DISPLAYCHANGED);
> +
> +      // When the window's chrome is hidden and the work area changes, 
> +      // the window is not invalidated as it is supposed to
> +      if (mIsHideChrome)
> +        Invalidate(PR_TRUE);
> +

I don't know what this is doing. Make sure someone else reviews these lines.

> +        // Figure out the maximized window's size and position
> +        RECT workArea;
> +        if (::SystemParametersInfo(SPI_GETWORKAREA, 0, &workArea, 0)) {
> +          PRInt32 workWidth = workArea.right - workArea.left;
> +          maxWidth = mConstrainedMaxWidth != -1 ?
> +                     PR_MIN(workWidth, mConstrainedMaxWidth) :
> +                     workWidth;
> +
> +          PRInt32 workHeight = workArea.bottom - workArea.top;
> +          maxHeight = mConstrainedMaxHeight != -1 ?
> +                      PR_MIN(workHeight, mConstrainedMaxHeight) :
> +                      workHeight;
> +          
> +          left = workArea.left;
> +          top = workArea.top;

Ask a Windows person to review this part since I don't know whether that is the right way to handle maximized windows.

> +      if (top != -1 && top > mmi->ptMaxPosition.y)
> +        mmi->ptMaxPosition.y = top;
> +
> +      result = PR_FALSE;
> +    }

Don't think the result = PR_FALSE is needed here

OK, with those comments addressed.

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 245991 [details] [diff] [review]
Patch v2.0

Ere, can you take a quick look the windows-specific code that Neil pointed out?

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #17)

> Don't add extra blank lines in parts of the code you haven't changed. There's a
> few places where this happens.

Ok, I removed those. I was just trying to make the space between functions consistent throughout the file.

> This is a confusing function name as it returns a boolean but the return value
> isn't the disabled state. Either call the function CacheDisabledAttr, or have
> it return the disabled state.

Good call, changed to CacheDisabledAttr.

> Add an NS_ENSURE_TRUE for the document too to be safe.

Ha, I had just copied that from the existing code but it felt sorta slimy. I 'll update this to be null-safe.

> Just use return (sizeMode == nsSizeMode_Maximized);

Done.

> Don't think the result = PR_FALSE is needed here

Good catch. Fixed.

Thanks for sifting through all that!

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch v2.1

Updated with Neil's comments addressed.

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 246075 [details] [diff] [review]
Patch v2.1

Sorry, Ere, I missed one edge case on the maximizing-overlaps-the-taskbar bit... New patch in a sec.

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch v2.2

Here we go. I missed the case where a hidechrome window is maximized from the taskbar. I have to set a flag in WM_SYSCOMMAND to catch the fact that the window is maximizing before the WM_GETMINMAXINFO message is sent. At that point mSizeMode has not yet been updated to 'maximized'. I clear the flag in WM_WINDOWPOSCHANGED because all sizing has completed.

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch v2.3

Grr, one other small edge case fixed... Restoring a window that was first maximized then minimized via the system menu covered the taskbar. Fixed now.

Comment 24

[Ere Maijala (slow)]

Comment on attachment 246178 [details] [diff] [review]
Patch v2.3

Rename mIsHideChrome to mIsChromeHidden or something.


+  PRInt32 sysMinWidth = ::GetSystemMetrics(SM_CXMINTRACK);
+  mConstrainedMinWidth = PR_MAX(sysMinWidth, aMinWidth);

Simplify this and height:

mConstrainedMinWidth = PR_MAX(::GetSystemMetrics(SM_CXMINTRACK), aMinWidth);


Have you tried to use GetWindowPlacement in WM_GETMINMAXINFO instead of the ugly booleans? I'd really like something else better..

Comment 25

[Ben Turner (not reading bugmail, use the needinfo flag!)]

*** Bug 360890 has been marked as a duplicate of this bug. ***

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #24)
> Have you tried to use GetWindowPlacement in WM_GETMINMAXINFO instead of the
> ugly booleans? I'd really like something else better..

Agreed. I did try GetWindowPlacement, but unfortunately the flags are not updated when they should be. Here's the progression of messages sent:

1. (Select 'Maximize' from system menu)
2. WM_SYSCOMMAND
3. WM_GETMINMAXINFO
4. (Animating rects indicating maximize)
5. WM_WINDOWPOSCHANGING
6. WM_GETMINMAXINFO
7. (Window maximized)
8. WM_WINDOWPOSCHANGED

So Windows uses two WM_GETMINMAXINFO messages - one for the animated rects and the other for actually sizing the window. GetWindowPlacement only reports that the window is maximized on #6, not #3. If we only modify the size for #6 then those animated rects fire incorrectly and look pretty funny. I think those booleans, ugly as they are, may be the only way around the problem.

I'm all for other approaches but I can't think of anything else at the moment...

Comment 27

[Ere Maijala (slow)]

Ok, then *sigh*. Just fix the minor stuff and you're good to go.

Comment 28

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch v2.4

Final patch for the trunk. Also tweaked the logic in the SC_RESTORE case to always reset mWasMaximized. Your other comments were addressed.

Comment 29

[Ere Maijala (slow)]

Comment on attachment 246558 [details] [diff] [review]
Patch v2.4

+      // Restrict the window from covering the taskbar if hidechrome is enabled

Just make this something like "... if chrome is hidden" and it's good. r=emaijala

Comment 30

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch v2.5

roc, everything look ok to you?

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  NS_WARN_IF_FALSE(GetInitialDirection(mDirection),
+                   "GetInitialDirection failed!");
+  NS_WARN_IF_FALSE(CacheDisabledAttr(), "CacheDisabledAttr failed!");

I think we prefer to have side-effecting code outside of macros like these.

Why not define SizeConstraints in nsIWidget so you can use it in those methods and you don't have to define it twice?

+         // Make sure a constraint is set

Should really be "Check if a constraint is set"

+         nsPoint nsMoveBy(0,0), nsSizeBy(0,0);
+         nsPoint nsBeyondMaximum(0,0), nsBeyondMinimum(0,0);
          nsPoint nsMouseMove(aEvent->refPoint - mLastPoint);

These names are horrible, they look like types? Would you mind fixing that?

+         PRInt32 x, y, cx, cy;
+         mResizingWindow->GetPositionAndSize(&x, &y, &cx, &cy);

"cx" and "cy" are unusual names for "width" and "height", would you mind fixing that? (I know this isn't your fault!)

Could we simplify things by instead of having -1 be a magic constraint value, say that (0,0,PR_INT32_MAX,PR_INT32_MAX) are the "no constraint" values? Then I think we could merge the min-width and max-width code to get something simpler, ditto for height.

Someone should figure out if we really need GetContentOf and if so, make it return a raw pointer, but whatever.

Why are we caching the disabled state? Is there any reason to believe that checking the attribute is a performance issue?

Comment 32

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Sorry, roc, I'll address your comments soon. It has been a busy few weeks at the office...

Comment 33

[Håkan Waara]

(In reply to comment #30)
> Created an attachment (id=246618) [details]
> Patch v2.5

Dunno if this patch is still interesting, but I noticed that it misses to rev the IID of nsIWidget.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 246618 [details] [diff] [review]
Patch v2.5

clearing request until updated patch is supplied

Comment 35

[Mook (songbird dead account)]

Attachment: updated to trunk

This is the last patch, updated to trunk; I imagine it has these problems:
- Doesn't do CSS pixel -> device pixel conversion (hey, Gecko actually does DPI now!)
- Doesn't read anything from CSS (it should be using min-width etc.)
- Might want to integrate with the BeginResizeDrag stuff
- Win32 only :/

But this does attempt to address comment 31 (and rev nsIWidget IID).

roc, could you give pointers towards some of these problems? Thanks :)

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can I deal with this after 1.9?

Comment 37

[Mook (songbird dead account)]

Comment on attachment 297280 [details] [diff] [review]
updated to trunk

It'd be nice to get this in, but I don't see it happening with everything else you have to do :)

Marking as depending on bug 407616 because that's going to conflict like crazy, and if this isn't going in before that anyway...

P.S. Hints on getting this to obey CSS min-width etc. would be great :)  So far it looks like I need to somehow get the docshell, and from there the document element and presshell, then the nsIFrame / stylePosition (which is in twips?).  Not sure how to get the docshell, though...

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

from the widget you can get a view and then a frame. However, post-1.9 this is going to change a bit, hopefully becoming considerably simpler.

Comment 39

[Mook (songbird dead account)]

(Sorry, should this be going on a newsgroup instead?)

Unfortunately, nsIView::GetViewFor((nsWindow*)) is just giving me null.  The client data there appears to be a nsWebShellWindow (but I can't get at that header) which isn't a view...

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Your first child widget probably has the view. Evil hack but that might get you working for now.

Comment 41

[Mook (songbird dead account)]

Attachment: now with css

still not asking for r?, waiting until post-1.9.  Thanks roc!

Comment 42

[Mark Finkle (:mfinkle) (use needinfo?)]

we are post 1.9

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsResizerFrame got a lot simpler, so you'll need to update your patch.

Making nsWindow use a lot of layout and style is bad. You should be able to set the constraints in the window, from layout, so that the window doesn't have to ask for them.

Comment 44

[[e-mail]]

>+        if (::SystemParametersInfo(SPI_GETWORKAREA, 0, &workArea, 0)) {
Use ::GetMonitorInfoW here, SPI_GETWORKAREA only works up to Windows 95.

Comment 45

[Mook (songbird dead account)]

Random notes from IRC for when I get to revise the patch to work better
- don't make layout depend on widget; instead, in nsRootBoxFrame::Reflow, have that call nsIWidget::SetSizeConstraints (using values from the frame's style context)
- get the widget via frame->GetWindow()
- make sure child XUL windows don't get to set constraints (presumably nsXULWindow must be able to figure out if it's a child)

Comment 46

[David Bradley]

Looks like this patch may have introduced a Windows only memory leak. The leak shows up with the patch but not without the patch. Could be the way Songbird is doing something, but I thought I'd throw it out in case someone else might have an Ah Ha moment.

The following show up in our leak results in Songbird.
nsThebesDeviceContext
nsThebesRenderingContext

Comment 48

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

This bug fixed would be great for us in calendar land. Its probably too late for 1.9.1, but I'd love to see this soon!

Comment 49

[Dave Townsend [:mossop]]

This is a pretty basic feature that is missing from our platform. Ben tells me he probably isn't coming back to this and Mook likely won't have time to work on it since songbird are on 1.9.0 for the time being. Is there any way we can get some traction on this for 1.9.2?

Comment 50

[Neil Deakin]

I have this mostly working again on Mac so I'll polish it a bit.

Comment 51

[Neil Deakin]

Attachment: work in progress

This patch adds support for minimum and maximum sizes on Mac and Linux and seems to work fine. It also now supports resizers inside popups.

Haven't tested the Windows part. It probably doesn't even build.

Comment 52

[Neil Deakin]

Attachment: support this on all platforms with tests

Comment 53

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I suspect it would be simpler to incorporate the size constraints in AdjustDimensions.

+  // cannot resize disabled popups
+  if (mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::disabled,
+                            nsGkAtoms::_true, eCaseMatters))

Why not?

+  PRUint32 minWidth;
+  PRUint32 maxWidth;
+  PRUint32 minHeight;
+  PRUint32 maxHeight;

Document that these are in device pixels?

+     * XXXbent All of the following pixel values are supposed to be device
+     *         pixels, but note that we currently assume device pixels map 1:1
+     *         to CSS pixels.

Is this true?

Should nsIWidget::Resize really honour the size constraints? When does it matter? XUL persisted sizes, maybe?

+  // check for 'i' in min
+  return attrName[1] == 'i' ? 0 : NS_MAXSIZE;

This is weird. Why not make attrName (and styleName?) nsIAtom* instead?

All the goop in nsXULWindow::SetMinMaxSize to get style seems unfortunate. How about instead doing mDocShell->GetPresShell()->FrameConstructor()->GetRootElementStyleFrame()->GetStylePosition()?

Comment 54

[Neil Deakin]

(In reply to comment #53)
> +  // cannot resize disabled popups
> +  if (mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::disabled,
> +                            nsGkAtoms::_true, eCaseMatters))
> 
> Why not?

Ben added this, but it probably shouldn't be there. I'll remove it.


> This is weird. Why not make attrName (and styleName?) nsIAtom* instead?
> 

I don't think converting to atoms would make this simpler. I'll just use a boolean flag instead.


> All the goop in nsXULWindow::SetMinMaxSize to get style seems unfortunate. How
> about instead doing
> mDocShell->GetPresShell()->FrameConstructor()->GetRootElementStyleFrame()->GetStylePosition()?

I'd only be able to that from within layout.

Comment 55

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Ooooh. I'll use that feature as soon as it's available... See http://tinyurl.com/crl2u3

Thanks for working on this, Enn !!!

Comment 56

[Neil Deakin]

Attachment: address comments

Comment 57

[Markus Stange [:mstange]]

So for panels, setting the minimum size goes through SetPreferredBounds, which has access to the actual intrinsic minimum size of the frame, even if no minimum size is specified using CSS / attributes.
Wouldn't it be better to do something similar for normal windows, too? I think this would make all those workarounds for clipped window contents obsolete (see e.g. bug 484013, bug 420736, bug 394598).

Another issue: Dynamic changes to the attributes / properties aren't propagated properly. E.g. if I set minwidth to 800 on the main browser window using the DOM Inspector, the size isn't honored when I resize the window with the native window resizer; only after I execute javascript:resizeTo(600, 400).

Moreover, if I add a resizer[dir=bottomright] to the identity popup, the panel runs away to the top left during a resize.

I also get loads of warnings during resize, but I don't know if they're caused by this patch:
WARNING: We should have hit the document element...: file /Users/markus/Programming/mmozilla/layout/xul/base/src/nsBoxObject.cpp, line 185

Comment 58

[Markus Stange [:mstange]]

I've found more issues with XUL resizers in top level windows (I added a resizer to the statusbar using the DOM Inspector):
 1. Funny things happen when you drag the resizer so far to the top left that
    the window would end up with a negative size: the window suddenly becomes
    very wide or very high.
 2. The resizer seems to respect only relative mouse cursor motions instead of
    ensuring that the it is always under the mouse cursor (as far as possible,
    respecting the window size constraints, of course).
    What I mean is:
    If I set a minwidth on the window and then drag the resizer so far to the
    left that it can't follow (because it respects the minwidth), and then
    drag to the right again, the resizer already moves to the right before
    the mouse cursor has returned on it. This shouldn't happen.

Comment 59

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 382538 [details] [diff] [review]
address comments

Let's get Markus' questions addressed.

Comment 60

[Neil Deakin]

(In reply to comment #57)
> So for panels, setting the minimum size goes through SetPreferredBounds, which
> has access to the actual intrinsic minimum size of the frame, even if no
> minimum size is specified using CSS / attributes.
> Wouldn't it be better to do something similar for normal windows, too? I think
> this would make all those workarounds for clipped window contents obsolete (see
> e.g. bug 484013, bug 420736, bug 394598).
> 

Yes, there's an issue where the width/height attributes on the <window> are used to both specify the box size of the content as well as the size of the outer window frame, and confusion occurs because both intrepretations are used at different times. I haven't changed that here.

The other issues are problems but not actually this bug, no? That is, do they work properly in some way before this patch?

Comment 61

[Markus Stange [:mstange]]

(In reply to comment #60)
> The other issues are problems but not actually this bug, no?

Oh, right, the problems in comment 58 are not caused by this patch.

Comment 62

[Neil Deakin]

(In reply to comment #57)
> So for panels, setting the minimum size goes through SetPreferredBounds, which
> has access to the actual intrinsic minimum size of the frame, even if no
> minimum size is specified using CSS / attributes.
> Wouldn't it be better to do something similar for normal windows, too? I think
> this would make all those workarounds for clipped window contents obsolete (see
> e.g. bug 484013, bug 420736, bug 394598).

I'm not really convinced it should do that for panels either. I think that it's more likely someone would want to explicitly set a minimum size if desired and if not, either not have one, or just use some default value.

> 
> Another issue: Dynamic changes to the attributes / properties aren't propagated
> properly. E.g. if I set minwidth to 800 on the main browser window using the
> DOM Inspector, the size isn't honored when I resize the window with the native
> window resizer; only after I execute javascript:resizeTo(600, 400).

Neither are changes to the width/height attribute, but I suppose there isn't a way of changing the minimum or maximum size any other way either.

Comment 63

[Markus Stange [:mstange]]

(In reply to comment #62)
> I'm not really convinced it should do that for panels either. I think that it's
> more likely someone would want to explicitly set a minimum size if desired and
> if not, either not have one, or just use some default value.

What makes you think so? I can't think of a common case where it's desirable to allow the window to be so small that its contents are clipped.

I think using the root frame's intrinsic constraints for the window constraints is a good fit and supports all use cases:
 - Default is "ensure that the window is large enough for its contents".
 - If you want to have a larger minimum size, you can just set it using CSS
   min-width / min-height. This will automatically be reflected in the frame's
   GetMinSize.
 - If you want to allow the window to clip its contents, just set
   overflow to hidden. This will set the intrinsic minimum size to 0x0.
   (I've tested this.)

Comment 64

[Markus Stange [:mstange]]

Hmm, turns out that using GetMinSize is only half the story. As soon as there's wrapped text in the window, the fun starts, since then the window's minimum height depends on its width. The height returned by GetMinSize will be the minheight that's valid if the window is infinitely wide - so the value is too small in most cases.

The patch I've got now solves this by not relying on the size constraints that are set on the nsIWidget; instead, it also checks and adjusts the window's size on every resize. On Mac, it does this in windowWillResize, so it can adjust the target size before the native window is actually resized.

Does anybody know if there are similar APIs on the other platforms? To be specific, I need a way to get the "intended new size" before the window is drawn in the resized state, so that adjusting the size doesn't flicker.

Comment 65

[Markus Stange [:mstange]]

Neil R. told me that there's WM_WINDOWPOSCHANGING for Windows.

Comment 66

[Neil Deakin]

(In reply to comment #63)
> What makes you think so?I can't think of a common case where it's desirable to
> allow the window to be so small that its contents are clipped.
> 

It is however the way in which most other application windows work. One can shrink a window down smaller than the UI inside it. Also, when a window size is changing, the UI often updates to hide items which could further adjust the intrinsic size.

In most cases, the intrinsic size is used to determine the default width and height. Using this for the minimum size as well would mean that one could never shrink it smaller than this size.

Finally, if the controls inside a window dictated a minimum size of, say, 400 pixels, you would have no means of specifying that you wanted a minimum size of 200 pixels.

Comment 67

[Markus Stange [:mstange]]

(In reply to comment #66)
> (In reply to comment #63)
> > What makes you think so?I can't think of a common case where it's desirable to
> > allow the window to be so small that its contents are clipped.
>
> It is however the way in which most other application windows work.

Which applications have you tested? On Mac, the only app I've found that allows clipping is "Preview", which allows the toolbar to get small enough that toolbar buttons are hidden. But the hidden buttons are put in a overflow menu - so they've explicitly considered this case.

> Also, when a window size is
> changing, the UI often updates to hide items which could further adjust the
> intrinsic size.

Where's the problem? Adjusting the intrinsic size will mean that it's propagated to the window constraints, so this should work just fine, no?

> Using this for the minimum size as well would mean that one could never
> shrink it smaller than this size.

Right. The user can't do that unless it has been explicitly allowed (by overriding min-width / min-height).
I still think that's the right default.

> Finally, if the controls inside a window dictated a minimum size of, say, 400
> pixels, you would have no means of specifying that you wanted a minimum size
> of 200 pixels.

I wanted to write "You just have to set min-width / min-height on the window. This will override the intrinsic minimum size" - but I've noticed that it doesn't always work. It works e.g. for the Certificate Manager, but it doesn't work for the main browser window.
I haven't found out yet what makes the difference, but I'm learning a great deal about XUL layout on my journey...

Comment 68

[Neil Deakin]

> I wanted to write "You just have to set min-width / min-height on the window.
> This will override the intrinsic minimum size"

I'm not sure I follow. If the intrinsic width of an element is 400 pixels, the intrinsic width of its parent is also at least 400 pixels. I want a window with a minimum width of 200 pixels.

Maybe I'm misunderstanding your thought here. Which of the following are you suggesting be used:

1. If a minwidth or minheight are specified on <window> use that value exactly. If not specified, use GetMinSize to determine the minimum size.
2. Use GetMinSize to determine the size always.

Comment 69

[Markus Stange [:mstange]]

(In reply to comment #68)
> I'm not sure I follow. If the intrinsic width of an element is 400 pixels, the
> intrinsic width of its parent is also at least 400 pixels.

Yeah, that's what I thought. But that's not what my printfs tell me. If I set min-width: 200px; on the Certificate Manager window, its GetMinSize changes to 200px, even though its intrinsic size is larger. Maybe I'm using the wrong frame... it's probably best if I debug this a little instead of telling half-truths.

I've tried to create a little screencast, but I've lost the fight against the codecs. I hope your QuickTime can play this:
http://tests.themasta.com/tmp/minsize3.mov

> Maybe I'm misunderstanding your thought here. Which of the following are you
> suggesting be used:
>
> 1. If a minwidth or minheight are specified on <window> use that value exactly.
> If not specified, use GetMinSize to determine the minimum size.
> 2. Use GetMinSize to determine the size always.

Hm, seems like I have been too focused on 2. Now that you say it, 1 will be a much simpler solution to the "override intrinsic size" problem. Let's do it that way :-)

Comment 70

[Markus Stange [:mstange]]

TryServer build of my WIP: https://build.mozilla.org/tryserver-builds/[e-mail]-try-e53c814f73d/try-e53c814f73d-macosx.dmg

Demo of size constraining that's not possible with either of minwidth/minheight attributes and GetMinSize, i.e. which requires what I outlined in comment 64:
http://www.screencast.com/users/markus_stange/folders/Jing/media/140d686f-5d7b-4618-bf63-6da672d0701d

Comment 71

[Neil Deakin]

Markus, are you working on this? I can provide feedback if you post a patch of your work so far.

Comment 72

[Markus Stange [:mstange]]

Not actively. I'll try to post a cleaned-up patch this afternoon.

Comment 73

[Markus Stange [:mstange]]

I have the cleaned-up patch almost ready, but for some reason it's preventing some windows from opening. I need to debug that.

Comment 74

[Markus Stange [:mstange]]

Attachment: intrinsic size approach, v1

This patch takes approach 2: Always use GetMinSize. Putting back support for the window attributes wouldn't be hard, but I didn't do it for two reasons:
 1. I couldn't think of a clean way to implement it. The thing that controls
    that the attributes have a higher priority than the intrinsic size needs to
    live either in nsXULWindow or in nsIWidget. In order to get the info there,
    I'd have to add methods to nsIBaseWindow or nsIWidget, respectively. I
    didn't want to do that.
 2. I still don't see the need for these attributes. style="min-width: 200px"
    is not much longer than width="200". And specifying the *inner* window size
    for the size constraints makes more sense to me.

This patch doesn't implement the way that I outlined in comment 64, since I couldn't get that to work reliably.
So this patch will allow windows with wrapped text in them to be made smaller than they should actually be. But we can worry about that later.

Behavior with this patch:

- Default: Use the intrinsic minimum / maximum size of the window's content.
- The intrinsic minimum size can be overridden by specifying a min-width/height on the window that's different from the 0. In XUL, min-width: 0; means "use the intrinsic min width", but any other value will just override the intrinsic size, even if it's smaller.

This also came to a surprise to me, but it looks like we already make use of this quirk in several places:
> http://mxr.mozilla.org/mozilla-central/search?string=min-width%3A+1px&find=\.css%24

Details about the patch:

- nsCSSFrameConstructor.cpp, nsPresShell.cpp, nsContainerFrame.cpp:

Rename SyncFrameViewGeometryDependentProperties to SyncWindowProperties, remove the callers in SyncFrameViewAfterReflow and SyncFrameViewProperties, add direct calls in PresShell::DoReflow and nsCSSFrameConstructor::ConstructRootFrame.

  Only when called from these places, the conditions in SyncFrViewGeomDepProps
  were satisfied.

Pass a rendering context when calling from PresShell::DoReflow, so that we can create a BoxLayoutState, which is necessary to get the root frame's minimum / maximum size. Set the size constraints on the window widget.

- Menupopup files: Set the size constraints on the popup widget.

- nsResizerFrame.cpp:

Here I made several changes in order to address the resizer issues from comment 57 and 58.
  - Identity popup runs away: This was caused by the margin that's set on the
    panel. This causes the popup widget to end up in a different position than
    specified in popup->MoveTo.
    Partially resolved by adding a comment and not setting the position when it
    didn't change.
  - Only relative motions are respected: Resolved by adding mMouseDownRect and
    computing the new size relative to the size at mouse down, instead of the
    size at the last mouse move. This also simplifies the code a little.
  - Funny things happen when the window is resized to negative sizes: This bug
    was caused by integer overflow in nsResizerFrame::AdjustDimensions.
    Negative sizes were interpreted as very large, which led to funny results.
    Resolved by converting the values to PRInt64. Is there a better way?
  - Horizontal-only popup size changes weren't respected. This was caused by
    setting aNotify to false when setting the width attribute on the popup.
    Since the height hadn't changed, setting the height attribute didn't have
    any effect, so the width change wasn't propagated, either.
    Resolved by specifying oldRect.height == rect.height for aNotify when
    setting the width.

I think the rest of the changes are pretty straightforward. In the Windows implementation I took out all the changes that dealt with making chromehidden windows behave sanely, since I don't think they belong into this bug. I also put the changes in #ifndef WINCE blocks because the try server told me that WinCE doesn't know these things.

I'd love to hear your feedback, Neil.

Comment 75

[Markus Stange [:mstange]]

Tryserver builds are here: https://build.mozilla.org/tryserver-builds/[e-mail]-try-96f37740627e/
I can't test this on linux - can somebody verify that it works there?

Comment 76

[[e-mail]]

Comment on attachment 387743 [details] [diff] [review]
intrinsic size approach, v1

> void
> nsResizerFrame::AdjustDimensions(PRInt32* aPos, PRInt32* aSize,
>+                                 PRUint32 aMinSize, PRUint32 aMaxSize,
>                                  PRInt32 aMovement, PRInt8 aResizerDirection)
> {
>+  PRInt32 oldPos = *aPos, oldSize = *aSize;
>+
>   switch(aResizerDirection)
>   {
>     case -1: // only move the window when the direction is top and/or left
>       *aPos+= aMovement;
>     case 1: // falling through: the window is resized in both cases
>       *aSize+= aResizerDirection*aMovement;
>   }
(See below.)

>+  if (PRInt64(*aSize) < PRInt64(aMinSize) || PRInt64(*aSize) > PRInt64(aMaxSize)) {
>+    // Constrain the size within the minimum and maximum size.
>+    *aSize = PRInt32(PR_MAX(PRInt64(aMinSize), PR_MIN(PRInt64(aMaxSize), PRInt64(*aSize))));
If you don't want to use PRInt64, you could try [untested]:
if (*aSize < 0 || PRUint32(*aSize) < aMinSize)
  *aSize = aMinSize;
else if (PRUint32(*aSize) > aMaxSize)
  *aSize = aMaxSize;

>+    // For left and top resizers, the window must be moved left by the same
>+    // amount that the window was resized.
>+    if (aResizerDirection == -1) {
>+      *aPos = oldPos + oldSize - *aSize;
>+    }
Might be worth doing this all the time, rather than doing it first in the switch above and then possibly again here.

Comment 77

[Neil Deakin]

(In reply to comment #74)
> Putting back support for the window attributes wouldn't be hard, but I didn't do it for two reasons:

I'd rather it be done anyway. I'll look at the patch tomorrow.

Comment 78

[Neil Deakin]

I tested this out a bit and it looks to be working quite well. Some issues:

- when a minimum size is set, a <resizer> in the page will resize to that minimum, but the Mac corner resizer leaves a small gap around 7-10 pixels on the right side. This is a test with just a 100x100 pixel image in it (also with the example below).
- it would be nice if setting the minwidth attribute alone, (without setting minheight) would allow that minimum width without changing the minimum height. That might be beyond the scope here though.
- an odd thing happens in this case:

<window height="100" maxheight="100" maxwidth="300">
  <image src="happy.png" width="100 height="100"/>
  <button label="This is a very long label used for testing"/>
</window>

Resizing causes the window to jump between the height of 100 to the intrinsic size as the mouse is dragged.

The main issue is that width/height set the outer window size, yet this patch makes minimum and maximum sizes apply to the inner window size. Maybe what is really needed is separate attributes for the outer sizes.

> In XUL, min-width: 0; means "use the intrinsic min width"

We should fix that now that we have new keywords. Maybe min-width: min-content could be implemented for this. But that's beyond this bug.

Comment 79

[Neil Deakin]

+ content->SetAttr(kNameSpaceID_None, nsGkAtoms::width, widthstr, oldRect.height == rect.height);
+ content->SetAttr(kNameSpaceID_None, nsGkAtoms::height, heightstr, PR_TRUE);
...
+ popup->MoveTo(rect.x, rect.y, PR_TRUE);

I didn't look at the code too closely but wanted to make sure we remember to check 'popup' here, as it could have been deleted during SetAttr.

Comment 80

[Markus Stange [:mstange]]

(In reply to comment #78)
> - when a minimum size is set, a <resizer> in the page will resize to that
> minimum, but the Mac corner resizer leaves a small gap around 7-10 pixels on
> the right side. This is a test with just a 100x100 pixel image in it (also with
> the example below).

The Mac corner resizer seems to respect the controls in the window's titlebar. When the toolbar collapse button is shown that seems to add up to about 108px.

> - it would be nice if setting the minwidth attribute alone, (without setting
> minheight) would allow that minimum width without changing the minimum height.

That works for me with the following test case:

<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
        minwidth="200">
  <vbox>
    <image src="compose-toolbar.png" width="100" height="100"/>
  </vbox>
</window>

Minimum width is 200px and minimum height is the intrinsic 100px.

> - an odd thing happens in this case:
> 
> <window height="100" maxheight="100" maxwidth="300">
>   <image src="happy.png" width="100 height="100"/>
>   <button label="This is a very long label used for testing"/>
> </window>
> 
> Resizing causes the window to jump between the height of 100 to the intrinsic
> size as the mouse is dragged.

I'll look into this.

> The main issue is that width/height set the outer window size, yet this patch
> makes minimum and maximum sizes apply to the inner window size. Maybe what is
> really needed is separate attributes for the outer sizes.

Separate attributes would be good, yeah.

Comment 81

[Markus Stange [:mstange]]

(In reply to comment #78)
> Resizing causes the window to jump between the height of 100 to the intrinsic
> size as the mouse is dragged.

In this case we're setting the window's min-height to 154 and max-height to 122. This jumping seems to be what Cocoa does when these things contradict.

I think we should guarantee non-contradicting constraints for the nsIWidget::SetSizeConstraints API and change the callers to let the minimum size win over the maximum size.

Comment 82

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Any updates on this bug?

Comment 83

[Neil Deakin]

Attachment: updated patch

Not much news on this bug no. Here is an updated latest patch.

Comment 84

[Neil Deakin]

Attachment: part 2: tests

Still some work to do on the bug:
 - fix up the tests
 - clean up the areas with XXXndeakin comments

Comment 85

[Neil Deakin]

More specifically, I don't have plans currently to work on this myself, although if someone can volunteer to fix up the tests, I can finish the rest.

Comment 86

[Neil Deakin]

Attachment: Part 1 - support minimum and maximum size constraints on windows and popups

This patch should now work properly. Still some minor reftest failures occasionally on Mac similar to that described in bug 696746, but those aren't specifically this bug.

Comment 87

[Neil Deakin]

Attachment: Part 2 - tests

Comment 88

[Timothy Nikkel (:tnikkel)]

Your second hunk in nsMenuPopupFrame.cpp is going to get conflicts due to bug 668437 landing.

Comment 89

[Neil Deakin]

(In reply to Timothy Nikkel (:tn) from comment #88)
> Your second hunk in nsMenuPopupFrame.cpp is going to get conflicts due to
> bug 668437 landing.

Did you mean a different bug? The patches in that bug have been in for a while.

Comment 90

[Timothy Nikkel (:tnikkel)]

No, I have the bug number correct. The patch in this bug has code in it that was removed in bug 668437.

Comment 91

[[e-mail]]

Has the widget reorganisation bitrotted this patch severely or is it possible to fix up all the paths?

Comment 92

[Neil Deakin]

Comment on attachment 587882 [details] [diff] [review]
Part 1 - support minimum and maximum size constraints on windows and popups

Ah, now I see the confusion. I attached the wrong patch.

Comment 93

[Neil Deakin]

Attachment: Part 1 - support minimum and maximum size constraints on windows and popups

Comment 94

[Neil Deakin]

Attachment: Part 2 - tests

Comment 95

[Jim Mathies [:jimm]]

Comment on attachment 588240 [details] [diff] [review]
Part 1 - support minimum and maximum size constraints on windows and popups

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

r+ on the win widget parts.

::: widget/windows/nsWindow.cpp
@@ +5185,5 @@
> +      MINMAXINFO* mmi = (MINMAXINFO*)lParam;
> +      mmi->ptMinTrackSize.x = PR_MAX(mmi->ptMinTrackSize.x, mSizeConstraints.minWidth);
> +      mmi->ptMinTrackSize.y = PR_MAX(mmi->ptMinTrackSize.y, mSizeConstraints.minHeight);
> +      mmi->ptMaxTrackSize.x = PR_MIN(mmi->ptMaxTrackSize.x, mSizeConstraints.maxWidth);
> +      mmi->ptMaxTrackSize.y = PR_MIN(mmi->ptMaxTrackSize.y, mSizeConstraints.maxHeight);

This plays nicely with features like aero snap, with Windows restricting the size appropriately.

Comment 96

[[e-mail]]

Comment on attachment 588240 [details] [diff] [review]
Part 1 - support minimum and maximum size constraints on windows and popups

>+  // The sizes are in inner window sizes, so convert them into outer window sizes.
(This confused me briefly because I'm used to the height and width on the window being outer sizes...)

>+        sizeConstraints = widget->GetSizeConstraints();
This is why widget size constraints have to be in outer sizes?

>+  if (PRInt64(*aSize) < PRInt64(aMinSize) || PRInt64(*aSize) > PRInt64(aMaxSize)) {
Looks like mixing PRUint32 and PRInt32 is a bad idea. Any reason not to stick with PRInt32 throughout?

>+    *aSize = PRInt32(PR_MAX(PRInt64(aMinSize), PR_MIN(PRInt64(aMaxSize), PRInt64(*aSize))));
Aren't we supposed to use NS_Max and NS_Min?

>+struct SizeConstraints {
>+  PRUint32 minWidth;
>+  PRUint32 maxWidth;
>+  PRUint32 minHeight;
>+  PRUint32 maxHeight;
>+};
Or maybe nsIntSize minSize, maxSize?

Comment 97

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to [e-mail] from comment #96)
> Comment on attachment 588240 [details] [diff] [review]
> >+    *aSize = PRInt32(PR_MAX(PRInt64(aMinSize), PR_MIN(PRInt64(aMaxSize), PRInt64(*aSize))));
> Aren't we supposed to use NS_Max and NS_Min?

NS_MAX and NS_MIN, yes. In fact, this should use mozilla::clamped.

(In reply to Jim Mathies [:jimm] from comment #95)
> ::: widget/windows/nsWindow.cpp
> @@ +5185,5 @@
> > +      MINMAXINFO* mmi = (MINMAXINFO*)lParam;
> > +      mmi->ptMinTrackSize.x = PR_MAX(mmi->ptMinTrackSize.x, mSizeConstraints.minWidth);
> > +      mmi->ptMinTrackSize.y = PR_MAX(mmi->ptMinTrackSize.y, mSizeConstraints.minHeight);
> > +      mmi->ptMaxTrackSize.x = PR_MIN(mmi->ptMaxTrackSize.x, mSizeConstraints.maxWidth);
> > +      mmi->ptMaxTrackSize.y = PR_MIN(mmi->ptMaxTrackSize.y, mSizeConstraints.maxHeight);

NS_ here too.

Comment 98

[Neil Deakin]

(In reply to [e-mail] from comment #96)
> >+  if (PRInt64(*aSize) < PRInt64(aMinSize) || PRInt64(*aSize) > PRInt64(aMaxSize)) {
> Looks like mixing PRUint32 and PRInt32 is a bad idea. Any reason not to
> stick with PRInt32 throughout?
> 

Not sure. I thought about changing to 32-bit ints here. Mstange wrote this so he might have had some reason.

Comment 99

[Markus Stange [:mstange]]

(In reply to Neil Deakin from comment #98)
> (In reply to [e-mail] from comment #96)
> > >+  if (PRInt64(*aSize) < PRInt64(aMinSize) || PRInt64(*aSize) > PRInt64(aMaxSize)) {
> > Looks like mixing PRUint32 and PRInt32 is a bad idea. Any reason not to
> > stick with PRInt32 throughout?
> > 
> 
> Not sure. I thought about changing to 32-bit ints here. Mstange wrote this
> so he might have had some reason.

No reason, as far as I recall. I like the nsIntSize suggestion.

Comment 100

[Neil Deakin]

Attachment: Part 1.2 - support minimum and maximum size constraints on windows and popups

Comment 101

[[e-mail]]

Comment on attachment 591447 [details] [diff] [review]
Part 1.2 - support minimum and maximum size constraints on windows and popups

> void
> nsResizerFrame::AdjustDimensions(PRInt32* aPos, PRInt32* aSize,
>+                                 PRInt32 aMinSize, PRInt32 aMaxSize,
>                                  PRInt32 aMovement, PRInt8 aResizerDirection)
> {
>+  PRInt32 oldPos = *aPos, oldSize = *aSize;
>+
>   switch(aResizerDirection)
>   {
>     case -1:
>       // only move the window when the direction is top and/or left
>       *aPos+= aMovement;
>       // falling through: the window is resized in both cases
>     case 1:
>       *aSize+= aResizerDirection*aMovement;
>       // use one as a minimum size or the element could disappear
>       if (*aSize < 1)
>         *aSize = 1;
>   }
>+
>+  if (*aSize < aMinSize || *aSize > aMaxSize) {
>+    // Constrain the size within the minimum and maximum size.
>+    *aSize = NS_MAX(aMinSize, NS_MIN(aMaxSize, *aSize));
>+
>+    // For left and top resizers, the window must be moved left by the same
>+    // amount that the window was resized.
>+    if (aResizerDirection == -1) {
>+      *aPos = oldPos + oldSize - *aSize;
>+    }
>+  }
> }
Looking at this again, I can't convince myself that this works correctly in all edge cases. It might be better to rewrite it like this:
void
nsResizerFrame::AdjustDimensions(PRInt32* aPos, PRInt32* aSize,
                                 PRInt32 aMinSize, PRInt32 aMaxSize,
                                 PRInt32 aMovement, PRInt8 aResizerDirection)
  PRInt32 oldSize = *aSize;

  *aSize += aResizerDirection * aMovement;
  // use one as a minimum size or the element could disappear
  if (*aSize < 1)
    *aSize = 1;

  // Constrain the size within the minimum and maximum size.
  *aSize = NS_MAX(aMinSize, NS_MIN(aMaxSize, *aSize));

  // For left and top resizers, the window must be moved left by the same
  // amount that the window was resized.
  if (aResizerDirection == -1)
    *aPos += oldSize - *aSize;
}

>+struct SizeConstraints {
>+  SizeConstraints()
>+  {
>+    maxSize = nsIntSize(NS_MAXSIZE, NS_MAXSIZE);
>+  }
Does the following version work?
SizeConstraints()
: maxSize(NS_MAXSIZE, NS_MAXSIZE)
{
}

Comment 102

[Karl Tomlinson (:karlt)]

Comment on attachment 591447 [details] [diff] [review]
Part 1.2 - support minimum and maximum size constraints on windows and popups

>+void nsWindow::SetSizeConstraints(const SizeConstraints& aConstraints)
>+{
>+  GdkGeometry geometry;
>+  geometry.min_width = aConstraints.minSize.width;
>+  geometry.min_height = aConstraints.minSize.height;
>+  geometry.max_width = aConstraints.maxSize.width;
>+  geometry.max_height = aConstraints.maxSize.height;
>+
>+  PRUint32 hints = GDK_HINT_MIN_SIZE | GDK_HINT_MAX_SIZE;
>+  gtk_window_set_geometry_hints(GTK_WINDOW(mShell), GTK_WIDGET(mContainer),
>+                                &geometry, GdkWindowHints(hints));

I think its better to pass NULL for the geometry_widget parameter here.
Providing the geometry_widget requests GTK to do more work to ensure that the
geometry sizes apply to the geometry_widget rather than the GtkWindow, but
here we really mean the GtkWindow.  mContainer is the same size, but the
existence of mContainer is mostly a quirk of implementation.

mShell should be null-checked I think, in case the widget has been destroyed.
That will also protect against calling this on child windows.
nsBaseWidget::SetSizeConstraints() can still be called when mShell is NULL.

Please add to the documentation for Resize methods to clarify that their
parameters will be filtered according to size constraints.  This is helpful to
know that size constraints must be changed (if necessary) before Resize is
used.

Did you have a strong reason for choosing to apply the size constraints to the
window outer dimensions?  Was that to make the Resize methods simpler?
Setting the content dimensions of windows makes more sense to me, but I
haven't thought enough about it to feel strongly.  I'm mostly asking because
if you write the reasons down here, it may be helpful to refer to in the
future if anyone is considering changing the behavior.

We're getting lucky that this works with GTK and bug 581866.  That's because
ClientToWindowSize() returns the client size for that port.  (I'm not asking you to fix these.)

Comment 103

[Neil Deakin]

(In reply to Karl Tomlinson (:karlt) from comment #102)
> Did you have a strong reason for choosing to apply the size constraints to
> the
> window outer dimensions?  Was that to make the Resize methods simpler?

It is. I would think that the arguments to SetSizeConstraints should be in outer coordinates to match Resize() and mBounds. Within the UI, the author would use inner sizes though.

Comment 104

[Mats Palmgren (inactive)]

Comment on attachment 591447 [details] [diff] [review]
Part 1.2 - support minimum and maximum size constraints on windows and popups

Sorry for the late review.

> layout/generic/nsContainerFrame.h
>+  // Converts the minimum and maximum sizes given in inner window CSS pixel sizes to
>+  // outer window device pixel sizes and assigns these constraints to the widget.
>+  static void SetSizeConstraints(nsPresContext* aPresContext,
>+                                 nsIWidget* aWidget,
>+                                 const nsSize& aMinSize,
>+                                 const nsSize& aMaxSize);

This static method doesn't seem to have anything to do with the
nsContainerFrame class per se.  I think you should move it to
nsLayoutUtils.

Please make the comment into a proper doc comment, with @param etc.
The "given in inner window CSS pixel sizes" part is wrong - the given sizes
are in app units.


> layout/xul/base/src/nsResizerFrame.cpp
>-/* adjust the window position and size according to the mouse movement and
>- * the resizer direction
>+/* Adjust the window position and size according to the mouse movement and
>+ * the resizer direction. The minimum and maximum size is used to constrain
>+ * the size.
>  */
> void
> nsResizerFrame::AdjustDimensions(PRInt32* aPos, PRInt32* aSize,
>+                                 PRInt32 aMinSize, PRInt32 aMaxSize,
>                                  PRInt32 aMovement, PRInt8 aResizerDirection)

Please move the comment to the header and make it a doc comment.


>widget/cocoa/nsCocoaWindow.mm
>@@ -456,17 +456,16 @@ nsresult nsCocoaWindow::CreateNativeWind
>   [mWindow setBackgroundColor:[NSColor clearColor]];
>   [mWindow setOpaque:NO];
>-  [mWindow setContentMinSize:NSMakeSize(60, 60)];
>   [mWindow disableCursorRects];
> 

Hmm, can we just remove this builtin minsize? or should we apply it in 
SetSizeConstraints instead?  Please get feedback from josh or smichaud
that this change is ok.

> widget/nsIWidget.h
>+// Size constraints for setting the minimum and maximum size of a widget.
>+// Values are in device pixels.

Make it a proper doc comment please.

>+struct SizeConstraints {

Hmm, this is in global scope, right?
It seems like a too generic name for that.  I think it needs to go in
mozilla:: or nsIWidget:: or have a 'ns' prefix or something...

>+  SizeConstraints()
>+  {
>+    maxSize = nsIntSize(NS_MAXSIZE, NS_MAXSIZE);
>+  }

": maxSize(NS_MAXSIZE, NS_MAXSIZE)" should work

>+  nsIntSize minSize;
>+  nsIntSize maxSize;
>+};

Please make it mMinSize, mMaxSize.

>+     * @param aConstraints: the size constraints in device pixels
>+     **/
>+    virtual void SetSizeConstraints(const SizeConstraints& aConstraints) = 0;

nit: remove the extra *

>+     * @return the constraints in device pixels
>+     **/
>+    virtual SizeConstraints GetSizeConstraints() = 0;

nit: remove the extra *

This method can be 'const'.  Is there any reason the return type can't be
"const SizeConstraints&" ?


> widget/windows/nsWindow.cpp
>+void nsWindow::SetSizeConstraints(const SizeConstraints& aConstraints)

nit: newline after 'void' please

>+  SizeConstraints c = aConstraints;
>+  if (mWindowType != eWindowType_popup) {
>+    c.minSize.width = NS_MAX(PRInt32(::GetSystemMetrics(SM_CXMINTRACK)), c.minSize.width);
>+    c.minSize.height = NS_MAX(PRInt32(::GetSystemMetrics(SM_CYMINTRACK)), c.minSize.height);
>+  }
>+
>+  nsBaseWidget::SetSizeConstraints(c);

This affects what GetSizeConstraints() later returns, right?
It seems like it would affect nsResizerFrame even when no minimum size
(zero) is set.  Just want to make sure it's intentional...

> nsIntSize nsWindow::ClientToWindowSize(const nsIntSize& aClientSize)
> {
>-  if (!IsPopupWithTitleBar())
>+  if (mWindowType == eWindowType_popup && 
>+      (mBorderStyle == eBorderStyle_default ||
>+       !(mBorderStyle & eBorderStyle_title)))

Could you do it like this to avoid duplicating the details of
IsPopupWithTitleBar:
   if (mWindowType == eWindowType_popup && 
       !IsPopupWithTitleBar())

> widget/xpwidgets/nsBaseWidget.h
>+  // Apply the current size constraints to the given size.
>+  virtual void ConstrainSize(PRInt32& aWidth, PRInt32& aHeight);

Doc comment please.  This method should be 'const' and the parameters
should have PRInt32* type.  It doesn't seem like it needs to be 'virtual',
maybe just inline it in the header.

Comment 105

[Neil Deakin]

Comment on attachment 591447 [details] [diff] [review]
Part 1.2 - support minimum and maximum size constraints on windows and popups

Feedback requested on this, which mstange changed:

>widget/cocoa/nsCocoaWindow.mm
>@@ -456,17 +456,16 @@ nsresult nsCocoaWindow::CreateNativeWind
>   [mWindow setBackgroundColor:[NSColor clearColor]];
>   [mWindow setOpaque:NO];
>-  [mWindow setContentMinSize:NSMakeSize(60, 60)];
>   [mWindow disableCursorRects];
> 

> Hmm, can we just remove this builtin minsize? or should we apply it in 
> SetSizeConstraints instead?  Please get feedback from josh or smichaud
> that this change is ok.

On Windows, the minimum is done within SetSizeConstraints.

Is there a reason (60, 60) is the size?

Comment 106

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 591447 [details] [diff] [review]
Part 1.2 - support minimum and maximum size constraints on windows and popups

>-  [mWindow setContentMinSize:NSMakeSize(60, 60)];

This looks fine to me.  But we seriously do need to set a minimum size, however we accomplish that.

> Is there a reason (60, 60) is the size?

The -[NSWindow setContentMinSize:] call dates back to 2006, in Josh's fix for bug 338723.  In bug 338723 comment #2 he says 60x60 "is about what Apple recommends".

Comment 107

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #106)

Another thing:

The line mstange wants to remove sets a minimum size for *all* windows that we create -- not just top-level ones.  If SetSizeConstraints only works for top-level windows, we'll need to use other means to set a minimum size for other kinds of windows.

Comment 108

[Neil Deakin]

OK, I'll leave the setContentMinSize line in and add the same constraint the SetSizeConstraint as on Windows.

Comment 109

[Neil Deakin]

Attachment: Part 1.3 - support minimum and maximum size constraints on windows and popups

Comment 110

[Mats Palmgren (inactive)]

Comment on attachment 604152 [details] [diff] [review]
Part 1.3 - support minimum and maximum size constraints on windows and popups

The static method nsContainerFrame::SetSizeConstraints doesn't seem like
it belongs in the nsContainerFrame class, it's just a convenience function
that operates on a widget.  I would prefer if it were a local function
in nsContainerFrame.cpp or a static method in nsLayoutUtils.
I don't feel strongly about it though, r=mats as you see fit

Comment 111

[Karl Tomlinson (:karlt)]

Comment on attachment 604152 [details] [diff] [review]
Part 1.3 - support minimum and maximum size constraints on windows and popups

(I only really reviewed the GTK and XP widget changes so please re-request review if there is anything else you would like me to check.)

Comment 112

[Neil Deakin]

(In reply to Mats Palmgren [:mats] from comment #110)
> I would prefer if it were a local function
> in nsContainerFrame.cpp or a static method in nsLayoutUtils.

I'd prefer the former too, but the caller is static, so I didn't see a need to change it.

Comment 113

[Neil Deakin]

So this is done, but the same image and menu reftest failures occur with this patch as with bug 230959.

Comment 114

[Marco Aurélio]

Any news on this one? This bug has been assigned for almost six years. :(

Comment 115

[Neil Deakin]

Checked in to inbound!

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8a0228a10fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/28468ad2ffdc

Comment 116

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Just barely missed 1.9.1!

Comment 117

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b8a0228a10fa
https://hg.mozilla.org/mozilla-central/rev/28468ad2ffdc