Closed Bug 966241 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: General, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified

People

(Reporter: agibson, Assigned: enndeakin)

References

Details

Attachments

(2 files)

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.
Blocks: fx-UITour
Attached image menu-panel.png
Added a screenshot of menu panel pointing to the wrong place
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.
Flags: needinfo?(enndeakin)
I've seen this on Windows and Linux as well.
OS: Mac OS X → All
Hardware: x86 → All
This was caused by bug 941051. I'm going to undo that change and fix it in a different way.
Blocks: 941051
Flags: needinfo?(enndeakin)
Attached patch Better FixSplinter Review
This patch reverts bug 941051 and just improves the intersection check instead.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Attachment #8369493 - Flags: review?(neil)
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?
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.
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.
(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?
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.
(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.
Attachment #8369493 - Flags: review?(neil) → review+
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?
Attachment #8369493 - Flags: approval-mozilla-aurora?
Attachment #8369493 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
For some reason this didn't get closed automatically:
http://hg.mozilla.org/mozilla-central/rev/9a97c90f0f2e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Keywords: verifyme
Bug Triage Day 24/2/2014

Validated the bug in latest Nightly B30 and it is resulting as expected
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
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.