Boris Zbarsky [:bzbarsky] Reporter
	Description • 20 years ago

This method has several issues:

1)  There are several different codepaths that should really be combined.
2)  The coordinate transformations being performed are far more complicated than
    necessary.
3)  The coordinate systems being used for the inputs to this function are
    undocumented as far as I can tell.

I'd like to aim at preventing things like bug 245163 coming up again.

I plan to try to do something about this, but if you know someone else
interested in doing it, please feel free to let them know about this bug.

Whatever you do, don't mention this bug in bug 245163 or add a dependency there.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 2 • 17 years ago

I'm not sure when, or whether I'll get to this... people should feel free to take it.

	Neil Deakin Assignee
	Comment 4 • 16 years ago

This patch cleans up the code, but shouldn't be making any functional changes. There are already various popup positioning tests and they still work with this patch.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 5 • 16 years ago

Comment on attachment 350779 [details] [diff] [review]
updated patch

I'll try to get to this sometime in the next week or so, but if roc gets here first that's good too...

Does it make sense to leave some ASCII art like what used to be there explaining the new setup?

If you have any advice on the right place to start reading the patch to get a hang for what the new setup is, that would be nice.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 6 • 16 years ago

+nsMenuPopupFrame::AdjustPositionForAnchorAlign(nsRect anchorRect,

const nsRect&

+                                               PRBool& hFlip, PRBool& vFlip)

We should be using the a* convention for parameter names here.

   PRInt8 popupAnchor(mPopupAnchor);
   PRInt8 popupAlign(mPopupAlignment);

Why is this needed?

I think AdjustPositionForAnchorAlign be a bit more logical if it first computed the anchor point by inflating aAnchorRect by the margin and switching on mPopupAnchor to choose the right corner, and then computed the popup origin by switching on mPopupAlignment and incorporating margins.

The rest looks good although I need to rereview for details.

	Neil Deakin Assignee
	Comment 7 • 16 years ago

I notice also that the right/bottom margins should really be subtracted from the position, not added, so fixing that bug as well.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 8 • 16 years ago

AdjustPositionForAnchorAlign looks so much nicer now!

+nsMenuPopupFrame::AdjustPositionForAnchorAlign(nsRect anchorRect,

const nsRect&

+  // screenX and screenY hold the screen position in app units where the popup
+  // should appear
+  PRInt32 screenX, screenY;

These should be nscoords. But why not make them just "nsPoint screenPt;"? Then you can simplify later code, e.g.

+      screenX = anchorRect.x + margin.left;
+      screenY = anchorRect.y + margin.top;

screenPt = anchorRect.TopLeft() + nsPoint(margin.left, margin.top)

Can we share the code that does resizing and flipping? We basically have two copies of the code, one for each axis. Maybe we can have a helper function that computes the resize or flip along a single axis.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 10 • 16 years ago

In AdjustPositionForAnchorAlign, would it make sense to just deflate anchorRect by margin?  It looks like that's what you effectively do before adjusting for mRect.

	Neil Deakin Assignee
	Comment 11 • 16 years ago

(In reply to comment #10)
> In AdjustPositionForAnchorAlign, would it make sense to just deflate anchorRect
> by margin?  It looks like that's what you effectively do before adjusting for
> mRect.

The margin is only applied based on the popupAlign. We only want the margin applied to the corner of the popup it is aligned with not all sides.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 12 • 16 years ago

But the only thing anchorRect (which should perhaps be const or just not a reference) is used for in AdjustPositionForAnchorAlign is to compute pnt, and for that purpose we only use one corner of the rect, right?

	Neil Deakin Assignee
	Comment 13 • 16 years ago

(In reply to comment #12)
> But the only thing anchorRect (which should perhaps be const or just not a
> reference) is used for in AdjustPositionForAnchorAlign is to compute pnt, and
> for that purpose we only use one corner of the rect, right?

Yes, but sometimes in the opposite direction. For example, for the popup and anchor here, the right margin pushes the popup left and the top margin pushes the popup down.

 ppaa
 ppaa
   aa

but in this case:

 pp
 pp
 aaa
 aaa

The same corner but the bottom margin pushes the popup up and the left margin pushes the popup to the right.

Essentially, sometimes Inflate needs to be used and sometimes Deflate. Maybe there's some way of writing this better but I don't think it's problematic as is.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 14 • 16 years ago

Um... deflate will make right margins push left, top margins push down, bottom margins push up, and left margins push right.  That's consistent with your two examples (and with the code as written right now, I think).

It's not a big deal; it just seemed like another simplification.

	Neil Deakin Assignee
	Comment 15 • 16 years ago

Maybe I'm misunderstanding something here. I originally had used Deflate but it makes the new tests fail.

I assume you meant to use anchorRect.Deflate(margin) near the beginning of the method.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 16 • 16 years ago

Doing that as-is will modify the rect in the caller, since it's passed by non-const reference.  Maybe that was the problem?  If it were passed by value, I do mean anchorRect.Deflate(margin) and then adjusting the second switch to only do the width/height adjustments.

	Walter Meinl
	Comment 17 • 16 years ago

Neil, after checking this in, I see a difference in submenus popping up at the left side of a main menu. They used to be aligned side by side to the main menu and now they overlap by a few pixels (which is of course the behavior for submenus popping up on the right side of the main menu). This is on windows and OS/2, I don't see a difference on linux.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 18 • 16 years ago

For reference: http://hg.mozilla.org/mozilla-central/rev/d3210f7e5a12

	Shawn Wilsher :sdwilsh
	Comment 19 • 16 years ago

How safe would this be for 1.9.1?  We'd kinda like to get bug 342619 on branch, but it'd need this bug.