Closed Bug 812943 Opened 7 years ago Closed 7 years ago

Panel anchor arrow never moves towards the middle of the panel

Categories

(Core :: XUL, defect)

16 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: markh, Assigned: markh)

References

Details

Attachments

(1 file, 1 obsolete file)

Consider the scenario where a panel is requested with a size roughly as wide as the window, but the anchor is in the middle of that window.

Expected: The panel is shown at the requested size and the anchor arrow is correctly positioned - meaning the anchor arrow is roughly 1/2 way along the panel.

Actual: The panel is resized (and possibly flipped) so the arrow remains at either the left or right hand size of the panel.  In the scenario above, this means the panel ends up roughly 1/2 as wide as it should be.
The anchor isn't supposed to be placed anywhere except at the left or right edge.

Is this in a test window? The popup will be resized to not extend outside the borders of the test window.
(In reply to Neil Deakin from comment #1)
> The anchor isn't supposed to be placed anywhere except at the left or right
> edge.

Yeah, I understand this is how the code is currently set up.  I guess the question is whether it can be changed?  I suspect that for many use-cases the ability to have the arrow move is more useful than having the requested size reduced by an arbitrary amount.

> Is this in a test window? The popup will be resized to not extend outside
> the borders of the test window.

This can be demonstrated in the test window using just the width/height of the window.  We have the same issue in the social chrome code where the screen width and height are the constraining factor.

For social, we really want the requested "flyout" (popup) size to be used if it can fit, and not be constrained to half the window height whenever we are anchoring the popup to an item 1/2 way down the screen.
I'd really like to help push this along - I've been requested to review a relatively hairy patch for the social code that works around this issue (bug 797209) but it makes far more sense to me to have this capability as a feature of panels.  We'd be happy if a new attribute was required to get this behaviour, which should mitigate any risk of panels getting this behaviour unintentionally.
What you want is that if the panel has to be moved to accomodate the window size, that the arrow shift position to remain at the same relative position?

I think the reason the arrow doesn't shift is because there isn't any way for the positioning code to know where the point of the arrow is (someone might create a theme that doesn't have any arrow at all but some other shape). The original implementation had different sizes for each platform as well. We could however just guess an assume that the centre of the arrow's box should be used even if that isn't correct.
(In reply to Neil Deakin from comment #4)

Yes, that all sounds correct and what we desire.
Enn,
  I'm playing with this and have a basic proof-of-concept 1/2 working, but before spending more time I'd appreciate some advice on the best way to proceed.  What I'm thinking is:

* nsMenuPopupFrame checks for a new attribute which enables this new "mode".

* SetPopupPosition does magic (see below) if this attribute is set.  It calculates an alignmentOffset which indicates how far off the alignment is from the "ideal" position - ie, how far the arrow needs to be moved.

* nsPopupBoxObject grows a new 'alignmentOffset' getter, much like the existing 'alignmentPosition' getter.  This would always return 0 if this mode isn't enabled.

* popup.xml, in adjustArrowPosition queries alignmentOffset, and applies that value to arrowbox.style.margin* (marginLeft/Top/Right/Bottom, depending on the alignment.)

SetPopupPosition magic:

There are a number of ways we could approach this.  I'm kinda leaning towards FlipOrResize being modified so that it still attempts to flip etc, but if the popup remains too large, calculate and store the alignmentOffset instead of resizing the popup.  The code will become more complex if we want to handle both flipping being enabled and disabled.  In this approach I'm thinking of a new attribute called, eg, loosealignment="true".

Another alternative might be to make no attempt at supporting both flipping and this new mode.  In this case, it might be better to create a new helper to be called instead of FlipOrResize.  This approach might even mean we could reuse the existing 'flip' attribute - support a new value - say flip="slide" or similar.

I'd appreciate your thoughts on this and how you would prefer things to look.
Flags: needinfo?(enndeakin)
Neil, a polite and gentle ping for the requested info - thanks!
This patch is on top of the patch in bug 798226.

As a strawman, the new behaviour of the panel happens if the flip attribute is set to "slide" (as it causes the arrow to "slide" along the panel).  On such panels, instead of resizing the panel when it can't fit on either side of the panel, it instead calculates an "alignment offset", which is the offset which needs to be applied to the arrow such that it is still aligned to the anchor.  This new behaviour only happens on one axis (depending on the alignment) - the other axis is flipped or resized as now.  The alignment offset is exposed via nsPopupBoxObject, and popup.xml grabs this value and applies it as a margin on the arrow itself.

It includes tests and seems to work quite well.  It should be relatively "safe" as this new behaviour is opt-in (via setting flip="slide").  There does appear to be one or 2 issues when I use it in bug 797209 and I'll continue to look into it, but feedback on this approach would be great.
Assignee: nobody → mhammond
Attachment #741184 - Flags: feedback?(neil)
Flags: needinfo?(enndeakin)
Comment on attachment 741184 [details] [diff] [review]
Optionally allow the arrow to "slide" rather than resizing the panel

>+  mAlignmentOffset = nsPoint(0, 0);
Why is this a point when you only ever use one coordinate at a time?

>+    bool slideHorizontal = mSlide && mPosition != POPUPPOSITION_UNKNOWN
>+                           && mPosition <= POPUPPOSITION_AFTEREND;
>+    bool slideVertical = mSlide && !slideHorizontal;
I would prefer mSlide && mPosition >= POPUPPOSITION_STARTBEFORE and I'm not clear why you need to check for POPUPPOSITION_UNKNOWN given that your slideVertical would become true in that case.

>+  // check if the popup can fir into the available space by "sliding" (ie, by
[Spelling nits: fit, i.e.]

>+              this.adjustArrowPosition();
I think you must have worked out that you didn't need this, but then forgot to remove it because the exception got eaten by the try/catch.

>+        // if this panel has a "sliding" arrow, we may have previously set margins...
>+        for (let style of ["margin-left", "margin-right", "margin-top", "margin-bottom"])
>+          arrowbox.style.removeProperty(style);
Why not just remove the "margin" property?
Attachment #741184 - Flags: feedback?(neil) → feedback+
(From update of attachment 741184 [details] [diff] [review])
>+              this.adjustArrowPosition();
Oh, this is from bug 798226, but you forgot to mark the dependency.
Depends on: 798226
(In reply to neil@parkwaycc.co.uk from comment #9)
> Comment on attachment 741184 [details] [diff] [review]
> Optionally allow the arrow to "slide" rather than resizing the panel
> 
> >+  mAlignmentOffset = nsPoint(0, 0);
> Why is this a point when you only ever use one coordinate at a time?

It was simply so I could use ToNearestPixels(), which is only available on point/rect-ish objects.  This patch takes a slightly different approach - it uses an nscoord for mAlignmentOffset, and uses a temp rect in nsPopupBoxObject::GetAlignmentOffset() so it can perform the conversion to device pixels.

> >+    bool slideHorizontal = mSlide && mPosition != POPUPPOSITION_UNKNOWN
> >+                           && mPosition <= POPUPPOSITION_AFTEREND;
> >+    bool slideVertical = mSlide && !slideHorizontal;
> I would prefer mSlide && mPosition >= POPUPPOSITION_STARTBEFORE

It now reads:

    bool slideHorizontal = mSlide && mPosition <= POPUPPOSITION_AFTEREND;
    bool slideVertical = mSlide && mPosition >= POPUPPOSITION_STARTBEFORE;

which I *think* is what you were asking for?

> and I'm not clear why you need to check for POPUPPOSITION_UNKNOWN given that your
> slideVertical would become true in that case.

I don't :)  Removed.

> >+              this.adjustArrowPosition();
> I think you must have worked out that you didn't need this, but then forgot
> to remove it because the exception got eaten by the try/catch.

Yes, thanks - all of the new calls to adjustArrowPosition() have been removed (ie, the existing calls are all that is necessary)

> 
> >+        // if this panel has a "sliding" arrow, we may have previously set margins...
> >+        for (let style of ["margin-left", "margin-right", "margin-top", "margin-bottom"])
> >+          arrowbox.style.removeProperty(style);
> Why not just remove the "margin" property?

Mainly as I didn't know that would work - I should have guessed it would though, thanks.  New patch does exactly that.

Anything not mentioned has been fixed.
Attachment #741184 - Attachment is obsolete: true
Attachment #746228 - Flags: review?(neil)
(In reply to Mark Hammond from comment #11)
> (In reply to comment #9)
> > (From update of attachment 741184 [details] [diff] [review])
> > >+              this.adjustArrowPosition();
> > I think you must have worked out that you didn't need this, but then forgot
> > to remove it because the exception got eaten by the try/catch.
> Yes, thanks - all of the new calls to adjustArrowPosition() have been
> removed (ie, the existing calls are all that is necessary)
Actually my original confusion was that I saw this patch before the one in bug 798226, but you are still correct that the new calls are unnecessary, as the position is adjusted anyway via the popupshowing event.
Comment on attachment 746228 [details] [diff] [review]
Updated based on comments

>+  int32_t pp = nsDeviceContext::AppUnitsPerCSSPixel();
>+  // Note that the offset might be along either the X or Y axis, but for the
>+  // sake of simplicity we use a point with only the X axis set so we can
>+  // use ToNearestPixels().
It's not as if app units are suddenly going to stop being square ;-)
Attachment #746228 - Flags: review?(neil) → review+
https://hg.mozilla.org/mozilla-central/rev/89ae725c0125
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Depends on: 883136
Component: XP Toolkit/Widgets: Menus → XUL
You need to log in before you can comment on or make changes to this bug.