874792 - Menu panel isn't correctly attached to its anchor

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[u428464]

I think it might be related to bug 870989, in latest UX builds the menu panel isn't correctly attached to the menu button in maximized mode. It's a recurrent problem with Firefox panels (bug 871155) , but here it's more important since the menu button is always on the right side of the navbar.

Comment 1

[u428464]

Attachment: Menu panel incorrectly attached to its anchor

As seen here the panel is pushed by the edge of the screen. As said in bug 871155 I think the best would be to have panels always attached to their respective anchors and never get pushed when near the edge of the screen or off-screen (panels should go off-screen and always point the center of their anchors e.g. Chrome).

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: WIP: allow panels to be positioned past the edges of the screen

Note: This patch does NOT work in RTL mode yet :(

Comment 3

[Mike de Boer [:mikedeboer]]

Comment on attachment 764849 [details] [diff] [review]
WIP: allow panels to be positioned past the edges of the screen

Jared, are you the man to ask for feedback here?

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 764849 [details] [diff] [review]
WIP: allow panels to be positioned past the edges of the screen

Review of attachment 764849 [details] [diff] [review]:
-----------------------------------------------------------------

Neil might be able to provide more feedback on this.

::: layout/xul/base/src/nsMenuPopupFrame.cpp
@@ +1414,5 @@
> +  // Keep a 3 pixel margin to the right and bottom of the screen for the WinXP dropshadow
> +  int32_t offset = -3;
> +  if (!mIgnoreScreenEdge) {
> +    // If a popup dares to ignore screen edges, we'll make sure it will
> +    offset = 1000;

Where does 1000 come from? It seems quite arbitrary :)

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Removing the items from M7 that do not block landing on m-c.

Comment 6

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] from comment #4)]
> 
> Where does 1000 come from? It seems quite arbitrary :)

Exactly! Hence the request for feedback. I'm not sure about a number of things, mostly I'm in search of a more generic solution that would work for RTL mode as well. There are two parts of positioning code that I now shield off with a boolean flag, which is not optimal. The x/y positioning is tricky to intercept correctly... in LTR mode it's just the width and height.

Neil, I'm looking forward to your fresh insights! ;)

Comment 7

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Adding "the other Neil" for his insights...

Comment 8

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Attachment: Demonstrate how the downloads panel can also be mis-aligned with the button in contrived edge-cases

The 3 pixel restriction for XP seems somewhat arbitrary and I don't understand why we enforce it at all.  It seems better in all cases to sacrifice the drop-shadow rather than the popup size - I can't see how a user would be confused or upset if an element abuts the edge of the screen such that the shadow isn't seen.  In a multi-monitor setup it's always possible the drop-shadow *would* be seen on a different monitor anyway.

Dropping this shadow doesn't seem a generic solution anyway.  Eg, let's say the window isn't maximized, but instead has been positioned such that the button itself abuts the edge of the screen (ie, so a small portion of the window is either cropped or on a second monitor).  Do we still want the arrow to be centered on the button (and therefore the panel itself is either cropped or across 2 screens?)  IMO, the answer to that would be "yes".

To demonstrate this, see the screen-shot, where the panel is mis-placed WRT the "downloads" button.  I reproduced this by moving the Fx window such that the downloads button was split between 2 monitors - and given this, I think it would be perfectly reasonable for the panel to also split between the 2.  Even if I didn't have the second monitor so the downloads button was simply cropped, I think it would also be fine for the panel to also be cropped.  IIUC, the fact that OSX allows different DPI settings per screen could be an interesting challenge here though :)

So, I guess my point is: in these edge cases, you are either going to need to accept that the arrow will not *always* be correctly centered, or the patch will need to do more than drop the 3px edge and arrange to modify the code that constrains to the edge of the screen (which is FlipOrResize IIUC)

Comment 9

[Neil Deakin]

Comment on attachment 764849 [details] [diff] [review]
WIP: allow panels to be positioned past the edges of the screen

>+  /** 
>+   * Allow the popup position to cross screen boundaries and stay uncorrected.
>+   */
>+  attribute boolean ignoreScreenEdge;
>+

This would be better done as an attribute on the popup element.


>   if (mInContentShell || !aIsMove || mPopupType != ePopupTypePanel) {

Why not just check mIgnoreScreenEdge on this line, since it skips all of the constraint checks? Is there specific aspects on constraining that you want and others that you do not want?

Note also that when mInContentShell is true, the popup must be constrained as content popups aren't allowed to extend outside of the content window area.

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to Neil Deakin from comment #9)
> Comment on attachment 764849 [details] [diff] [review]
> WIP: allow panels to be positioned past the edges of the screen
> 
> >+  /** 
> >+   * Allow the popup position to cross screen boundaries and stay uncorrected.
> >+   */
> >+  attribute boolean ignoreScreenEdge;
> >+
> 
> This would be better done as an attribute on the popup element.
> 

As it is now, I implemented it as a property on the popup (which IIRC can also be invoked as an attribute 'ignorescreenedge="true"'), which is picked up by the interfaces it inherits - I needed the property setter to bubble up to the actual implementation that enforces the restraints.

> 
> >   if (mInContentShell || !aIsMove || mPopupType != ePopupTypePanel) {
> 
> Why not just check mIgnoreScreenEdge on this line, since it skips all of the
> constraint checks? Is there specific aspects on constraining that you want
> and others that you do not want?

I believe I tried that and led to all kinds of weirdness. Will try again, sir.

> 
> Note also that when mInContentShell is true, the popup must be constrained
> as content popups aren't allowed to extend outside of the content window
> area.

Noted. ;)

Comment 11

[Mike de Boer [:mikedeboer]]

(In reply to Mark Hammond (:markh) from comment #8)
> Created attachment 765147 [details]
> Demonstrate how the downloads panel can also be mis-aligned with the button
> in contrived edge-cases
> 
> The 3 pixel restriction for XP seems somewhat arbitrary and I don't
> understand why we enforce it at all.  It seems better in all cases to
> sacrifice the drop-shadow rather than the popup size - I can't see how a
> user would be confused or upset if an element abuts the edge of the screen
> such that the shadow isn't seen.  In a multi-monitor setup it's always
> possible the drop-shadow *would* be seen on a different monitor anyway.
> 

When I saw the '3px' correction for XP, I immediately flagged it as a hack. We *could* make an effort to dig up the bug no. (of ancient times, no doubt) to see if the assertions made there still hold true.

However, I did not introduce the 3px rule here. What I'm trying to do is quite different. If I can remove the hardcoded values in one go, that would be great tho, since I'm touching the code there at this point.

Comment 12

[Mike de Boer [:mikedeboer]]

Attachment: v1: Allow panels to be positioned past the edges of the screen

Neil, I implemented your suggestions, except for the attribute one (see comment 10). Meanwhile I've figured out that it does need to something else if we were to make it an attribute, instead of a property - following the implementation of the "consumeoutsideclicks" attribute.

I think it's ready for the first round of review.

Comment 13

[Dão Gottwald [:dao]]

(In reply to Mark Hammond (:markh) from comment #8)
> Do we still want the
> arrow to be centered on the button (and therefore the panel itself is either
> cropped or across 2 screens?)  IMO, the answer to that would be "yes".

Why? While it would be aesthetically pleasing to have the arrow centered on the button, the panel being split or even cropped doesn't seem useful.

Comment 14

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Mike de Boer [:mikedeboer] from comment #11)
> When I saw the '3px' correction for XP, I immediately flagged it as a hack.
> We *could* make an effort to dig up the bug no. (of ancient times, no doubt)
> to see if the assertions made there still hold true.
> 
> However, I did not introduce the 3px rule here. What I'm trying to do is
> quite different.

Sorry, I must have misunderstood - I thought that 3px rule was the one that was actually biting you and what was preventing the panel from being hard against the right edge of the screen and thus causing the offset of the arrow from the anchor.

(In reply to Dão Gottwald [:dao] from comment #13)
> (In reply to Mark Hammond (:markh) from comment #8)
> > Do we still want the
> > arrow to be centered on the button (and therefore the panel itself is either
> > cropped or across 2 screens?)  IMO, the answer to that would be "yes".
> 
> Why? While it would be aesthetically pleasing to have the arrow centered on
> the button, the panel being split or even cropped doesn't seem useful.

In that screen-shot I uploaded, the arrow is pointing at a completely different widget *and* the panel is cropped such that the word "Downloads" bleeds out of the panel.  I believe what I described would indeed be an improvement over that.

Comment 15

[Mike de Boer [:mikedeboer]]

The patch I uploaded yesterday has a serious downside: if applied and a panel chooses to ignore screen edges, no position correction will be performed at all. This also means that the panel won't be flipped when it reaches the leftmost edge, which is very common in RTL mode for the Australis Panel.
In other words, this patch requires panel consumers to implement the flip themselves. In the case of the Australis panel I think it'd be sufficient to pass the arrow position as 'topleft', instead of 'topright'.

Comment 16

[Mike de Boer [:mikedeboer]]

It seems I posted my previous comment too soon... the panel implementation still corrects arrow positioning (not related to panel box positioning) in RTL mode.

Comment 17

[Neil Deakin]

Comment on attachment 765253 [details] [diff] [review]
v1: Allow panels to be positioned past the edges of the screen

No, attributes should be used for these, not boxobject properties, as all other popup flags/features are implemented this way. Properties/methods should only be added to nsIPopupBoxObject that must be implemented in native code. Attributes have the advantage that one doesn't need to use script to change the setting, nor does one have to be concerned about potential confusing errors occurring because the value gets reset if the popupframe goes away.

You can add a getter/setter for the attribute in popup.xml if you like though.

You should be able to just either call AttrValueIs when needed, or cache the 'ignorescreenedge' attribute (note that attributes are all lowercase) within the various InitializePopup methods.

Comment 18

[Justin Dolske [:Dolske]]

Arguably P3, but going with P4 since it only happens in certain situations.

Comment 19

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Let me try and re-phrase comment 8 :)

(In reply to Dão Gottwald [:dao] from comment #13)
> Why? While it would be aesthetically pleasing to have the arrow centered on
> the button, the panel being split or even cropped doesn't seem useful.

The currently attached patch does exactly this.  If the anchor got too close to the edge, the popup would either clip (in the single monitor case) or split across 2 monitors).

Is that the intent?

If not, I believe that dropping the 3px edge might solve the use-case in the screenshot without the crop/split side-effect - the offset from the center of the anchor is ~= to the gap between the right of the panel and the right edge of the screen.

Also, note that the current patch would also avoid "flipping".  If the user could customize the UI such that the anchor could be on the left of the screen rather than the right, you would end up with *most* of the panel clipped rather than it flipping so most is visible.  I'm pretty sure that is an undesirable side-effect (ie the patch should still enter "FlipOrResize", but just do the "flip" part and not the "resize" part.)

Comment 20

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Attachment: remove 3px XP drop-shadow allowance + css tweaks.

I had a bit more of a play with this.  Dropping the 3px edge gets very close - close enough that some additional CSS tweaks should be able to come up with an acceptable solution.

This patch drops the 3px border for the Windows XP drop-shadow, and slightly reduces the margin of the panel arrow via CSS.  This isn't 100% pixel-perfect, but is very close and further CSS tweaks should be able to come up with an acceptable solution.  This is just FYI and to demonstrate a lighter-weight solution might be suitable.

Comment 21

[u428464]

The panel is always attached to it anchor in Google Chrome and it almost never crops between two screens.

Comment 22

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Guillaume C. [:ge3k0s] from comment #21)
> The panel is always attached to it anchor in Google Chrome and it almost
> never crops between two screens.

The panels always have some number of pixels between the "point" of the arrow and the edge of the panel itself.  If this distance is greater than the distance from the middle of the anchor to the edge of the screen, then there is no choice other than to (a) crop/split the panel, (b) accept that the arrow can't point at the middle of the anchor, or (c), reduce the distance between this "arrow point" and the panel edge.

The most recent attachment I uploaded does (c) - it reduces the distance from the arrow-point to the panel-edge, which allows the arrow point to be at the center of the button without needing to crop.  It also removes an artificial restriction that the panel can't be closer than 3px from the right of the screen, which also helps.

Comment 23

[u428464]

Ok thanks for the precision. ;-) What will happen if the anchor is partially off-screen and you click on it ? (one of the only case where Chrome splits a little part of the panel).

Comment 24

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Guillaume C. [:ge3k0s] from comment #23)
> Ok thanks for the precision. ;-) What will happen if the anchor is partially
> off-screen and you click on it ? (one of the only case where Chrome splits a
> little part of the panel).

Currently, the panel will crop *and* point at the wrong element entirely - see the screenshot I previously attached.

The patch that mdeboer added means that the panel would split/crop (although it probably needs a little more work so the panel will still "flip" if it can).  However, it doesn't solve the underlying issue I mentioned in comment 22 - given there simply aren't enough pixels between the arrow point and the panel edge, it will crop this panel (ie, it will point at the middle of the button, but will crop/split the right-hand border of the panel)

So IMO, we do want *both* strategies:

(1) Something like my CSS patch so that in the usual, common case, we can show that panel without cropping/splitting.

(2) Something like mdeboer's patch so that in edge-cases (such as when the button is artificially moved very close to the screen edge), the panel can crop/split without causing the arrow to point at the wrong element and without causing "bleeding" like the screen-shot I attached demonstrates.  This is the case you mention when Chrome splits.  Note however that Dao seems to disagree with this in comment 13.

Even though we want both IMO, it is (1) that is probably the most critical here for Australis as it deals with the common, usual case.

Comment 25

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Enn and I just had a very brief chat about this.  Given the existence of bug 615476 (which is calling for an even larger distance between the arrow and the panel edge), this problem is going to become even worse, and is likely to start impacting panels other than just this "customize" one.  We feel that it probably makes sense to build into the panels the ability to detect this situation and automatically "slide" the arrow closer to the edge when necessary.

There is already some support for "sliding" (via bug 812943) which might be able to be refactored to help fix this (specifically, the existing support only kicks-in when a special attribute is set, and also avoids "flipping", so things would have to be rejigged such that the behaviour becomes the default (ie, a new attribute might be introduced to *prevent* the new behaviour) and the panel still flips if necessary, and after the flip we examine the screen position to determine if a "slide" is necessary.)

Comment 26

[:Gijs (he/him)]

Note that the automatic cropping that currently occurs on OS X (where, if I understood Mark correctly, the OS repositions the panel to be fully onscreen (maybe only in the single-monitor case?) and we then set height/width attributes) causes bug 879500. The width cropping also intereferes with the new help/customize button styling (bug 877684), because the margins/paddings get clipped and the help button looks very asymmetrical. 

Is this bug going to fix these issues? Comment #25 makes it sound like it might, but I can't tell for sure.

Comment 27

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to :Gijs Kruitbosch from comment #26)
> Note that the automatic cropping that currently occurs on OS X (where, if I
> understood Mark correctly, the OS repositions the panel to be fully onscreen
> (maybe only in the single-monitor case?)

It's not clear to me that this is actually the case.  I think it is *us* who arranges for the panel to be moved such that it remains fully on screen.  Given we arrange for it to never be off screen, the OS should never find it necessary to move it due to it being off-screen.

It's possible there is a bug in our code for keeping it on-screen, or it's possible there are other restrictions OSX places on window positioning (eg, maybe OSX will not let it get within, say, 5px of the edge and move it if it does.)  Windows never enforces anything like that, which might explain why bug 879500 can't be seen there currently (although the fact it *could* be seen in the past makes me skeptical we are talking about the same thing) 

> and we then set height/width attributes) causes bug 879500. 

Given the above, that's not quite clear to me yet - as it is us adjusting the position, we shouldn't see the move as "unexpected" causing those attributes to be set.

> Is this bug going to fix these issues? Comment #25 makes it sound like it
> might, but I can't tell for sure.

I'm really not sure, but I wouldn't expect it to fix bug 879500.  There are 2 possible outcomes from this bug:

* We tweak borders and margins etc such that we keep the arrow in the correct place while still arranging the keep the entire panel on-screen.  But given we already believe we are keeping the entire panel on-screen now, the reason why bug 879500 occurs remains a mystery, and thus wouldn't be fixed by this.

* In some cases we decide to move the panel slightly off-screen such that the arrow can be placed correctly.  Now, if OSX would then ignore our request to place it slightly off-screen and move it back on-screen, then obviously we would find ourself with this bug still occurring (ie, the arrow would not be positioned correct), *and* bug 879500 happening (as the OS moving the panel would have caused those sticky 'height' and 'width' attributes).

I think we need to determine the root cause for bug 879500 first, independently of this bug.

Comment 29

[Mike de Boer [:mikedeboer]]

Attachment: v2: Allow panels to be positioned past the edges of the screen

Using an attribute now.

Comment 30

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Mike de Boer [:mikedeboer] from comment #29)
> Created attachment 774803 [details] [diff] [review]

Mike,
  I'm wondering if you have any thoughts on comment 25, and specifically:

* If the customize button gets close to the left of the screen, the panel doesn't "flip" as would be expected, but instead crops so the panel isn't fully visible.

* This patch isn't going to work as desired with bug 615476 - in that case we would see it cropped on the right-hand edge of the screen.

Comment 31

[u428464]

(In reply to Mark Hammond (:markh) from comment #30)
> * This patch isn't going to work as desired with bug 615476 - in that case
> we would see it cropped on the right-hand edge of the screen.

I think the plan here was to inset the panels more only when possible (1. if the anchor isn't at the edge of the screen 2. If the browser is in restored mode and not at the edges too). The best would be to have this patch and the sliding arrow you mentioned before.

Comment 32

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Guillaume C. [:ge3k0s] from comment #31)
> I think the plan here was to inset the panels more only when possible

That "when possible" is what is referred to in comment 25 as "sliding".  The existing, limited sliding support relies on CSS - and bug 615476 should ideally also be a CSS-only tweak.

So IMO, the best way forward here would be to:

* fix this specific bug using just CSS if possible - something like attachment 767768 [details] [diff] [review].

* fix bug 615476 by improving the "sliding" support (ie, the "when possible" part), plus adding additional CSS tweaks for the default inset.

Comment 33

[Dão Gottwald [:dao]]

(In reply to Mark Hammond (:markh) from comment #32)
> So IMO, the best way forward here would be to:
> 
> * fix this specific bug using just CSS if possible - something like
> attachment 767768 [details] [diff] [review].
> 
> * fix bug 615476 by improving the "sliding" support (ie, the "when possible"
> part), plus adding additional CSS tweaks for the default inset.

and probably:

* reduce the arrow size on OS X (Windows and Linux were taken care of in bug 764755 and bug 765714)

Comment 34

[Mike de Boer [:mikedeboer]]

(In reply to Mark Hammond (:markh) from comment #30)
> Mike,
>   I'm wondering if you have any thoughts on comment 25, and specifically:
> 
> * If the customize button gets close to the left of the screen, the panel
> doesn't "flip" as would be expected, but instead crops so the panel isn't
> fully visible.
> 
> * This patch isn't going to work as desired with bug 615476 - in that case
> we would see it cropped on the right-hand edge of the screen.

The 'ignorescreenedges' attribute I introduce here is dead-simple and only 'fixes' the following case; the panel won't be cropped, flipped, etc, but instead will be left alone. Nasty side-effect is that part of the panel will be displayed off-screen IF there's no secondary monitor attached (which I suspect is the mode of our users).

Ideally, the following should be the behavior of panels:
1) ignore screen edges always as soon as we detect that the panel can and will be displayed on a secondary monitor.
2) if no secondary monitor is present, screen edges should be respected; no cropping should ever happen; the panel arrow should move to stay attached to its anchor at all times, unless the anchor itself is displayed (partly) off-screen. This is in accordance with bug 615476.

IMO, that's what needs to done in this bug, which renders my patch/ attempt 'invalid' as well, I'm afraid.

Neil, what do you think? What are the (rough) steps to take to get to that state?

Comment 35

[Neil Deakin]

(In reply to Mike de Boer [:mikedeboer] from comment #34)
> Ideally, the following should be the behavior of panels:
> 1) ignore screen edges always as soon as we detect that the panel can and
> will be displayed on a secondary monitor.
> 2) if no secondary monitor is present, screen edges should be respected; no
> cropping should ever happen; the panel arrow should move to stay attached to
> its anchor at all times, unless the anchor itself is displayed (partly)
> off-screen. This is in accordance with bug 615476.
> 

I assume you mean to only ignore the edges if there has been placed such that it is on the same side where the popup is opening.

Why would we want to add code for this uncommon case when flipping the panel is a reasonable solution?

Comment 36

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Mike de Boer [:mikedeboer] from comment #34)

Thanks Mike.  I agree completely with this, but there is a subtlety I'm advocating (and which I only understood since this bug was opened, so haven't been very clear.)

> Ideally, the following should be the behavior of panels:
> 1) ignore screen edges always as soon as we detect that the panel can and
> will be displayed on a secondary monitor.

The subtlety is that (1) above is somewhat flexible, and should be able to be expressed as CSS.  I'm advocating (1) be we-written as:

1.1) If things don't quite fit, see if we can flip and improve things.  If so, we are done.  But note that for this specific bug, we can't flip - we are already in the best orientation.

1.2) See if we can "slide" the arrow towards the closest edge.  Bug 615476 wants to give us alot of gap we can reduce - but even without that we have enough gap to solve this specific bug - and can express that in just CSS, just like the existing sliding support does.

IOW - we can fix this for this specific bug by hard-coding some CSS - something like attachment 767768 [details] [diff] [review].  We can then fix it in the general case by improving the existing "sliding" support - which is *still* a CSS-heavy fix. 

> 2) if no secondary monitor is present, screen edges should be respected; no
> cropping should ever happen; the panel arrow should move to stay attached to
> its anchor at all times, unless the anchor itself is displayed (partly)
> off-screen. This is in accordance with bug 615476.

And then, yeah! If all else fails, IMO it's better to do the split/crop thing than it is to resize.  But this should be a last resort, only when we can't ccerce a decent looking panel and arrow with the correct content area size.

Comment 37

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Or to put it yet another way: the requirements of this panel are not special - it should not be necessary to add a new special attribute just to get correct, sane behaviour.

Comment 38

[Mike de Boer [:mikedeboer]]

Attachment: Patch: use state flag attributes to allow CSS styling in different situations

Mark, this is stab at it with your patch as a starting point.

Since I REALLY want to know in which state the panel is in when I open it, I decided to set attributes that expose what nsMenuPopupFrame has been up to internally, like 'what the *beep* happened in SetPopupPosition()?' or 'Did it flip now, or slide, or bounce or do-the-hamsterdance?'

Please let me know what you think!

Comment 39

[Dão Gottwald [:dao]]

Comment on attachment 781757 [details] [diff] [review]
Patch: use state flag attributes to allow CSS styling in different situations

>+#PanelUI-popup[position-adjusted] .panel-arrow {
>+  -moz-margin-end: 2px;
>+}

This should use the child selector.

Why can we not just change the arrow margin for this particular panel without the position-adjusted fiddling?

Comment 40

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [:dao] from comment #39)
> This should use the child selector.

I know it should; WIP.

> Why can we not just change the arrow margin for this particular panel
> without the position-adjusted fiddling?

I tried, which correctly positions the panel when the panel touches the screen edge, but doesn't work when the panel position/ size is not adjusted; the arrow is positioned too far from its anchor.

Comment 41

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 781757 [details] [diff] [review]
Patch: use state flag attributes to allow CSS styling in different situations

Review of attachment 781757 [details] [diff] [review]:
-----------------------------------------------------------------

TBH I'm not that keen on adding attributes which are only used to satisfy your curiosity ;)  See test_popupAnchor.xul - this tests (most?) all aspects of flipping and resizing without needing these attributes - they can simply be deduced from examining what position/orientation was requested versus what was actually done.  I probably wouldn't support this test using these attributes as currently they test more than simply "is it flipped" or "was the position adjusted" - they check exactly what flip was done and the exact pixels for the adjustment.

So just cancelling feedback for now - if you can find some real use for these attributes that clearly makes code easier to understand or write, I'd be up for it, but I'm mildly against it as purely a diagnostic tool for this single issue that risks getting stale or becoming even less relevant over time.  I'd recommend having them as a separate patch in your queue which you can discard once this panel issue is resolved and no other panel issues remain on your radar.

Comment 42

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Mark Hammond (:markh) from comment #41)
> Comment on attachment 781757 [details] [diff] [review]
> Patch: use state flag attributes to allow CSS styling in different situations
> 
> Review of attachment 781757 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> TBH I'm not that keen on adding attributes which are only used to satisfy
> your curiosity ;)

Sorry, this comment doesn't make much sense.  You use this attribute in the CSS, but like Dao, I'd really like to understand why it can't be done without this.  I see this as very closely related to bug 615476 - in both cases, we need a way to "slide" the arrow so it can become very close to the edge of the panel without it actually resizing or repositioning.  The limited sliding support that does exist touches the margin of the arrowbox (see adjustArrowPosition in popup.xml) whereas my patch here (and yours) touch the arrow - so maybe experimenting with the arrowbox margin will work better?  Either way, I think this bug, the existing sliding support and bug 615476 are all manifestations of the same problem and they all should be able to use an identical strategy that need not involve new custom attributes.

Comment 43

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

I fear my contribution in this bug has been mainly stop energy :(

The end-game for this bug should be, IMO, as I write in comment 42.  However, I understand Australis might want something before we can achieve that.  So if the timing dictates, I'd support something like Mike's most recent patch here, plus a new bug that captures the implementation of bug 615476 and the removal of any "hacks" introduced here into a single "better sliding" solution.

Comment 44

[Mike de Boer [:mikedeboer]]

Comment on attachment 781757 [details] [diff] [review]
Patch: use state flag attributes to allow CSS styling in different situations

Mark! It's been a while, but I'd like to revisit this bug :)

What would you like me to change in this patch to make it acceptable, as you stated in comment 43?
I'm asking this specifically, because it's quite hard for me to extract this from the conversation above.

Of course, I will take responsibility for (filing) the necessary follow-ups.

Comment 45

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 781757 [details] [diff] [review]
Patch: use state flag attributes to allow CSS styling in different situations

f+ in the interests of pragmatism and under the assumption that landing this is time critical.  I think a followup bug should be to back this change out and to have FlipOrResize handle this case and set alignmentOffset appropriately, then have adjustArrowPosition() (in popup.xml) tweak the margin automatically - it already has margin-tweaking code in place to handle when the arrow slides towards the middle of the panel - this is conceptually the same, except we are sliding towards the edge.

Comment 46

[Neil Deakin]

Comment on attachment 781757 [details] [diff] [review]
Patch: use state flag attributes to allow CSS styling in different situations

>+    if (mHFlip || mVFlip) {
>+      mContent->SetAttr(kNameSpaceID_None, nsGkAtoms::flipped,
>+                        NS_LITERAL_STRING("true"), true);
>+    } else {
>+      mContent->UnsetAttr(kNameSpaceID_None, nsGkAtoms::flipped, true);
>+    }
>+    if (mPositionAdjusted) {
>+      mContent->SetAttr(kNameSpaceID_None, nsGkAtoms::positionAdjusted,
>+                        NS_LITERAL_STRING("true"), true);
>+    } else {
>+      mContent->UnsetAttr(kNameSpaceID_None, nsGkAtoms::positionAdjusted, true);
>+    }
>+

It isn't safe to change attributes here. If a listener removes the popup from the document, a crash will occur when accessing this frame's members. You would need to use an event here similar to how nsMenuFrame sets -moz-menuactive.

You don't use the flipped attribute anywhere. Is that for another patch?

Comment 49

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: introduce 'popupPositionAdjusted' event to let consumers know when this happens

Comment 52

[Neil Deakin]

Comment on attachment 8336182 [details] [diff] [review]
Patch 2: introduce 'popupPositionAdjusted' event to let consumers know when this happens

> const PanelUI = {
>   /** Panel events that we listen for. **/
>-  get kEvents() ["popupshowing", "popupshown", "popuphiding", "popuphidden"],
>+  get kEvents() ["popupshowing", "popupshown", "popuphiding", "popuphidden", "popupPositionAdjusted"],

Event names should be all lowercase.



>   mVFlip = false;
>   mHFlip = false;
>   mAlignmentOffset = 0;
>+  mPositionAdjusted = false;

You don't initialize this in any of the other two Initialize methods. For completeness, it should also be initialized in the constructor.

> 
>   // if aAttributesOverride is true, then the popupanchor, popupalign and
>   // position attributes on the <popup> override those values passed in.
>   // If false, those attributes are only used if the values passed in are empty
>   if (aAnchorContent) {
>     nsAutoString anchor, align, position, flip;
>     mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::popupanchor, anchor);
>     mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::popupalign, align);
>@@ -750,16 +751,20 @@ nsMenuPopupFrame::ShowPopup(bool aIsCont
>         return;
>     }
> 
>     // do we need an actual reflow here?
>     // is SetPopupPosition all that is needed?
>     PresContext()->PresShell()->FrameNeedsReflow(this, nsIPresShell::eTreeChange,
>                                                  NS_FRAME_HAS_DIRTY_CHILDREN);
> 
>+    if (mPositionAdjusted) {
>+      FireDOMEvent(NS_LITERAL_STRING("popupPositionAdjusted"), mContent);
>+    }

mPositionAdjusted is set by SetPopupPosition which won't have been called yet. The call a line earlier to FrameNeedsReflow just marks the frame for reflow but the actual reflow happens later, so at this point you won't know the final size of the popup.


I've lost track of what this bug is about. Can you summarize or point to what specific case or cause requires the 2 pixel extra margin?

Comment 53

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Neil Deakin from comment #52)
> I've lost track of what this bug is about. Can you summarize or point to
> what specific case or cause requires the 2 pixel extra margin?

If the Fx window is maximized, the panel shown when the hamburger button is pressed is not quite centered under the button as the panel is at the right edge of the screen.  FTR, I was advocating FlipOrResize() take this into account and have adjustArrowPosition() tweak the margins accordingly.

Comment 54

[Markus Stange [:mstange]]

I've offered to take this.

Comment 55

[Markus Stange [:mstange]]

Attachment: Part 1: Remove shadow offset.

Comment 56

[Markus Stange [:mstange]]

Attachment: Part 2: Attempt to use sliding for the menu panel.

Correcting the arrow position after the position adjustment is implemented for sliding panels. I see no reason to prefer flipping over sliding for this panel, so let's try to use it.

Comment 57

[Markus Stange [:mstange]]

Attachment: Part 3: Support sliding for the menu panel.

The panel is opened with position "bottomcenter topright", which causes us to go the InitPositionFromAnchorAlign route in InitializePopup, which sets mPosition POPUPPOSITION_UNKNOWN, so we disable sliding.
If we base the sliding position on the return value of GetAlignmentPosition(), it works.

Comment 58

[Markus Stange [:mstange]]

Attachment: Part 4: Make adjustment sign consistent and use a transform for moving the arrow.

Sometimes we adjusted in the wrong direction. This makes the code more readable and more consistent: When the panel ends up more to the left than intended, the offset is negative, and when it ends up more to the right the adjustment is positive. The arrow needs to be moved in the opposite direction to this offset.
In some extreme cases we applied large margins, which caused the popup to grow. Using a transform instead of a margin avoids that problem.

Comment 59

[Markus Stange [:mstange]]

Using sliding for the panel also ensures that the panel isn't resized based on its position. Now the only case where opening it will make it smaller is when its size is bigger than the screen.

Comment 60

[Neil Deakin]

Comment on attachment 8340104 [details] [diff] [review]
Part 2: Attempt to use sliding for the menu panel.

This should be reviews by someone else.

Comment 61

[Neil Deakin]

Comment on attachment 8340106 [details] [diff] [review]
Part 3: Support sliding for the menu panel.

>-    bool slideHorizontal = mSlide && mPosition >= POPUPPOSITION_BEFORESTART
>-                                  && mPosition <= POPUPPOSITION_AFTEREND;
>-    bool slideVertical = mSlide && mPosition >= POPUPPOSITION_STARTBEFORE
>-                                && mPosition <= POPUPPOSITION_ENDAFTER;
>+    uint8_t position = GetAlignmentPosition();
>+    bool slideHorizontal = mSlide && position >= POPUPPOSITION_BEFORESTART
>+                                  && position <= POPUPPOSITION_AFTEREND;
>+    bool slideVertical = mSlide && position >= POPUPPOSITION_STARTBEFORE
>+                                && position <= POPUPPOSITION_ENDAFTER;

This should be rewritten to avoid calling GetAlignmentPosition when mSlide is false.

Comment 62

[Markus Stange [:mstange]]

Attachment: Part 3: Support sliding for the menu panel.

Comment 63

[Neil Deakin]

Comment on attachment 8340107 [details] [diff] [review]
Part 4: Make adjustment sign consistent and use a transform for moving the arrow.

>           if (position.indexOf("_after") > 0) {
>             arrowbox.pack = "end";
>-            arrowbox.style.marginBottom = offset + "px";
>           } else {
>             arrowbox.pack = "start";
>-            arrowbox.style.marginTop = offset + "px";
>           }
>+          arrowbox.style.transform = "translate(0, " + -offset + "px)";

The old code is wrong here in terms of the sign in one case, right?

Comment 64

[Markus Stange [:mstange]]

Yes. I haven't checked in which case, but I've tested the new code in all cases.

Comment 65

[:Gijs (he/him)]

Comment on attachment 8340104 [details] [diff] [review]
Part 2: Attempt to use sliding for the menu panel.

Review of attachment 8340104 [details] [diff] [review]:
-----------------------------------------------------------------

Apologies for the delay, I had intermittent internet access over the weekend due to travel. This looks good to me.


If this doesn't fix the lopsided margins on OS X when the menupanel opens close to the edge of the screen, can you please file a followup bug for that?

Comment 66

[u428464]

Shouldn't this land on Nightly ?

Comment 67

[Markus Stange [:mstange]]

As soon as I've fixed the test failures.
https://tbpl.mozilla.org/?tree=Try&rev=bf772f5ba192

Comment 68

[u428464]

I've tested the try build. It's very nice. However the panels should really be allowed to be positioned past the edges of the screen, because the patch doesn't cover the case when part of the window is off screen.

Comment 69

[:Gijs (he/him)]

(In reply to Guillaume C. [:ge3k0s] from comment #68)
> I've tested the try build. It's very nice. However the panels should really
> be allowed to be positioned past the edges of the screen, because the patch
> doesn't cover the case when part of the window is off screen.

Why, though? What would the user do to read the text / use the information that is otherwise offscreen? Panels can't be moved by users, so that'd mean they'd have to adjust the window themselves and then re-trigger the panel in some way. That seems suboptimal.

Comment 70

[u428464]

(In reply to :Gijs Kruitbosch from comment #69)
> (In reply to Guillaume C. [:ge3k0s] from comment #68)
> > I've tested the try build. It's very nice. However the panels should really
> > be allowed to be positioned past the edges of the screen, because the patch
> > doesn't cover the case when part of the window is off screen.
> 
> Why, though? What would the user do to read the text / use the information
> that is otherwise offscreen? Panels can't be moved by users, so that'd mean
> they'd have to adjust the window themselves and then re-trigger the panel in
> some way. That seems suboptimal.

It's a bit confusing to have a panel appearing under another buttons than the one that triggers the panel. See for example https://bugzilla.mozilla.org/attachment.cgi?id=765147

Comment 71

[Dão Gottwald [:dao]]

(In reply to Guillaume C. [:ge3k0s] from comment #70)
> It's a bit confusing to have a panel appearing under another buttons than
> the one that triggers the panel. See for example
> https://bugzilla.mozilla.org/attachment.cgi?id=765147

Yes, but there's no ideal solution here.

Comment 72

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

While this is very tricky and involves many tradeoffs, there are a few cases where the "right thing" would involve the panel being off the screen.

* The panel is off the screen by only a few pixels, meaning (eg) only the border and margin is off screen, but the content is entirely on-screen.

* The user has multiple screens and has explicitly moved the main application window such that it spans 2 screens.  In this case, it seems reasonable the panel could also span 2 screens.

* The user has a single screen but has still chosen to explicitly move the app window such that it is partially off screen.  A panel also being partially off-screen in this case probably wouldn't be that surprising to such a user.

I'm not suggesting this means we can just ignore the existing constraints and unconditionally move it off-screen, but there are at least some use-cases where moving it off-screen would be better than the status quo.  Whether it is worth the effort to sanely support these use-cases is a different question - and given our resourcing constraints, I'd have trouble arguing it is.

Comment 73

[Florian Bender]

Attachment: australis-menu-screen-edge.png

(In reply to Mark Hammond [:markh] from comment #72)
> * The panel is off the screen by only a few pixels, meaning (eg) only the
> border and margin is off screen, but the content is entirely on-screen.

It simply looks odd in an OS X maximized window, thus this situation needs to be taken into account (I guess most people run Firefox maximized).

(Only other solution I can think of is moving the arrow to the corner, which would break the current UI concenpt.)

Comment 74

[u428464]

Any news on the patch here ?

Comment 75

[u428464]

As a follow-up it would be nice to introduce the sliding behaviour to all panels. Even if the menu panel is more likely to be affected by this issue. 

Any news on the test failing here ?

Comment 76

[Markus Stange [:mstange]]

Attachment: Part 5: Fix test after removing shadow offset

test_largemenu.xul was expecting the 3px shadow margins that part 1 removes and needs to be updated.

I'm not completely sure about the " <= screen.height" change. Is it correct?
Example values in that comparison are here: https://tbpl.mozilla.org/?tree=Try&rev=dc7815eafac7

Comment 77

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b5fa5fb3a22
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb9c10bf3dd1
https://hg.mozilla.org/integration/mozilla-inbound/rev/50469056e5ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d0086188688

Comment 78

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8b5fa5fb3a22
https://hg.mozilla.org/mozilla-central/rev/cb9c10bf3dd1
https://hg.mozilla.org/mozilla-central/rev/50469056e5ab
https://hg.mozilla.org/mozilla-central/rev/2d0086188688