966241 - UITour: Menu panel arrow is on the wrong side when window is maximized

Status: VERIFIED FIXED

(Firefox :: General, defect, P2)

Description

[Alex Gibson [:agibson]]

Operating System: Mac OSX (possibly others?)

STR:

In the latest nightly start the UITour with the window maximised:

https://www-demo3.allizom.org/en-US/firefox/29.0a2/whatsnew/

Expected results:

Menu panel arrow should be on the right, pointing to the new app menu icon.

Actual results:

The arrow is on the left, pointing to the search bar.

Note this bug does not happen if there is some space to the right of the browser window, so it only appears to be effected when the browser is flush with the edge of the desktop.

Comment 1

[Alex Gibson [:agibson]]

Attachment: menu-panel.png

Added a screenshot of menu panel pointing to the wrong place

Comment 2

[Matthew N. [:MattN]]

I can reproduce the issue as well on Mavericks (HiDPI). Neil, do you have time to take a look at this?

Perhaps we could workaround this using @flip="none" on the panel but a proper fix would be good.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

I've seen this on Windows and Linux as well.

Comment 4

[Neil Deakin]

This was caused by bug 941051. I'm going to undo that change and fix it in a different way.

Comment 5

[Neil Deakin]

Attachment: Better Fix

This patch reverts bug 941051 and just improves the intersection check instead.

Comment 6

[[e-mail]]

Comment on attachment 8369493 [details] [diff] [review]
Better Fix

What about the test changes?

>-    // ensure that anchorRect is on screen
>-    if (!anchorRect.IntersectRect(anchorRect, screenRect)) {
>-      anchorRect.width = anchorRect.height = 0;
>-      // if the anchor isn't within the screen, move it to the edge of the screen.
>-      if (anchorRect.x < screenRect.x)
>-        anchorRect.x = screenRect.x;
>-      if (anchorRect.XMost() > screenRect.XMost())
>-        anchorRect.x = screenRect.XMost();
>-      if (anchorRect.y < screenRect.y)
>-        anchorRect.y = screenRect.y;
>-      if (anchorRect.YMost() > screenRect.YMost())
>-        anchorRect.y = screenRect.YMost();
>-    }
>+    // Ensure that anchorRect is on screen.
>+    anchorRect = anchorRect.Intersect(screenRect);
I had a look at the code of IntersectRect here can't actually work out what the change in behaviour is here, could you please elucidate?

Comment 7

[Neil Deakin]

The rounding code added to the test makes this test fail less. A number of other popup related tests have similar code. So I'd rather keep it in place.

IntersectRect considers any rectangle with a zero width *OR* zero height to not intersect. The code in the conditional block then sets both the width *AND* the height to 0. I'm not sure why the code is doing this. Intersect already adjusts the rectangle accordingly, so it is all that is needed.

Comment 8

[Matthew N. [:MattN]]

FYI: We'd like this patch on Aurora by this Friday for the first Aurora release in this cycle so that this bug won't affect the Australis update tour shown to users at that time.

Comment 9

[[e-mail]]

(In reply to Neil Deakin from comment #7)
> IntersectRect considers any rectangle with a zero width *OR* zero height to
> not intersect. The code in the conditional block then sets both the width
> *AND* the height to 0. I'm not sure why the code is doing this. Intersect
> already adjusts the rectangle accordingly, so it is all that is needed.

So this is literally an edge case where the fact that the two rectangles intersect along an edge, and therefore the intersection needs to be a line, when the old code was reducing it to a point?

Comment 10

[Neil Deakin]

Sort of. When using centre alignment (bottmcentre for example), the anchor width is always 0. If this is near the edge of the screen, the intersection check changes the height to be 0 as well and the popup gets positioned in the wrong place vertically.

Comment 11

[[e-mail]]

(In reply to Neil Deakin from comment #10)
> Sort of. When using centre alignment (bottmcentre for example), the anchor
> width is always 0. If this is near the edge of the screen, the intersection
> check changes the height to be 0 as well and the popup gets positioned in
> the wrong place vertically.

OK, that makes perfect sense now. Thanks for making me feel slow on the uptake.

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8369493 [details] [diff] [review]
Better Fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 941051 
User impact if declined: The arrow on an arrow panel can appears in the wrong position (e.g. if the window is maximized and the UITour panel opens on the menu button)
Testing completed (on m-c, etc.): None yet
Risk to taking this patch (and alternatives if risky): Low(?) risk panel positioning change. Unlikely to be much worse than this bug which will show up immediately upon launching Aurora for the tour.
String or IDL/UUID changes made by this patch: None

Enn, shouldn't we add an automated test for this case? I'd like to get this on Aurora tonight for tomorrows UITour build so perhaps we can do that in a follow-up or separate patch?

Comment 13

[Neil Deakin]

https://hg.mozilla.org/integration/fx-team/rev/9a97c90f0f2e

Comment 14

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0b3a6ee57157

Comment 15

[Matthew N. [:MattN]]

For some reason this didn't get closed automatically:
http://hg.mozilla.org/mozilla-central/rev/9a97c90f0f2e

Comment 16

[svbabukumar]

Bug Triage Day 24/2/2014

Validated the bug in latest Nightly B30 and it is resulting as expected

Comment 17

[Cornel Ionce [:noni] [Hubs QA]]

Confirming, this is fixed on latest Nightly and on latest Aurora builds.

User Agents:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (X11; Linux i686; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (X11; Linux i686; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0