822165 - Allow panels to always-flip on the "outside" edge of the panel

Status: RESOLVED INVALID

(Core :: XUL, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

Attachment: As described.

As a followup from bug 798226, the social flyouts desire a way to tell a panel it should always flip on the "outside" edge of the anchor.  In effect, this means that the offset specified for the panel should always be considered as related to the start of the anchor, even when flipped.  IOW, when the panel is flipped, the offset should not be considered as applying to the end of the anchor.  Further, the panel should *always* be considered flippable (which is not the default behaviour - there are cases where the panel can be moved but not flipped).

FWIW, as mentioned in bug 798226, I'm not convinced the existing behaviour WRT the offset is actually useful to anyone - if someone goes to the effort of calculating a specific anchor offset, that offset is almost certainly going to be wrong if the panel flips and the offset if applied against a different point.  But under the assumption that approach will not get traction, I'm opening this bug instead.

I'm attaching a patch which works for the social flyouts.  However, I'm not happy with how the new functionality is spelled.  This patch allows a panel to accept a new value for the "flip" attribute - "bothoutside".  The idea is that flipping is still allowed in both directions but will always be an "outside" flip (ie, arrow will always be relative to an offset from the anchor start position).  In addition, when this attribute has been specified, the panel can *always* be flipped.  There is no support for "bothinside".

Other things I considered would be to allow the "position" string to take a 3rd "token" - eg, 'position="topcenter topright outside"'.  Then the "flip" attribute could get similar treatment and allow something like 'flip="both always"'.

I'm looking for feedback on all aspects of this, and hopefully some better ideas about how to spell this feature.

See also the patch to bug 799014 which leverages this to give us the behaviour we need.

Comment 1

[Neil Deakin]

Comment on attachment 692826 [details] [diff] [review]
As described.

># HG changeset patch
># Parent 4721577f08c6d4542a4e8937ed2a711d317e9aac
># User Mark Hammond <[e-mail]>
>
>-    mFlipBoth = flip.EqualsLiteral("both");
>+    mFlipBoth = flip.EqualsLiteral("both") || flip.EqualsLiteral("bothoutside");
>+    mFlipOutside = flip.EqualsLiteral("bothoutside");

I think it would be clearer to use an single enumerated value for both flip cases instead of two booleans.


>+  FlipStyle maybeInside = mFlipOutside ? FlipStyle_Outside : FlipStyle_Inside;
...
>   if (popupAnchor >= POPUPALIGNMENT_LEFTCENTER) {
>     if (popupAnchor == POPUPALIGNMENT_LEFTCENTER ||
>         popupAnchor == POPUPALIGNMENT_RIGHTCENTER) {
>       aHFlip = FlipStyle_Outside;
>-      aVFlip = FlipStyle_Inside;
>+      aVFlip = maybeInside;
>     }
>     else {
>-      aHFlip = FlipStyle_Inside;
>+      aHFlip = maybeInside;
>       aVFlip = FlipStyle_Outside;
>     }
>   }
>   else {
>-    FlipStyle anchorEdge = mFlipBoth ? FlipStyle_Inside : FlipStyle_None;
>+    FlipStyle anchorEdge = mFlipBoth ? maybeInside : FlipStyle_None;
>     aHFlip = (popupAnchor == -popupAlign) ? FlipStyle_Outside : anchorEdge;
>     if (((popupAnchor > 0) == (popupAlign > 0)) ||
>         (popupAnchor == POPUPALIGNMENT_TOPLEFT && popupAlign == POPUPALIGNMENT_TOPLEFT))
>       aVFlip = FlipStyle_Outside;
>     else
>       aVFlip = anchorEdge;
>   }

If mFlipOutside is true, then both aHFlip and aVFlip will be set to true no matter what code path is used. Instead, just check if mFlipOutside is true and set both and do the existing code in an else block.


>   // If a panel is being moved, don't constrain or flip it. But always do this for
>   // content shells, so that the popup doesn't extend outside the containing frame.
>-  if (mInContentShell || !aIsMove || mPopupType != ePopupTypePanel) {
>+  if (mInContentShell || !aIsMove || mFlipOutside || mPopupType != ePopupTypePanel) {

I'm not convinced that it is a good idea to make flip="bothoutside" cause this unrelated behaviour change.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Bug 798226 gives us what we need here, so rejecting this one.