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.

See also bug 577928 comment 33-37.

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

(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

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...

(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.

(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.

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?)

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.

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.

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.

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)

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

Builds with these patches and those in various other bugs are at: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/neil@mozilla.com-37bee4c1be77/

(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/neil@mozilla.com-37bee4c1be77/ No windows builds there... :(

Some Windows builds are at http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/neil@mozilla.com-97b2f557b03c/tryserver-win32/

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.

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...

(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.

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

New builds will be available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/neil@mozilla.com-c981f59a91cb/

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.

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 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!]

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).

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

(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 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?

(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?'

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

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 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?

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

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

(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.

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.

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

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...

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.

(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).

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.

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

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.

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.

(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 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";

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?

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.

(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 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?

(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.

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 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?

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.

(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!

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.

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)?

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