606343 - Fix alignment of arrow panel so that the arrow points to center of the anchor

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[:Margaret Leibovic]

Right now, the arrowpanel arrow doesn't point to the center of its anchor. Although it looks like a windows-only problem right now, that is because the pinstripe theme is setting a negative margin on popup notifications to adjust for this (http://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/browser/browser.css#1941) (also because popup notifications are the only panels implemented as arrow panels right now).

I tried adding these margins to the widget theme css then to the xul widget code itself, but both sets of changes caused test_arrowpanel.xul to fail, so I think we need a different solution.

See bug 577928 comments 27-29 for some existing discussion about this.

Comment 1

[Dão Gottwald [:dao]]

See also bug 577928 comment 33-37.

Comment 2

[Justin Dolske [:Dolske]]

This needs to block, arrow panels not pointing to the right anchor make a primary piece of new UI rather confusing to use.

Comment 3

[Neil Deakin]

(In reply to comment #0)
> Right now, the arrowpanel arrow doesn't point to the center of its anchor.

Actually, I realized the intent was not to have the arrow point to the centre of the anchor, but instead for the popup to be aligned along the left/right edge of the anchor (as other menus/popups do).

If we want to actually have the popup centred on the anchor, then I'd need to add more position support in nsMenuPopupFrame.cpp

Comment 4

[Nochum Sossonko [:Natch]]

IMHO there's no need for more position support, only to support the current positions, and per those positions set the arrow on the panel to align with the anchor. Unless that's considered "breaking" the api...

Comment 6

[Justin Dolske [:Dolske]]

(In reply to comment #3)
> (In reply to comment #0)
> > Right now, the arrowpanel arrow doesn't point to the center of its anchor.
> 
> Actually, I realized the intent was not to have the arrow point to the centre
> of the anchor, but instead for the popup to be aligned along the left/right
> edge of the anchor (as other menus/popups do).
> 
> If we want to actually have the popup centred on the anchor, then I'd need to
> add more position support in nsMenuPopupFrame.cpp

Maybe I'm parsing your comment wrong, but the arrow definitely needs to point to the center of its anchor node, just like the bookmark (star) panel does on OS X. I checked with faaborg just to be sure.

Attachment 466966 [details] shows the basic scheme, the last few mockups in attachment 466899 [details] show why this is important... When multiple doorhanger anchors are present in the URL bar, it should be clear which icon is associated with the panel, pointing at an edge would be ambiguous.

Comment 7

[Robert Kaiser]

(In reply to comment #3)
> Actually, I realized the intent was not to have the arrow point to the centre
> of the anchor, but instead for the popup to be aligned along the left/right
> edge of the anchor (as other menus/popups do).

I start to think the best thing to do would be to have it point to the center of the anchor as long as the anchor is smaller than (2 * the distance from the edge of the panel to the tip of the arrow) and have it aligned as you say above when the anchor is larger. This way it would fit well for pointing to an icon , like we are using it right now, but also pointing to, say, a text field (like the URLbar) in some later or add-on incarnation.

This is just theoretical "would look good" talk, of course, but this would make sense to me as a target. Note that I'm just a community voice here, just trying to give feedback and ideas.

Comment 8

[Neil Deakin]

Attachment: patch

This patch adds some positioning support to point the arrow to the center of the anchor. I've done this for notifications in this patch.

Doing this means larger default margins on the popup, but I wonder if there are cases where a larger anchor might not use centered arrows.

More polish of how the positioning flags are used is needed as well as tests, but I'd rather have some feedback first (from who?)

Comment 9

[:Margaret Leibovic]

Attachment: osx screenshot

I applied the patch and the arrow disappeared :(

It's applied on top of my patch for bug 577931, but I don't think that should affect the alignment of the panel itself.

Comment 10

[Neil Deakin]

That image looks like what would happen if you hadn't rebuilt layout.

When I apply your patches, I get the arrow correctly appearing centered and underneath the geolocation icon.

Comment 11

[:Margaret Leibovic]

Attachment: site identity panel screenshot

I thought I had rebuilt layout, but I guess something I did something wrong. The arrow is appearing for me now.

I applied this patch with the patch from bug 597557 (the one updated to work on osx), and although the popup notification alignment is correct now, there seem to be some alignment problems with the site identity panel and the edit bookmarks panel. I haven't investigated this too much, so something else may be causing those panels to go awry, but I figured I'd bring it to your attention. I tried setting popupanchor="bottomcenter" on those panels, but that didn't change their position.

Comment 12

[Neil Deakin]

Attachment: Part 1: add centering position types

This adds support for:

- setting the two parts of the position (popupanchor and popupalign) separately in the position
- four new centering anchor values: topcenter, bottomcenter, leftcenter, rightcenter

Typically, for arrow panels, position="bottomcenter topleft" would be used (or topright to hang to the left)

Comment 13

[Neil Deakin]

Attachment: Part 2: fix up arrow panels and notifications to use centred alignment

Comment 14

[Neil Deakin]

Attachment: Part 2: use centering for bookmarks and identity popup

This patch relies on the patches in other bugs to make the bookmarks and identity panels use arrow panels.

Comment 15

[Neil Deakin]

Attachment: Part 4: issue in rtl ui with arrow panels

Comment 16

[Neil Deakin]

Builds with these patches and those in various other bugs are at:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-37bee4c1be77/

Comment 17

[Nochum Sossonko [:Natch]]

(In reply to comment #16)
> Builds with these patches and those in various other bugs are at:
> 
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-37bee4c1be77/

No windows builds there... :(

Comment 18

[Neil Deakin]

Some Windows builds are at http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-97b2f557b03c/tryserver-win32/

Comment 19

[Luke Iliffe (Harlequin99)]

Attachment: Windows 7 Screenshot with dialog in content area and pointing to right of anchor

I have attached a screenshot of on Win 7. I can see 2 small issues:
1. The arrow appears to me to be pointing to the right of the anchor rather than the centre.

2. The geolocation and password doorhangers appear to be entirely within the content area, as bug 607252 was dupped to this I assumed this would not be the case.

Compare with the site identity doorhanger where neither of these issues are present.

Comment 20

[pino_sb]

Some issues I notice in the trybuild

1. as noted above, doorhangers are beneath the point they should aim at, whereas other panels are positioned closer, yet still off slightly
2. Expanding the folder field in the "edit bookmark" dialog makes the panel flicker as it resizes (not terribly bad)
3. Both selecting the folder dropdown in the "edit bookmark" dialog and selecting the additional options in a doorhanger will move the panel back and forth horizontally.
4. When opening site identity panel in different tabs, the previous 'larry' image shortly shows (it quickly swaps to the right color); probably not caused by this bug?

Also button styles look rather weird, but that is due other patches...

Comment 21

[Neil Deakin]

(In reply to comment #20)
> 2. Expanding the folder field in the "edit bookmark" dialog makes the panel
> flicker as it resizes (not terribly bad)

This is a different bug.

> 4. When opening site identity panel in different tabs, the previous 'larry'
> image shortly shows (it quickly swaps to the right color); probably not caused
> by this bug?

Also a different bug.

Please check or file these bugs separately. Thanks.

Comment 22

[Neil Deakin]

Attachment: Part 2, version 2: fix issues in previous comments

This fixes:
 - alignment of arrow on Windows 7
 - horizontal and vertical padding around arrow
 - horizontal jumping when closing the menu button popup

Comment 23

[Neil Deakin]

New builds will be available at:
 http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-c981f59a91cb/

Comment 24

[(mostly gone) XtC4UaLL [:xtc4uall]]

In reply to comment #20)
> 2. Expanding the folder field in the "edit bookmark" dialog makes the panel
> flicker as it resizes (not terribly bad)

FYI, that seems to be Bug 511010.

Comment 25

[pino_sb]

This new build seems to fix the mentioned issues indeed. It looks really nice. Only thing that could be changed is for the arrow to actually touch the element it points at, in stead of a 5 pixel gap, but I suppose that is a design choice.

Comment 26

[[e-mail]]

Comment on attachment 488189 [details] [diff] [review]
Part 1: add centering position types

>+    case POPUPALIGNMENT_LEFTCENTER:
>+      pnt = nsPoint(anchorRect.x, anchorRect.y + anchorRect.height / 2);
>+      anchorRect.y = pnt.y;
>+      anchorRect.height = 0;
Why do you need to change the anchorRect?

>-  FlipStyle anchorEdge = mFlipBoth ? FlipStyle_Inside: FlipStyle_None;
>+    FlipStyle anchorEdge = mFlipBoth ? FlipStyle_Inside: FlipStyle_None;
[Looks like this : lost its preceding space. Your big chance to restore it!]

Comment 27

[:Margaret Leibovic]

Attachment: bug when panel flips horizontally

The panels look great with the new patch! However, I found a bug :( I attached a screenshot of what happened when I moved my browser window to the right edge of my screen. This seems to only happen for a small range of horizontal window positions, because when I moved the browser window further right, the arrow pointed to the anchor correctly (with the panel still flipped horizontally).

Comment 28

[Neil Deakin]

Likely something not accounting for the margin. I'll take a look, and see if I can create a followup patch.

Comment 29

[Neil Deakin]

(In reply to comment #26)
> >+    case POPUPALIGNMENT_LEFTCENTER:
> >+      pnt = nsPoint(anchorRect.x, anchorRect.y + anchorRect.height / 2);
> >+      anchorRect.y = pnt.y;
> >+      anchorRect.height = 0;
> Why do you need to change the anchorRect?

So that when the popup is flipped, it flips around the centreline rather than the edge of the anchor.

Comment 30

[Dão Gottwald [:dao]]

Comment on attachment 489551 [details] [diff] [review]
Part 2, version 2: fix issues in previous comments

>--- a/toolkit/content/PopupNotifications.jsm
>+++ b/toolkit/content/PopupNotifications.jsm
>@@ -397,17 +397,18 @@ PopupNotifications.prototype = {
>     this.panel.hidden = false;
> 
>     this._refreshPanel(notificationsToShow);
> 
>     if (this.isPanelOpen && this._currentAnchorElement == anchorElement)
>       return;
> 
>     // Make sure the identity popup hangs in the correct direction.
>-    var position = (this.window.getComputedStyle(this.panel, "").direction == "rtl") ? "after_end" : "after_start";
>+    var position = (this.window.getComputedStyle(this.panel, "").direction == "rtl") ?
>+      "bottomcenter topright" : "bottomcenter topleft";

This seems wrong... That is, the existing code seems wrong and the new code too. We'd normally want after_start or the arrow panel counterpart for RTL. /Only in the case of the location bar/ we typically want the opposite, because the location bar is always LTR, but this isn't location bar specific code.

Btw, it seems that "bottomcenter topleft" really means something like "bottomcenter topstart", since the horizontal orientation is flipped for RTL? Maybe I'm just confused.

I wonder whether the fact that there's an arrow obsoletes the "popup needs to hang towards the center of the location bar" idea. Could we avoid adding the new (and imho confusing) position values then and just let the backend do the right thing? Or at least allow the attribute to be absent and assume "bottomcenter topleft" in that case?

Comment 31

[Neil Deakin]

(In reply to comment #30)
> Btw, it seems that "bottomcenter topleft" really means something like
> "bottomcenter topstart", since the horizontal orientation is flipped for RTL?
> Maybe I'm just confused.

It does mean 'topstart', but the existing string is 'topleft'. I could add 'topstart' as  synonym if desired.


> I wonder whether the fact that there's an arrow obsoletes the "popup needs to
> hang towards the center of the location bar" idea. Could we avoid adding the
> new (and imho confusing) position values then and just let the backend do the
> right thing?

What is 'the right thing?'

Comment 32

[Dão Gottwald [:dao]]

"bottomcenter topleft" (well, "bottomcenter topstart") seems like the position I'd usually expect for arrow panels, if there's room.

Comment 33

[Neil Deakin]

The geolocation icon always appears on the left (it's in the location bar) so I think that's why this code handles special-cases the position for rtl. However, the notification code doesn't seem to require the icon to be in the location bar; in those situations the position should not be adjusted, so the code is wrong in general.

> "bottomcenter topleft" (well, "bottomcenter topstart") seems like the position
> I'd usually expect for arrow panels, if there's room.

I could default the arrow panel to use that value if that's what you mean.

Comment 34

[Neil Deakin]

Attachment: Part 2, version 3: default to 'bottomcenter topleft' and fix issue described in comment 27

Comment 35

[Dão Gottwald [:dao]]

Comment on attachment 491188 [details] [diff] [review]
Part 2, version 3: default to 'bottomcenter topleft' and fix issue described in comment 27

>--- a/toolkit/content/PopupNotifications.jsm
>+++ b/toolkit/content/PopupNotifications.jsm
>@@ -397,17 +397,18 @@ PopupNotifications.prototype = {
>     this.panel.hidden = false;
> 
>     this._refreshPanel(notificationsToShow);
> 
>     if (this.isPanelOpen && this._currentAnchorElement == anchorElement)
>       return;
> 
>     // Make sure the identity popup hangs in the correct direction.
>-    var position = (this.window.getComputedStyle(this.panel, "").direction == "rtl") ? "after_end" : "after_start";
>+    var position = (this.window.getComputedStyle(this.panel, "").direction == "rtl") ?
>+      "bottomcenter topright" : "bottomcenter topleft";

This still seems wrong. Can you remove that code?

Comment 36

[Neil Deakin]

It requires a fix to PopupNotifications to support icons both inside and outside the location bar. I filed bug 612895 on this.

Comment 37

[Dão Gottwald [:dao]]

The question is why we'd want it to be flipped for the location bar.

Comment 38

[Neil Deakin]

(In reply to comment #37)
> The question is why we'd want it to be flipped for the location bar.

Because the code here is opening using the direction of the panel which is different that the location bar's direction which is always left-to-right.

Gavin suggested it should either use the anchor's direction instead or allow one to configure the anchor position for each notification. I didn't look into which is better since it's not an issue caused by this bug.

Comment 39

[Dão Gottwald [:dao]]

The original thinking for the bookmarking and identity panels was that they should "hang toward the middle of the location bar" (and just using the anchor's direction wouldn't do this, btw). This was without the arrows. With the arrow added and the panel border being moved away from the location bar, we should revisit this. For instance, the bookmarking panel hanging toward the right (if there's room) in LTR mode doesn't seem strange to me. I'm suggesting that RTL isn't actually a problem that needs solving, there's just some bogus code that can be removed.

Comment 40

[Neil Deakin]

The patches in this bug maintain the current behaviour, so what does that have to do with this bug?

Comment 41

[Dão Gottwald [:dao]]

The current PopupNotifications.jsm behavior is wrong, I'm not sure why we'd want to maintain it. The bookmark and identity panels aren't currently arrow panels, so the current behavior kind of makes sense to me, but I assume they would be arrow panels when your patches land, as the patches wouldn't make sense otherwise...

Comment 42

[Neil Deakin]

They look like arrow panels on Mac to me. (on 3.6 as well)

I accept that what you describe is bug and have filed bug 612895 on changing this, as I said earlier.

Comment 43

[Dão Gottwald [:dao]]

(In reply to comment #42)
> They look like arrow panels on Mac to me. (on 3.6 as well)

Pinstripe has some special styling. They aren't arrow panels per se and don't have such styling on other platforms.

> I accept that what you describe is bug and have filed bug 612895 on changing
> this, as I said earlier.

Maybe I'm missing the point of that bug -- it seems to imply that PopupNotifications.jsm should provide means for consumers to do the custom positioning rather than hardwiring it in PopupNotifications.jsm. I'm saying we can probably just drop that code, as it was obviously copied from the bookmark or identity panel, where that code was added when they weren't arrow panels (modulo Mac).

Comment 44

[Neil Deakin]

Could you maybe just create a patch that makes the change you think should be made? Your last comment has confused me into thinking your talking about a different fix than I am.

Comment 45

[Dão Gottwald [:dao]]

Attachment: what I had in mind

This is untested. It's supposed to make arrow panels default to "bottomcenter topleft" and let the bookmark, identity and notification panels utilize this.

Comment 46

[Neil Deakin]

I don't understand this patch. Can you please provide an actual working and tested patch?

- the bookmark panel should be using 'bottomcenter topright' so that it opens the opposite way (towards the left and the rest of the location bar)
- in rtl mode, the panels should open in the same direction.

The existing code manually flips the anchor position as the location bar is always ltr, but the popup can be ltr or rtl, but you seem to have removed this code, so it looks as if the popup will open the wrong way.

Comment 47

[Dão Gottwald [:dao]]

Depends on how you define "right way" and "wrong way". In my book the reading direction is the primary factor. As I tried to say earlier, we let the panels hang toward the center of the location bar when they were directly glued to the location bar. Making them arrow panels changes this.

Comment 48

[Neil Deakin]

(In reply to comment #47)
> Depends on how you define "right way" and "wrong way". In my book the reading
> direction is the primary factor. As I tried to say earlier, we let the panels
> hang toward the center of the location bar when they were directly glued to the
> location bar. Making them arrow panels changes this.

Then you should file a bug requesting this behaviour or have a discussion about that elsewhere. This bug is about changing the alignment of arrow panels from the edge of the popup to the centre of the arrow.

Comment 49

[Dão Gottwald [:dao]]

Comment on attachment 488212 [details] [diff] [review]
Part 4: issue in rtl ui with arrow panels

>diff --git a/toolkit/content/widgets/popup.xml b/toolkit/content/widgets/popup.xml
>--- a/toolkit/content/widgets/popup.xml
>+++ b/toolkit/content/widgets/popup.xml
>@@ -319,17 +319,19 @@
>         var hideAnchor = false;
>         if (horizPos == 0) {
>           container.orient = "vertical";
>           arrowbox.orient = "";
>           if (vertPos == 0) {
>             hideAnchor = true;
>           }
>           else {
>-            arrowbox.pack = popupRect.left + popupRect.width / 2 < anchorRect.left ? "end" : "start";
>+            let rtl = (window.getComputedStyle(this, "").direction == "rtl");

The second getComputedStyle argument is optional.

>+            arrowbox.pack = popupRect.left + popupRect.width / 2 < anchorRect.left ?
>+                              (rtl ? "start" : "end") : (rtl ? "end" : "start");

Not sure if it's easier to follow, but this would be shorter:

arrowbox.pack = (popupRect.left + popupRect.width / 2 < anchorRect.left) != rtl ?
                  "end" : "start";

Comment 50

[:Margaret Leibovic]

The patches here appear to be built on top of my patch in bug 597557, but I was waiting to land that until this bug is fixed to avoid the appearance of a regression on OSX. Enn, will you land our patches together when this is reviewed?

Comment 51

[Neil Deakin]

Only one of the patches https://bugzilla.mozilla.org/attachment.cgi?id=488199 is dependent on bug 597557.

But all can be checked in together if you like.

Comment 52

[[e-mail]]

(In reply to comment #29)
> (In reply to comment #26)
> > >+    case POPUPALIGNMENT_LEFTCENTER:
> > >+      pnt = nsPoint(anchorRect.x, anchorRect.y + anchorRect.height / 2);
> > >+      anchorRect.y = pnt.y;
> > >+      anchorRect.height = 0;
> > Why do you need to change the anchorRect?
> So that when the popup is flipped, it flips around the centreline rather than
> the edge of the anchor.
Oh, for some reason I thought this was the alignment of the popup. Silly me ;-)

Comment 53

[[e-mail]]

Comment on attachment 488189 [details] [diff] [review]
Part 1: add centering position types

>+    // if there is a space in the position, assume it is the anchor and
>+    // alignment as two separate tokens.
Sorry not to think of this before, but why can't the consumers switch to the popupanchor and popupalign attributes?

Comment 54

[Neil Deakin]

(In reply to comment #53)
> Comment on attachment 488189 [details] [diff] [review]
> Part 1: add centering position types
> 
> >+    // if there is a space in the position, assume it is the anchor and
> >+    // alignment as two separate tokens.
> Sorry not to think of this before, but why can't the consumers switch to the
> popupanchor and popupalign attributes?

They can, but the openPopup method only takes one position string and I don't really want to add another argument to it.

Comment 55

[:Margaret Leibovic]

I'm poking this bug, since it seems like it's pretty close to being resolved, but it's been a while since there's been any activity in here. It seems like we need a response from Neil.

Comment 56

[[e-mail]]

Comment on attachment 488189 [details] [diff] [review]
Part 1: add centering position types

Just noting that there are now too many ways of positioning a popup:

a) popupanchor and popupalign (where supported)
b) position (before/after/start/end)
c) position="popupanchor popupalign" (new for this patch)

Can we not at least deprecate one of them?

Comment 57

[Neil Deakin]

The first has been deprecated for a long time. Maybe it's time to remove support entirely. This is part of the reason for using c instead.

I don't really like having c as well as b, but I think the terminology of b is hard to understand.

Comment 58

[[e-mail]]

(In reply to comment #57)
> The first has been deprecated for a long time. Maybe it's time to remove
> support entirely. This is part of the reason for using c instead.
Ah, that makes much more sense now thanks!

Comment 59

[(no longer active)]

Please attach a folded patch containing the author and check-in comment lines.  It's not clear which patches need to land here, and in which order.

Comment 60

[gadjo]

I've just added a bug 616607 and set it blocking bug 595432, but maybe it's better to connect it with this bug (or maybe it will be solved with this bug)?

Comment 61

[Neil Deakin]

http://hg.mozilla.org/mozilla-central/rev/d0b7c821c18a
http://hg.mozilla.org/mozilla-central/rev/5ce0b1299094
http://hg.mozilla.org/mozilla-central/rev/fdaa466ab54f
http://hg.mozilla.org/mozilla-central/rev/44641ad32c29