Closed Bug 798226 Opened 7 years ago Closed 7 years ago

Resizing a panel while it is open may move the arrow away from the anchor

Categories

(Core :: XUL, defect)

16 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: markh, Assigned: markh)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 18 obsolete files)

9.53 KB, patch
neil
: review+
Details | Diff | Splinter Review
If you have a popup showing with the anchor position on the right, then use the sizeTo() method, the right-hand edge of the panel moves to reflect the new size.  As the anchor arrow is on the right, the arrow is no longer aligned with the anchor.

Somewhat related to bug 793157 and the root cause for bug 798212
Blocks: 799014
Hey Neil,

Gavin said you might have an idea of what's going on here. Any clue?

-Mike
Attached patch Rework arrow positon computation (obsolete) — Splinter Review
This patch reworks the arrow positioning code to use the position passed into 'openPopup' as well as the resulting flip state.

It doesn't handle all situations yet, but it would be good to get some feedback if this patch fixes this bug as well as the dependent bugs.

Try builds are at https://tbpl.mozilla.org/?tree=Try&rev=d7787295961e
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Attachment #673383 - Flags: feedback?(mhammond)
Flags: needinfo?(enndeakin)
Assuming the Downloads Panel bug is the same one that Mark originally reported, I don't believe this patch has fixed the problem.

STR for Downloads Panel (Linux only for now, since we only do the resize there):

1) Start a download.
2) Cancel the download.
3) Right click on the panel, and choose to "Clear List"
4) Click on the Downloads button to close the panel.
5) Click on the Downloads button again to re-open the panel.

What happens?

I still get this:

https://bugzilla.mozilla.org/attachment.cgi?id=667646
Neil:

Apply this patch, and then initiate a download so that the panel appears.

Then right click on the download panel, and choose to clear the list. You should see the bug then.

-Mike
Attached patch arrow tests (obsolete) — Splinter Review
I'm seeing the same as Mike for a simple resize.  Also, the flipping etc seems to be wrong - I'm seeing the anchor flip in the wrong direction in some cases.

I've tried to make a test case for this stuff but it is currently very messy :(

* After the popupshown event I'm seeing the arrow widget is still moving towards its final position and size (which I guess is the arrow fading in or similar) - but this obviously makes it hard to test.  There is a very very hacky waitForStableSize() function - not sure what can be done about that.

* The tests aren't complete, but they do demonstrate the problem with a simple move as Mike's test shows, and also shows the flipping being wrong.

I've run out of time today and it's probably not worth spending more time unless this test looks like it could be checked in and used (once it is in a reasonable shape, of course).  FWIW, I've been testing with the "Rework arrow positon computation" attachment applied.
When I debug this I see the popup being resized and positioned by nsMenuPopupFrame correctly. When it opens and displays incorrectly, I see:

- calls to nsWindow::Resize and nsWindow::Move which move the shrunk popup into the new place with the arguments having the correct expected values. In my test, this position is (724, 135)
- the popup is opened and the the popup positioned. The coordinates look correct, although it is now (723, 135). Perhaps some margin got added?
- call to nsWindow::Show to show the window. mBounds is set to (724, 135, 155, 53) which is correct.
- OnConfigureEvent is called with *incorrect* event values: (357, 135, 155, 53). The '357' value is what the old x-position was before the popup was resized.

So, there is an extra OnConfigureEvent event being sent with incorrect values. It's interesting that the size is being changed while the popup is closed though. Maybe gtk doesn't like that happening?
Not sure what's happening here.
I'm on Windows, so I don't think I'm seeing the Gtk issue.

The key problem is that the patch doesn't handle the case when the panel is already open - the calculations for the position of the arrow only happens in popupshown.

This patch just takes Neil's patch and moves the arrow calc code into a new method, then arranges to call that new method whenever sizeTo, moveTo or moveToAnchor is called.  This addresses almost all issues I can see.
Attached patch new and improved tests (obsolete) — Splinter Review
Almost all tests now pass when my other patch is applied.  The tests also found bug 806316 (and has a work-around for it) - it might be worth an explicit test for that bug if the tests can be checked-in and used.

The only test failures I see are probably related to the fact the tests are mochitest-plain, so the panel seems to be restricted to the app window (whereas I guess a browser-chrome test would allow the panels to go outside the borders when necessary).  However, one of the failing tests fails to move the arrow at all when it probably should be (even if the actual position differs from the requested position due to it potentially moving outside the window).

At the risk of telling people how to suck eggs, the test is run via:

% TEST_PATH=toolkit/content/tests/widgets/test_popupanchor.xul make -C {obj-dir} mochitest-plain

There is a 'debugging' variable which is set to true, which causes things to go slow enough to see the actual sizing happen.
Attachment #676925 - Attachment is obsolete: true
clarifying the summary to reflect the actual problem.
Summary: Resizing a panel may move it away from the anchor → Resizing a panel while it is open may move the arrow away from the anchor
Mark, your testcase is using noautohide="true". Is that the case with the popup you originally filed this bug on? The gtk issue does not use noautohide="true".

It would be helpful to understand what specific panels are at issue here. Panels, anchoring and popup placement have a number of complex interactions, some of which are platform-specific.

Your patch also only handles when the popup is moved by calling one of the methods, and doesn't handle when the popup is moved by other means, for example the window or anchor moving. If you are in fact using a noautohide popup, this will be important.
(In reply to Neil Deakin from comment #6)
> When I debug this I see the popup being resized and positioned by
> nsMenuPopupFrame correctly. When it opens and displays incorrectly, I see:
> 
> - calls to nsWindow::Resize and nsWindow::Move which move the shrunk popup
> into the new place with the arguments having the correct expected values. In
> my test, this position is (724, 135)
> - the popup is opened and the the popup positioned. The coordinates look
> correct, although it is now (723, 135). Perhaps some margin got added?
> - call to nsWindow::Show to show the window. mBounds is set to (724, 135,
> 155, 53) which is correct.
> - OnConfigureEvent is called with *incorrect* event values: (357, 135, 155,
> 53). The '357' value is what the old x-position was before the popup was
> resized.
> 
> So, there is an extra OnConfigureEvent event being sent with incorrect
> values. It's interesting that the size is being changed while the popup is
> closed though. Maybe gtk doesn't like that happening?
> ...
> Not sure what's happening here.

Any ideas, Karl?
Flags: needinfo?(karlt)
(In reply to Neil Deakin from comment #6)
> - calls to nsWindow::Resize and nsWindow::Move which move the shrunk popup
> into the new place with the arguments having the correct expected values. In
> my test, this position is (724, 135)
> [...]
> - OnConfigureEvent is called with *incorrect* event values: (357, 135, 155,
> 53). The '357' value is what the old x-position was before the popup was
> resized.
> 
> So, there is an extra OnConfigureEvent event being sent with incorrect
> values.

Is mListenForResizes set on the nsWindow?
If so, the Resize will be effected immediately (instead of delayed until just before NativeShow).  That would make the Resize happen before the Move.
Did that Resize change the position or only the size?

If that Resize only changed the size and mListenForResizes is set then I'd expect a configure-event signal first in response to the Resize and then another in response to the Move.  Is there a subsequent OnConfigureEvent with the correct values?
Flags: needinfo?(karlt)
A log with NSPR_LOG_MODULES=Widget:5 may be helpful.
Attached file NSPR_LOG_MODULES=Widget:5 (obsolete) —
Here's a log for the following actions:

1) Open browser
2) Type URL for a download in the URL bar
3) Press enter
4) Accept to save the download
5) Click on the download button
6) Stop the download
7) Right click on the download item, and choose to clear the list
8) Click on the download button again to hide the panel
9) Click on the download button again to show the panel (here's where the problem presents itself)
10) Shut down browser.
Attachment #677590 - Attachment mime type: text/x-log → text/plain
(In reply to Neil Deakin from comment #11)
> Mark, your testcase is using noautohide="true". Is that the case with the
> popup you originally filed this bug on? The gtk issue does not use
> noautohide="true".

I get the exact same results from the tests both with and without this flag.  The panels in question do *not* use the flag.

> Your patch also only handles when the popup is moved by calling one of the
> methods, and doesn't handle when the popup is moved by other means, for
> example the window or anchor moving.

Right - and this is the use-case we care about (ie, an explicit move/resize while the panel is showing.)  I'm guessing from comment 3 ("Linux only for now, since we only do the resize there") that the same basic issue applies to the downloads panel too.

IIUC though, your patch doesn't handle *any* moves or resizes while it is open, right?  To handle that, we'd need (a) that patch of mine applied and (b) to ensure the function to place the arrow is called in all cases which could resize.  So maybe we could do this in 2 stages - handle the explicit calls first and handle the implicit moves later.  At least that gives us the correct handling for *some* use cases - not all use-cases, but better than no use-cases :)  And it sounds like it would cover all use-cases mentioned in this bug.
I haven't really managed to match up the STR in comment 16 with the log.
Truncating the log at the point after the problem happens would help, but I'm not seeing anything wrong here.
There are the only Move() calls in the log, but I doubt it corresponds to 9, because it is too soon after the window is created.  This is also the only place I saw inconsistent configure event coords.  Are you sure you are doing the same thing that Neil was doing?

nsWindow [a0191900]
	mShell 9deff880 mContainer a8fafd80 mGdkWindow 9dd0e110 0x1c01103
GetScreenBounds 0,0 | 1x1
nsWindow::Move [a0191900] 65 24
nsWindow::SetSizeMode [a0191900] 0
###!!! ASSERTION: must have binding parent when in native anonymous subtree with a parent node: '!IsInNativeAnonymousSubtree() || GetBindingParent() || !GetParent()', file ../../../dist/include/nsIContent.h, line 236
nsWindow [ab284500]
	mShell aaea1380 mContainer aae34740 mGdkWindow 9dd0e1c0 0x1c01121
WARNING: OpenGL-accelerated layers are not supported on this system.: file /media/Projects/mozilla/fx-team/widget/xpwidgets/nsBaseWidget.cpp, line 812
WARNING: GetDefaultCharsetForLocale: need to add multi locale support: file /media/Projects/mozilla/fx-team/intl/locale/src/unix/nsUNIXCharset.cpp, line 105
GetScreenBounds 0,0 | 1x1
nsWindow::SetSizeMode [a0191900] 0
nsWindow::Destroy [ab284500]
nsWindow::~nsWindow() [ab284500]
nsWindow [ab285800]
	mShell aaea1380 mContainer 9db6ec90 mGdkWindow a18c63d0 0x1c0113b
nsWindow::SetSizeMode [a0191900] 0
GetScreenBounds 65,24 | 1807x1111
GetScreenBounds 0,0 | 499x344
nsWindow::Move [a0191900] 719 407
nsWindow::SetSizeMode [a0191900] 0
GetScreenBounds 0,0 | 499x344
GetScreenBounds 65,24 | 1807x1111
nsWindow::Show [a0191900] state 1
nsWindow::NativeResize [a0191900] 499 344
size_allocate [a0191900] 0 0 499 344
nsWindow::OnWindowStateEvent [a0191900] changed 1 new_window_state 0
nsWindow::OnWindowStateEvent [a0191900] changed 1 new_window_state 0
configure event [a0191900] 720 435 499 344
GetScreenBounds 719,407 | 499x344
configure event [a0191900] 719 407 499 344
GetScreenBounds 719,407 | 499x344
configure event [a0191900] 720 435 499 344
GetScreenBounds 719,407 | 499x344
configure event [a0191900] 720 435 499 344
GetScreenBounds 719,407 | 499x344

But the configure event coords don't matter because they are never used.  It is the GetScreenBounds numbers that are used, and they look right.
Hm - I'm also finding it difficult to map the steps that I gave with the log.

So here are a couple of things that might help.

First, here's a link to a screencapture of me demonstrating the problem, with the log being dumped simultaneously:

http://youtu.be/ZK66P3pC6lY

I'll attach the log next.

I'll also try to recreate the bug, and just capture the last 50 or so lines in the terminal, and attach those as well.

Does any of that help?
Is there anything else I can provide that would help?
Flags: needinfo?(karlt)
It's a bit confusing this bug, and the confusion began partly at comment 3 because the 'downloads panel bug' referred to isn't the same bug and partly because the original report has no testcase nor any steps to reproduce.

So, a separate bug should be filed on the downloads panel Linux issue.
The duplicate bug 797547 is reopened for the linux issue.
(In reply to Mark Hammond (:markh) from comment #17)
> IIUC though, your patch doesn't handle *any* moves or resizes while it is
> open, right?  To handle that, we'd need (a) that patch of mine applied and
> (b) to ensure the function to place the arrow is called in all cases which
> could resize.  So maybe we could do this in 2 stages - handle the explicit
> calls first and handle the implicit moves later.  At least that gives us the
> correct handling for *some* use cases - not all use-cases, but better than
> no use-cases :)  And it sounds like it would cover all use-cases mentioned
> in this bug.

That's ok. We may want to eventually just have a move/resize event fire on the popup so this can be handled more easily.
(In reply to Neil Deakin from comment #23)
> It's a bit confusing this bug, and the confusion began partly at comment 3
> because the 'downloads panel bug' referred to isn't the same bug and partly
> because the original report has no testcase nor any steps to reproduce.
> 
> So, a separate bug should be filed on the downloads panel Linux issue.

I apologize - I was pretty certain these were the same bugs. They sounded the same on paper, anyhow. :)
Flags: needinfo?(karlt)
Comment on attachment 673383 [details] [diff] [review]
Rework arrow positon computation

This was showing up in my pending requests.  f- not due to the code but simply as it doesn't address the specific problem in this bug.

Any ideas on when we might get further updates?
Attachment #673383 - Flags: feedback?(mhammond) → feedback-
(In reply to Mark Hammond (:markh) from comment #27)
> Any ideas on when we might get further updates?

I was expecting you to finish off your patch.
(In reply to Neil Deakin from comment #28)
> (In reply to Mark Hammond (:markh) from comment #27)
> > Any ideas on when we might get further updates?
> 
> I was expecting you to finish off your patch.

Sorry for the confusion, but I'm not really clear on the current status.  You are happy with my patch even though it doesn't handle the popup being moved by other means?  So really it is just the test that needs work?
Since you aren't using noautohide, your patch should cover most cases that are relevant to you, so the patch is fine for this bug (with review of course). If you wanted to fix this in general for all panels though, it would need to support when the panel was resized via other means (window moving, a resizer in the panel, etc...)
Comment on attachment 673383 [details] [diff] [review]
Rework arrow positon computation

Changing to f+ after clarifying the scope of the changes for this bug.
Attachment #673383 - Flags: feedback- → feedback+
Attached patch fixes and tests (obsolete) — Splinter Review
Attaching a new version of my patch which is on-top of Neil's.  This patch:

* Removes the printf()

* Fixes a bug where moveToAnchor ignores the offset values if the anchor needs to be flipped.

* My earlier patch which splits our the arrow positioning code in popup.xml and arranges to call it in the various move/resize functions on an arrow panel.

* Fairly comprehensive tests, including for the moveToAnchor issue above and has "todo" tests for bug 812943.

It seems to work well for me.
Attachment #677294 - Attachment is obsolete: true
Attachment #677295 - Attachment is obsolete: true
Attachment #682998 - Flags: feedback?(enndeakin)
Comment on attachment 682998 [details] [diff] [review]
fixes and tests

The tests don't pass when I run this. I assume you are still working on this?

>+    // should be added to the position.  We also add the offset to the anchor
>+    // pos so a later flip/resize takes the offset into account.
>+    nscoord anchorOffset = presContext->CSSPixelsToAppUnits(mXPos);
>+    if (IsDirectionRTL()) {
>+      screenPoint.x -= anchorOffset;
>+      anchorRect.x -= anchorOffset;
>+    } else {
>+      screenPoint.x += anchorOffset;
>+      anchorRect.x += anchorOffset;
>+    }
>+     screenPoint.y += presContext->CSSPixelsToAppUnits(mYPos);

This only handles horizontal flipping.

> +  var arrowMidX = (arrowRect.left + arrowRect.right) / 2;

You can also just use arrowRect.width

> +function isArrowOffsetFromAnchor(offset, side) {

I'm not clear what this function is doing. Can you explain why the middle of the arrow is relevant? None of the actual positioning arrow panel code uses the middle of the arrow in the computations.  

> +      isArrowOffsetFromAnchor(0, "top");
> +      is(arrow.getBoundingClientRect().left, origArrowRect.left, "panel should not have moved horizontally");
> +      isArrowPositionedOn("top"); // check it flipped.

It's a bit confusing here as the lines are in a different order in each test, making it a bit harder to compare.


> +      var offset = Math.floor(-anchorLeft / 2);

I'm not sure what this is doing. If the anchor was changed to be really large, the popup won't be flipped. Shouldn't this account for the size of the anchor and available space?

> +  var testIter = Iterator(tests);

This doesn't run the tests in any particular order does it? Does that matter?
(In reply to Neil Deakin from comment #33)
> Comment on attachment 682998 [details] [diff] [review]
> fixes and tests
> 
> The tests don't pass when I run this. I assume you are still working on this?

I wasn't :)  They all work for me - what failures do you see?  I'm guessing we might still need to wait for the final size of the panels after resizing?

> 
> >+    // should be added to the position.  We also add the offset to the anchor
> >+    // pos so a later flip/resize takes the offset into account.
> >+    nscoord anchorOffset = presContext->CSSPixelsToAppUnits(mXPos);
> >+    if (IsDirectionRTL()) {
> >+      screenPoint.x -= anchorOffset;
> >+      anchorRect.x -= anchorOffset;
> >+    } else {
> >+      screenPoint.x += anchorOffset;
> >+      anchorRect.x += anchorOffset;
> >+    }
> >+     screenPoint.y += presContext->CSSPixelsToAppUnits(mYPos);
> 
> This only handles horizontal flipping.

Doh.  I obviously need to fix the vertical test case for this.

> > +  var arrowMidX = (arrowRect.left + arrowRect.right) / 2;
> 
> You can also just use arrowRect.width

I need the mid-point of the arrow - so using width I'd still need something like arrowRect.left + (arrowRect.width/2), right?  That doesn't seem a huge amount better - but I can change it if you feel strongly.

> > +function isArrowOffsetFromAnchor(offset, side) {
> 
> I'm not clear what this function is doing. Can you explain why the middle of
> the arrow is relevant? None of the actual positioning arrow panel code uses
> the middle of the arrow in the computations.

The idea is that I want to check the arrow is in the correct place.  When the panel is flipped, the arrow moves from pointing to the start of the element to the end (or vice-versa) and +- the offset.  Hence, in the flip tests I can't simply test the arrow hasn't moved (which I can do when no flipping takes place)

How do you suggest I check for this?

> > +      isArrowOffsetFromAnchor(0, "top");
> > +      is(arrow.getBoundingClientRect().left, origArrowRect.left, "panel should not have moved horizontally");
> > +      isArrowPositionedOn("top"); // check it flipped.
> 
> It's a bit confusing here as the lines are in a different order in each
> test, making it a bit harder to compare.

Sure, I can fix that.

> > +      var offset = Math.floor(-anchorLeft / 2);
> 
> I'm not sure what this is doing. If the anchor was changed to be really
> large, the popup won't be flipped. Shouldn't this account for the size of
> the anchor and available space?

It is simply calculating an offset which forces the panel to be flipped to fit.  An offset to the left of 1/2 the x-position of the anchor achieves this.  I'll see if I can make something similar or add a comment.

> > +  var testIter = Iterator(tests);
> 
> This doesn't run the tests in any particular order does it? Does that matter?

Actually, it does seem to run them in the order they are specified in the source, which actually surprises me - but the tests do not rely on that (we take care to reset the panel to a known state between each test.
Attached patch updated (obsolete) — Splinter Review
This includes the fix for the y offset and also simplifies the tests drastically - rather than "remembering" the arrow positions and comparing, we just work out where it should be and compare it to that.

It would be great if you can tell me the test failures you see, but otherwise I'll push this to try tomorrow and see what breaks there.
Attachment #682998 - Attachment is obsolete: true
Attachment #682998 - Flags: feedback?(enndeakin)
Attachment #683461 - Flags: feedback?(enndeakin)
Updates so the tests work on all platforms.  The key issues were:

* The pixel values were often off by < 1 pixel (ie, a fraction of a pixel) - so now any difference of < 1 pixel is treated as OK.

* The code for checking the flip direction seems to always think the arrow is exactly in the middle of the panel, which seems kinda strange, but I worked around this by allowing the middle to be treated as OK - see comments in the bug.
Attachment #683461 - Attachment is obsolete: true
Attachment #683461 - Flags: feedback?(enndeakin)
Attachment #684283 - Flags: feedback?(enndeakin)
Is there anything else you need from me here?
I still get one failure (within the subtest flippingResizeHorizontal):

 arrow should be 0px from left side of anchor: 6.149993896484375 should be equal to 0

> * The pixel values were often off by < 1 pixel (ie, a fraction of a pixel) - so now any difference of < 1 pixel is treated as OK.

You should also be able to just round both values, which is what other popup tests do.

> It is simply calculating an offset which forces the panel to be flipped to fit.  An offset to the left of 1/2 the x-position of the anchor achieves this.  I'll see if I can make something similar or add a comment.

This only works coincidentally though because the anchor isn't very wide. If you made the anchor 500 pixels wide, it would not flip. This is why this block is confusing because you're relying on positions and sizes that aren't used in determining whether to flip or not. Either change it to factor in the anchor width or add a comment which clarifies exactly what's going on here.

> +    }
> +	nscoord anchorYOffset = presContext->CSSPixelsToAppUnits(mYPos);
> +    screenPoint.y += anchorYOffset;
> +	anchorRect.y += anchorYOffset;

The indenting isn't correct here.
> You should also be able to just round both values, which is what other popup tests do.

Actually some tests are rounding and/or doing 'Math.abs(a - b) <= 0.5' so I would suggest using 0.5 instead.
Attached patch updated based on comments (obsolete) — Splinter Review
> I still get one failure (within the subtest flippingResizeHorizontal):

Hrmph - I possibly forgot to upload a version with that fixed.  I've a clean try run at https://tbpl.mozilla.org/?tree=Try&rev=956293cbf90d

> > * The pixel values were often off by < 1 pixel (ie, a fraction of a pixel)
> > so now any difference of < 1 pixel is treated as OK.

> You should also be able to just round both values, which is what other 
> popup tests do.

I've stick with checking for a full pixel out as I did get a couple of test failures moving to 0.5 and didn't dig too deep into it - but < 1 pixel sounds reasonable at face value and may just be an artifact of the tests doing things differently than the code - which itself seems a good thing!

> This only works coincidentally though because the anchor 
> isn't very wide. If you made the anchor 500 pixels wide, 
> it would not flip. This is why this block is confusing 
> because you're relying on positions and sizes that aren't 
> used in determining whether to flip or not. Either change
> it to factor in the anchor width or add a comment 
> which clarifies exactly what's going on here.

I changed it to use the "far side" of the anchor, so the width should be accounted for.  I also clarified (hopefully!) the comments.

> The indenting isn't correct here.

Doh - mixed tabs and spaces :(  Fixed.
Attachment #684283 - Attachment is obsolete: true
Attachment #684283 - Flags: feedback?(enndeakin)
Attachment #687625 - Flags: feedback?(enndeakin)
Attachment #687625 - Flags: feedback?(enndeakin) → feedback+
>    // Size the to such that it only just fits from the left-hand side of

Fix: Size the *panel* such that...
Attached patch Fixed comment (obsolete) — Splinter Review
Fixed the comment, so I hope this is looking good and we can get review on the actual patch that does the real work too.
Attachment #687625 - Attachment is obsolete: true
Attachment #688578 - Flags: review?(enndeakin)
Remove printf and change uuid.
Attachment #673383 - Attachment is obsolete: true
Attachment #688782 - Flags: review?(neil)
Attachment #688578 - Flags: review?(enndeakin) → review+
Comment on attachment 688782 [details] [diff] [review]
Rework arrow positon computation, v2

Just a quick skim though:

>@@ -545,16 +547,17 @@ nsMenuPopupFrame::InitializePopup(nsICon
>+  mPosition = POPUPPOSITION_UNKNOWN;
Not sure why you're setting this here on its own. I did notice that you don't set it in the constructor though, which is where I was expecting it to be set.

>+      mPosition = POPUPPOSITION_UNKNOWN;
>       InitPositionFromAnchorAlign(anchor, align);
Already sets the position, no?

>+void
>+nsMenuPopupFrame::GetAlignmentPosition(int8_t* aPosition, int8_t* aAnchor, int8_t* aAlignment,
>+                                       bool* aHFlip, bool* aVFlip) const
Grabbing 5 variables like this looks ugly. Perhaps GetAlignmentPosition() could returns a numeric position code which the box object translates into a string?

>+#define POPUPPOSITION_UNKNOWN 0
>+#define POPUPPOSITION_BEFORESTART 1
>+#define POPUPPOSITION_BEFOREEND -1
>+#define POPUPPOSITION_AFTERSTART 2
>+#define POPUPPOSITION_AFTEREND -2
>+#define POPUPPOSITION_STARTBEFORE 4
>+#define POPUPPOSITION_ENDBEFORE -4
>+#define POPUPPOSITION_STARTAFTER 8
>+#define POPUPPOSITION_ENDAFTER -8
>+#define POPUPPOSITION_OVERLAP 16
>+#define POPUPPOSITION_AFTERPOINTER 17
>+
>+// True if this is one of the 'before' positions. This indicates that we
>+// must switch to one of the 'after' positions when flipping the position
>+// vertically by shifting the value up 1. Otherwise, shift the value down 1.
>+#define POPUPPOSITION_ISBEFORE(v) ((v > 0 ? v : -v) & (POPUPPOSITION_BEFORESTART | POPUPPOSITION_STARTBEFORE))
As I understand it, the only special meaning of the codes is that it should be easy to generate horizontally and vertically flipped versions of them. While your scheme makes horizontal flipping easy, vertical flipping is ugly. I've actually thought of two alternative schemes for your consideration:
a)
#define POPUPPOSITION_UNKNOWN 0
#define POPUPPOSITION_BEFORESTART 1
#define POPUPPOSITION_BEFOREEND -1
#define POPUPPOSITION_STARTBEFORE 2
#define POPUPPOSITION_ENDBEFORE -2
#define POPUPPOSITION_AFTERSTART 4
#define POPUPPOSITION_AFTEREND -4
#define POPUPPOSITION_STARTAFTER 8
#define POPUPPOSITION_ENDAFTER -8
#define POPUPPOSITION_OVERLAP 16
#define POPUPPOSITION_AFTERPOINTER 17
  if (vFlip) {
    if (ENDBEFORE <= position && position <= STARTBEFORE)
      position <<= 2;
    else
      position >>= 2;
  }
b)
#define POPUPPOSITION_UNKNOWN -1
#define POPUPPOSITION_BEFORESTART 0
#define POPUPPOSITION_BEFOREEND 1
#define POPUPPOSITION_AFTERSTART 2
#define POPUPPOSITION_AFTEREND 3
#define POPUPPOSITION_STARTBEFORE 4
#define POPUPPOSITION_ENDBEFORE 5
#define POPUPPOSITION_STARTAFTER 6
#define POPUPPOSITION_ENDAFTER 7
#define POPUPPOSITION_OVERLAP 8
#define POPUPPOSITION_AFTERPOINTER 9
  if (hFlip)
    position ^= 1;
  if (vFlip)
    position ^= 2;
No good names for those XOR constants sorry.

>-        var container = document.getAnonymousElementByAttribute(this, "anonid", "container");
>-        var arrowbox = document.getAnonymousElementByAttribute(this, "anonid", "arrowbox");
>-        var arrow = document.getAnonymousElementByAttribute(this, "anonid", "arrow");
>-
>         var anchor = this.anchorNode;
>         if (!anchor) {
>           arrow.hidden = true;
This change looks wrong.

>+        if (position.indexOf("start_") == 0 || position.indexOf("end_") == 0) {
Good news! We now have .startsWith (and .endsWith) primitives :-)

>-            // In RTL, everything should be inverted.
So, what happens in RTL with the new code?

>+        var arrow = document.getAnonymousElementByAttribute(this, "anonid", "arrow");
>+        arrow.hidden = false;
>         arrow.setAttribute("side", anchorClass);
>         this.setAttribute("side", anchorClass);
[Odd; container inherits side, but arrow does not?]
Attachment #688782 - Flags: feedback?(enndeakin)
(In reply to comment #44)
> (From update of attachment 688782 [details] [diff] [review])
> >+        if (position.indexOf("start_") == 0 || position.indexOf("end_") == 0) {
> Good news! We now have .startsWith (and .endsWith) primitives :-)
Well, maybe not, they keep getting backed out.

For .startsWith you could use position.lastIndexOf("start_", 0) == 0
Or you could split the position on the underscore and test the halves.
It turns out that this doesn't meet our requirements for social - I really should have tried to use these patches in our context before now :(

The problem is the way the offsets are used in the flip situation.  Let's say we have a panel with "before_start" and an offset of 10px.  When this panel is flipped, the offset is applied against the "end" of the anchor position (ie, the arrow is drawn 10px past the end of the anchor), whereas we want the arrow drawn at a 10px offset from the start of the anchor, even if the panel alignment gets adjusted via flipping.  IOW, once the panel gets flipped, the requested offset needs to be adjusted by the anchor width/height so the arrow position remains where it would have been without the flip.

To put it another way, when we say we want an offset of 10px, we are specifying the exact position we want the arrow relative to the "position" we requested, *not* relative to the adjusted position after flipping.

TBH, I think this is what most users of an "offset" will be expecting (which is different than what these patches do).  When someone specifies a position and offset, they probably have an exact arrow position in mind.  I can't see why someone would specify an offset but not care exactly where that offset was applied from.

FWIW, our anchor is the sidebar itself.  I guess it might be possible to work around this by creating a dummy 0px element above the sidebar which we could use as the anchor, but (a) that might not work given it is 0px and (b) as above, I'm not sure what we've come up with here really will meet anyones requirements for the offset when the panel gets flipped.

Thoughts/comments?  And sincere apologies for the very late realization of this :(
Thinking some more, it's likely (but I haven't tested) that comment 46 is describing a new feature - that the current patches make the offset handling work as it does now.  Assuming that is the case, what I'm describing could be considered a part of the "sliding arrow" bug 812943 need not derail this.
It sounds like you just want to position the popup at a point rather than a rectangular anchor. Maybe you just want to add support for mFlipBoth to openPopupAtScreen and use that instead?
(In reply to Neil Deakin from comment #48)
> It sounds like you just want to position the popup at a point rather than a
> rectangular anchor. Maybe you just want to add support for mFlipBoth to
> openPopupAtScreen and use that instead?

What we want is better described as "open the popup anchored to a client point" - ie, the point we specify is where the tip of the arrow should be - which is almost identical to what bug 812943 is asking for.  We don't care where the popup is actually positioned, so long as the arrow is at the point we specify and the popup is of the size we request.  However, the point is relative to the window - so as the window moves, the panel/arrow should too - IOW, conceptually it is still relative to the anchor.

So maybe we want a method like openPopupAnchoredAtPoint().  OTOH though, is there really a use-case for the current behaviour of "position the arrow at exactly XXpx from an arbitrary corner of the anchor"?
Attached patch Reset flip members on move. (obsolete) — Splinter Review
I just discovered an additional problem with these patches - when calling moveToAnchor multiple times, once the panel gets flipped it remains in the flipped state even when a subsequent moveToAnchor should not cause it to be treated as flipped.  When this occurs, the panel is repositioned correctly but the arrow remains on the "flipped" side rather than the requested non-flipped side.

The issue is simply that mVFlip and/or mHFlip aren't reset until the popup is hidden.  The following patch resets both these to false in :InitializePopup, and has a couple of tweaks to the test to demonstrate this.  This patch needs to be applied on top of the other patches here (actually, I haven't applied it against the new versions of the attachments here, but hopefully the patch is trivial enough it is easy to apply)
Attachment #690289 - Flags: review?(enndeakin)
Attachment #690289 - Flags: review?(enndeakin) → review+
Depends on: 821283
Addresses comments and adds a test. The test relies on bug 821283.
Attachment #688782 - Attachment is obsolete: true
Attachment #688782 - Flags: review?(neil)
Attachment #688782 - Flags: feedback?(enndeakin)
Attachment #691799 - Flags: review?(neil)
Comment on attachment 691799 [details] [diff] [review]
Rework arrow positon computation, v3

> void
> nsMenuPopupFrame::InitializePopup(nsIContent* aAnchorContent,
>                                   nsIContent* aTriggerContent,
>                                   const nsAString& aPosition,
...
>+  mPosition = POPUPPOSITION_UNKNOWN;
Still not sure why you're setting this here but not initialising it in the constructor with the anchor and align. Rest looks good!
Attachment #691799 - Flags: review?(neil) → review+
Whiteboard: [leave open]
> +        if (position.indexOf("start_") == 0 || position.indexOf("end_") == 0) {

Shouldn't we be using .startsWith() and .contains() instead of .indexOf() ?
Comment 45 suggests those methods were backed out, so I didn't change this. But a followup bug could be filed.
This is an integrated version of the other 2 patches with r+ from enndeakin and what I landed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c928f50fe4fc
Attachment #688578 - Attachment is obsolete: true
Attachment #690289 - Attachment is obsolete: true
Attachment #694641 - Flags: review+
Once that last patch makes it to m-c, I think we are OK to close this.
Whiteboard: [leave open]
Backed out due to android test bustage

https://hg.mozilla.org/integration/mozilla-inbound/rev/52b2bf6fc12e
Neil,  are you OK with me re-landing this with the test disabled on android?  If you are and you know how I might actually do that, please let me know what the secret sauce is :)
Flags: needinfo?(enndeakin)
Attachment 691799 [details] [diff] has caused a regression in panels that are positioned close to the edge of the screen. See bug 824290.
Backed out this patch until the issue of the os adjusting the popup to fit onscreen can be solved.
Flags: needinfo?(enndeakin)
https://hg.mozilla.org/mozilla-central/rev/c454966ef45c

I'm going to assume that this should have had a [leave open] on the whiteboard. Please close this if it shouldn't have (and put it on in the future so your bugs don't get accidentally resolved ;)...).
Flags: in-testsuite+
Whiteboard: [leave open]
To make this less confusing, I created a new bug 824963, for my patch.
Depends on: 824963
Blocks: 793057
Taking this bug now that the heavy-lifting is being done in bug 824963
Assignee: enndeakin → mhammond
Whiteboard: [leave open]
Attached patch updated against bug 824963 (obsolete) — Splinter Review
The following patch is on top of the patches in bug 824963.  I'll ask for review once that bug lands.
Attachment #691799 - Attachment is obsolete: true
Attachment #694641 - Attachment is obsolete: true
This patch has been updated against trunk, and updated against the popup anchor tests - specifically, a couple of "todo_is" calls related to this bug have been updated to simple "is".  Enn already reviewed the main portion of this patch, but it was long enough ago a new review is probably necessary, and given Enn is away for a couple of months, Neil draws the short straw :)
Attachment #713808 - Attachment is obsolete: true
Attachment #740152 - Flags: review?(neil)
Blocks: 812943
Comment on attachment 740152 [details] [diff] [review]
Updated against trunk

Seems reasonable to me, but I haven't read too deeply into the code.
Attachment #740152 - Flags: review?(neil) → review+
https://hg.mozilla.org/mozilla-central/rev/ec8eddd13fac
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
This made changes that need documentation on MDN, right? (https://developer.mozilla.org/en-US/docs/XUL/panel)
Keywords: dev-doc-needed
Component: XP Toolkit/Widgets: Menus → XUL
You need to log in before you can comment on or make changes to this bug.