Improve arrow panel placement

RESOLVED FIXED in mozilla23

Status

()

Toolkit
XUL Widgets
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Neil Deakin, Assigned: markh)

Tracking

unspecified
mozilla23
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 696038 [details] [diff] [review]
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.
(Assignee)

Comment 1

5 years ago
Created attachment 696241 [details] [diff] [review]
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.
(Assignee)

Comment 2

5 years ago
Created attachment 697330 [details] [diff] [review]
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.
Attachment #696241 - Attachment is obsolete: true
Attachment #697330 - Flags: feedback?(enndeakin)
(Assignee)

Comment 3

5 years ago
Created attachment 697331 [details] [diff] [review]
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.
Attachment #697331 - Flags: feedback?(enndeakin)
(Assignee)

Updated

5 years ago
Blocks: 798226
OS: Mac OS X → All
Hardware: x86 → All
(Reporter)

Comment 4

5 years ago
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) {
...
(Reporter)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
(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?
(Reporter)

Comment 7

5 years ago
Mark, any thought on comment 4?
Flags: needinfo?(mhammond)
(Assignee)

Comment 8

5 years ago
Created attachment 704778 [details] [diff] [review]
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.
Attachment #697330 - Attachment is obsolete: true
Attachment #697330 - Flags: feedback?(enndeakin)
Flags: needinfo?(mhammond)
(Assignee)

Comment 9

5 years ago
Created attachment 704779 [details] [diff] [review]
Check existing and new position before flipping (now with the actual changes!)

doh - forgot to qref - this is the correct patch.
Attachment #704778 - Attachment is obsolete: true
(Assignee)

Comment 10

5 years ago
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.
(Assignee)

Comment 11

5 years ago
Created attachment 705171 [details] [diff] [review]
Panel tests, disabled for android

Same tests as previous patch but disabled on Android.
Attachment #697331 - Attachment is obsolete: true
Attachment #697331 - Flags: feedback?(enndeakin)
Attachment #705171 - Flags: feedback?(enndeakin)
Blocks: 797209
(Reporter)

Comment 12

5 years ago
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."
Attachment #704779 - Flags: review+
(Reporter)

Comment 13

5 years ago
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.
Attachment #705171 - Flags: feedback?(enndeakin) → feedback-
(Assignee)

Comment 14

5 years ago
Created attachment 713791 [details] [diff] [review]
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!
Attachment #705171 - Attachment is obsolete: true
Attachment #713791 - Flags: feedback?(enndeakin)
(Reporter)

Comment 16

5 years ago
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?
Attachment #713791 - Flags: feedback?(enndeakin) → feedback+
(Assignee)

Comment 17

5 years ago
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!
Attachment #713791 - Flags: review?(enndeakin)
(Reporter)

Comment 18

5 years ago
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.
Attachment #713791 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 19

5 years ago
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!
Attachment #696038 - Flags: review?(neil)
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.
Attachment #696038 - Flags: review?(neil) → review+
(Assignee)

Comment 21

5 years ago
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...
Assignee: nobody → mhammond
(Reporter)

Comment 22

5 years ago
Did we fix the regression (bug 824290) the patch caused?
(Assignee)

Comment 23

5 years ago
(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)
(Assignee)

Comment 24

5 years ago
Created attachment 738360 [details] [diff] [review]
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
Attachment #713791 - Attachment is obsolete: true
Attachment #738360 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/01a8ea541103
https://hg.mozilla.org/mozilla-central/rev/d625a0a9476e
https://hg.mozilla.org/mozilla-central/rev/db41fc9ad61d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.