824963 - Improve arrow panel placement

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Neil Deakin]

Attachment: Patch

The existing code relies on a lot of guesswork as to where to place the arrow and popup. Instead, use the alignment supplied when opening the popup.

This bug was split off from bug 798226, where a mostly working patch is available. However, it will need to be updated to handle when the os moves the popup on its own to fit on screen.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: Check current direction before flipping

What I think is happening is that the new code wants to flip the panel due to the size not fitting - but the panel is already in the direction to be flipped.  popup.xml then sees there was a flip and moves the arrow to the wrong end of the panel.

Specifically, we have a panel with "after_end", so the right-hand sides of the panel and anchor should be aligned.  In FlipOrResize, the condition:

      // check whether there is more room to the left and right (or top and
      // bottom) of the anchor and put the popup on the side with more room.
      if (aScreenEnd - endpos >= startpos - aScreenBegin) {

returns false as the panel is very close to the RHS of the screen - so we enter the branch where we decide to flip - but we are *already* oriented correctly (ie, the panel is already to the left of the anchor) - so flipping is unnecessary - we just want to resize.

The following patch works for me with the "downloads" button, and I've a patch to my panel tests that demonstrates the problem and demonstrates the fix works.  Note that only the second chunk is hit in this specific case and the behaviour seems exactly the same as before any of these patches are applied.

However, my test which tries the same thing on the left-hand-side does *not* seem to be fixed with this patch (which should hit the first chunk of the patch).  Thus, I don't think the patch is totally correct, but should give you a reasonable idea of what is going wrong.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: Use mPopupAlignment to check direction before flip

This attachment checks the current alignment before attempting a flip.  Eg, in the horizontal direction, the check:

+    bool isAlreadyBefore = mPopupAlignment == POPUPALIGNMENT_TOPRIGHT ||
+                           mPopupAlignment == POPUPALIGNMENT_BOTTOMRIGHT;

is used to determine if the panel is already "before" the anchor.  This is then passed as a new param to FlipOrResize, and the code there checks this variable before attempting to flip.  I'm confident you will suggest a better name for this variable ;)  However, please feel free to simply roll this into your patch with whatever changes you desire.  This patch works correctly when the anchor is to the extreme left or right of the window/screen and solves the problem with the downloads panel when the anchor is near the right.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Attachment: Panel tests

This is a "port" of the tests from bug 798226.  All features added by that bug are marked as "todo" in this patch (so the tests all pass with the patches in this bug).  This also includes a test case for the "anchor on the very right" and "anchor on the very left" cases - so the test demonstrates the problem with the downloads manager, and passes when the patch to check the current direction before flipping is applied.

Like the other patch, I'm more than happy for you to fold all these attachments into a single patch for landing, then I'll make the necessary changes to the test for bug 798226.

Comment 4

[Neil Deakin]

Is it the case that the new value of aScreenPoint is always equal to the existing value when we don't really want to flip? Could we just do:

-        *aFlipSide = true;
-        aScreenPoint = startpos - aSize - aMarginBegin - aOffsetForContextMenu;
-        // check if the new position is still off the left or top edge of the
-        // screen. If so, resize the popup.
-        if (aScreenPoint < aScreenBegin) {

+        nscoord newScreenPoint = startpos - aSize - aMarginBegin - aOffsetForContextMenu;
+        if (newScreenPoint != aScreenPoint) {
+          *aFlipSide = true;
+          aScreenPoint = newScreenPoint;
+          // check if the new position is still off the left or top edge of the
+          // screen. If so, resize the popup.
+          if (aScreenPoint < aScreenBegin) {
...

Comment 5

[Neil Deakin]

Note also on Mac, that the problem I described when filing this bug also exists with your patch, that the OS can shift a popup onscreen on its own, so the arrow can appear incorrectly.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

(In reply to Neil Deakin from comment #5)
> Note also on Mac, that the problem I described when filing this bug also
> exists with your patch, that the OS can shift a popup onscreen on its own,
> so the arrow can appear incorrectly.

I don't have a Mac to repro this, but I don't see how this can happen - the code as it stands doesn't seem to ever cause the popup to be off-screen (on Windows), so I don't understand why the OS would move it.

Do any of the tests fail on Mac?

Comment 7

[Neil Deakin]

Mark, any thought on comment 4?

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Attachment: Check existing and new position before flipping.

(In reply to Neil Deakin from comment #4)
> Is it the case that the new value of aScreenPoint is always equal to the
> existing value when we don't really want to flip? Could we just do:
...

Yes!  This new patch does just this and the test pass.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Attachment: Check existing and new position before flipping (now with the actual changes!)

doh - forgot to qref - this is the correct patch.

Comment 10

[Mark Hammond [:markh] [:mhammond]]

Nice looking try run at https://tbpl.mozilla.org/?tree=Try&rev=657f6b15bf29 - except android (which is probably just the dumb test; I'll fix it).

Also FYI, the patch in this bug still mentions bug 798226 in the header commit message.

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Attachment: Panel tests, disabled for android

Same tests as previous patch but disabled on Android.

Comment 12

[Neil Deakin]

Comment on attachment 704779 [details] [diff] [review]
Check existing and new position before flipping (now with the actual changes!)

Can you add to the comment something like:

"If the new position is the same as the existing position, the popup has already flipped. This can happen, for example, if the popup has a negative margin that pushes it off the anchored side of the screen."

Comment 13

[Neil Deakin]

Comment on attachment 705171 [details] [diff] [review]
Panel tests, disabled for android

> +      func.call(tests, function() {

Do you mean to use 'tests' here?

I find this test with all the iterators very difficult to read and understand. I'd rather have one large array containing all tests (not an object) where the iteration order is guaranteed.

Comment 14

[Mark Hammond [:markh] [:mhammond]]

Attachment: Use an array of tests

(In reply to Neil Deakin from comment #13)
> Comment on attachment 705171 [details] [diff] [review]
> ...
> I find this test with all the iterators very difficult to read and
> understand. I'd rather have one large array containing all tests (not an
> object) where the iteration order is guaranteed.

Ask and ye shall receive!

Comment 15

[Mark Hammond [:markh] [:mhammond]]

try at https://tbpl.mozilla.org/?tree=Try&rev=c897ff16ed0c

Comment 16

[Neil Deakin]

Comment on attachment 713791 [details] [diff] [review]
Use an array of tests

>+    panel.hidePopup();
>+    panel.sizeTo(100, 50);

The previous patch used (50, 50) for the middle tests. Was this an intentional change?

Comment 17

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 713791 [details] [diff] [review]
Use an array of tests

(In reply to Neil Deakin from comment #16)
> The previous patch used (50, 50) for the middle tests. Was this an
> intentional change?

It was done to simplify the code so the same size if used for all tests.  Using 100, 50 is really just because a 50,50 popup means it's hard to visually see which side of the panel the arrow is positioned on if tests fail.

Requesting review on this - assuming it is granted, this only leaves Neil's original patch without a review (which I'd be happy to do, although I'm not a peer).  It would be awesome to finally get this landed!  And thanks for sticking with me on this!

Comment 18

[Neil Deakin]

Comment on attachment 713791 [details] [diff] [review]
Use an array of tests

I got a failure when I tried these tests. This is because the previous test (test_mousecapture_area.html) causes an assertion after it is unloaded. I fixed it by moving the SimpleTest.expectAssertions(2) line from test_videocontrols.html to the new test you added here.

Comment 19

[Mark Hammond [:markh] [:mhammond]]

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

I don't think this patch needs anything else to be done and it's just going to get stale - there are a number of extra tests too, which is a bonus!  Neil already reviewed a very similar patch in bug 798226, so I'm hoping he can do the same here!

Comment 20

[[e-mail]]

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

>@@ -546,16 +549,17 @@ nsMenuPopupFrame::InitializePopup(nsICon
>   EnsureWidget();
> 
>   mPopupState = ePopupShowing;
>   mAnchorContent = aAnchorContent;
>   mTriggerContent = aTriggerContent;
>   mXPos = aXPos;
>   mYPos = aYPos;
>   mAdjustOffsetForContextMenu = false;
>+  mPosition = POPUPPOSITION_UNKNOWN;
I don't think this is necessary now that you initialise it in the constructor. (And unless aAnchorContent is null then the anchor, alignment and position all get overwritten anyway.) r=me with that fixed.

Comment 21

[Mark Hammond [:markh] [:mhammond]]

I pushed a try run with the requested change to https://tbpl.mozilla.org/?tree=Try&rev=1d426bb2b0bc, and I'll land all 3 patches once it goes green...

Comment 22

[Neil Deakin]

Did we fix the regression (bug 824290) the patch caused?

Comment 23

[Mark Hammond [:markh] [:mhammond]]

(In reply to Neil Deakin from comment #22)
> Did we fix the regression (bug 824290) the patch caused?

I believe the attachment "Check existing and new position before flipping (now with the actual changes!)" solves this case.  At least it fixed it on Windows where the OS was not actually moving the Window (and IIUC, the OS should never move the popup as we go to extraordinary lengths to prevent requesting an offscreen position in the first place)

Comment 24

[Mark Hammond [:markh] [:mhammond]]

Attachment: Wait for arrow images to load before starting tests

I ended up with try failures on some platforms, that after much frustration I determined was caused by the images for the arrow loading slightly after the popupshown event.  This meant there was a very brief time when the "old" arrow image (ie, vertical vs horizontal or vice-versa) was being used, which screwed up the measurements taken by the tests.

The fix I came up with was to listen to *both* the arrow's load event and the popupshown event and start each test after they both fire.  Given this doesn't sound particularly hacky and is a test-only change, I'm going to carry the r+ forward (unless someone objects, or course ;)

Nice green try run at https://tbpl.mozilla.org/?tree=Try&rev=3269a6e4f6f8

Comment 25

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/01a8ea541103
https://hg.mozilla.org/integration/mozilla-inbound/rev/d625a0a9476e
https://hg.mozilla.org/integration/mozilla-inbound/rev/db41fc9ad61d

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/01a8ea541103
https://hg.mozilla.org/mozilla-central/rev/d625a0a9476e
https://hg.mozilla.org/mozilla-central/rev/db41fc9ad61d