1130400 - 'menu-button' button in menu panel - drop-down menu shown in wrong position

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[dw-dev]

Attachment: 'menu-button' button drop-down menu in menu panel

I am the developer of the Print Edit and Tile Tabs add-ons, both of which use 'menu-button' buttons.

If a 'menu-button' button is the first button in a row of buttons in the menu panel, then when the drop-marker is clicked, the drop down menu is shown in the wrong position (a long way to the right of the button).  Please attached screenshot.

Note, everything works fine if the button is the second or third button in a row of buttons in the menu panel.

Comment 1

[Paul Silaghi, QA [:pauly]]

Couldn't reproduce on 38.0a1 (2015-02-09) Win 7.
The drop-down menu is shown below the icon.
Try on a new profile, only with Print Edit installed.

Comment 2

[dw-dev]

Attachment: Test version of Print Edit 13.8

Comment 3

[dw-dev]

Please ignore the attachment - I hadn't realized that I couldn't attach a .xpi file.

The reason that you can't reproduce the problem is because Print Edit 13.0 - 13.8 all include a workaround that catches the 'onpopupshowing' event and re-positions the drop-down menu.

I have sent a test version (Print Edit 13.8test1.xpi) without the workaround directly to your email address.

Comment 4

[Paul Silaghi, QA [:pauly]]

Confirmed the problem, but I'm not sure if it's really a FF bug.
Gijs, could you please take a look?

Comment 5

[dw-dev]

I'm fairly certain this is a bug in Firefox.  When Australis was being introduced (Firefox 29), it took the Firefox developers quite a long time and several iterations to get 'menu-button' buttons working correctly in the new Menu Panel.

If you try the test version of Print Edit (13.8test1) in Firefox 32.0, the menu button drop-down menu works perfectly, but in Firefox 33.1.1 and later thew drop-down menu is displayed in the wrong position.
When doing this test, the menu button must be the first button in a row of buttons in the Menu Panel.

Comment 6

[:Gijs (he/him)]

If this broke between 32 and 33, I bet I broke this with bug 467442. :-\

The question is just... how? :-)

:tn, do you have time to look into this, or to give me some hints as to where to look?

Comment 7

[:Gijs (he/him)]

(I can repro in the last row of the panel on OS X. Not sure if/when/where/why it is conditional on which row of the panel it's in...)

Comment 8

[Timothy Nikkel (:tnikkel)]

It doesn't appear to be bug 467442 as I got this regression range:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=38ecfc3922b8&tochange=e8df6826a571

This includes bug 1033052 which is blamed for another menupopup positioning regression, so I'll pre-emptively blame it.

Comment 10

[Timothy Nikkel (:tnikkel)]

Menu popup frames depend on all the frames in its ancestor chain having their final position when we call SetPopupPosition, but thats not true for how inline frames are reflowed, they get positioned after we reflow their children. So I think we should just use a reflow callback to do the SetPopupPosition call(s). I'll write up a patch hopefully soon.

Comment 11

[Timothy Nikkel (:tnikkel)]

Attachment: Part 1. Pass the anchor frame to SetPopupPosition.

Seems like this was an oversight in bug 562740.

Comment 12

[Timothy Nikkel (:tnikkel)]

Attachment: Part 2. Use reflow callback to position popup.

Use a reflow callback to position the popup after reflow, because we may not have our final position during reflow.

Comment 13

[Neil Deakin]

Comment on attachment 8577095 [details] [diff] [review]
Part 2. Use reflow callback to position popup.

>+bool
>+nsMenuPopupFrame::ReflowFinished()
>+{
>+  mReflowCallbackPosted = false;
>+
>+  SetPopupPosition(mReflowCallbackAnchor, false, mReflowCallbackSizedToPopup);
>+
>+  mReflowCallbackAnchor = nullptr;
>+  mReflowCallbackSizedToPopup = false;
>+
>+  return false;
>+}

This calls SetPopupPosition twice for every popup. Is that necessary just to support this specific edgecase? I assume things break if we only call SetPopupPosition during the callback? 


>+
>+void
>+nsMenuPopupFrame::ReflowCallbackCanceled()
>+{
>+  mReflowCallbackPosted = false;
>+  mReflowCallbackAnchor = nullptr;
>+  mReflowCallbackSizedToPopup = false;
> }
> 
> nsIContent*
> nsMenuPopupFrame::GetTriggerContent(nsMenuPopupFrame* aMenuPopupFrame)
> {
>   while (aMenuPopupFrame) {
>     if (aMenuPopupFrame->mTriggerContent)
>       return aMenuPopupFrame->mTriggerContent;
>@@ -1937,16 +1972,23 @@ nsMenuPopupFrame::MoveToAttributePositio
> 
>   if (NS_SUCCEEDED(err1) && NS_SUCCEEDED(err2))
>     MoveTo(xpos, ypos, false);
> }
> 
> void
> nsMenuPopupFrame::DestroyFrom(nsIFrame* aDestructRoot)
> {
>+  if (mReflowCallbackPosted) {
>+    PresContext()->PresShell()->CancelReflowCallback(this);
>+    mReflowCallbackPosted = false;
>+    mReflowCallbackAnchor = nullptr;
>+    mReflowCallbackSizedToPopup = false;
>+  }
>+
>   nsMenuFrame* menu = do_QueryFrame(GetParent());
>   if (menu) {
>     // clear the open attribute on the parent menu
>     nsContentUtils::AddScriptRunner(
>       new nsUnsetAttrRunnable(menu->GetContent(), nsGkAtoms::open));
>   }
> 
>   ClearPopupShownDispatcher();
>diff --git a/layout/xul/nsMenuPopupFrame.h b/layout/xul/nsMenuPopupFrame.h
>--- a/layout/xul/nsMenuPopupFrame.h
>+++ b/layout/xul/nsMenuPopupFrame.h
>@@ -147,17 +147,18 @@ public:
> protected:
>   virtual ~nsXULPopupShownEvent() { }
> 
> private:
>   nsCOMPtr<nsIContent> mPopup;
>   nsRefPtr<nsPresContext> mPresContext;
> };
> 
>-class nsMenuPopupFrame MOZ_FINAL : public nsBoxFrame, public nsMenuParent
>+class nsMenuPopupFrame MOZ_FINAL : public nsBoxFrame, public nsMenuParent,
>+                                   public nsIReflowCallback
> {
> public:
>   NS_DECL_QUERYFRAME_TARGET(nsMenuPopupFrame)
>   NS_DECL_QUERYFRAME
>   NS_DECL_FRAMEARENA_HELPERS
> 
>   explicit nsMenuPopupFrame(nsStyleContext* aContext);
> 
>@@ -394,16 +395,20 @@ public:
>       mPopupShownDispatcher->CancelListener();
>       mPopupShownDispatcher = nullptr;
>       return true;
>     }
> 
>     return false;
>   }
> 
>+  // nsIReflowCallback
>+  virtual bool ReflowFinished() MOZ_OVERRIDE;
>+  virtual void ReflowCallbackCanceled() MOZ_OVERRIDE;
>+
> protected:
> 
>   // returns the popup's level.
>   nsPopupLevel PopupLevel(bool aIsNoAutoHide) const;
> 
>   // redefine to tell the box system not to move the views.
>   virtual void GetLayoutFlags(uint32_t& aFlags) MOZ_OVERRIDE;
> 
>@@ -521,31 +526,36 @@ protected:
>   int8_t mPopupAlignment;
>   int8_t mPopupAnchor;
>   int8_t mPosition;
> 
>   // One of PopupBoxObject::ROLLUP_DEFAULT/ROLLUP_CONSUME/ROLLUP_NO_CONSUME
>   uint8_t mConsumeRollupEvent;
>   FlipType mFlip; // Whether to flip
> 
>+  nsIFrame* mReflowCallbackAnchor;
>+  bool mReflowCallbackSizedToPopup;
>+
>   bool mIsOpenChanged; // true if the open state changed since the last layout
>   bool mIsContextMenu; // true for context menus
...
>   bool mHFlip;
>   bool mVFlip;
> 
>+  bool mReflowCallbackPosted;
>+

Perhaps the three added fields should be in a struct? Or at least the three fields should be declared all together next to each other.

Comment 14

[Timothy Nikkel (:tnikkel)]

(In reply to Neil Deakin from comment #13)
> This calls SetPopupPosition twice for every popup. Is that necessary just to
> support this specific edgecase? I assume things break if we only call
> SetPopupPosition during the callback? 

I actually didn't try calling it only in the callback, I figured this would be safest, and probably not a big deal for performance. But I can change it to just make the call in the callback if you think that will be okay.

Comment 15

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/29a213d5fe80

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/29a213d5fe80

Comment 17

[Timothy Nikkel (:tnikkel)]

Attachment: Part 2 v2

Used a struct.

I left the two SetPopupPosition calls in case someone depends on the effects of that function before we get to process reflow callbacks. We set the bounds of our frame in that method, so it seems safer this way. If you think the overhead of the extra calls is too much I can try it without.

Comment 18

[Timothy Nikkel (:tnikkel)]

Attachment: Part 2. v3

I made the struct a little nicer to use.

Comment 19

[Neil Deakin]

Comment on attachment 8579889 [details] [diff] [review]
Part 2. v3

OK, well let's try it.

This fixes bug 433037 too?

Comment 20

[Timothy Nikkel (:tnikkel)]

(In reply to Neil Deakin from comment #19)
> This fixes bug 433037 too?

The testcase in bug 433037 worked fine for me in Nightly (without the patch from this bug of course), so either I can't reproduce or it's already fixed.

Comment 21

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4867d8464f88

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4867d8464f88

Comment 23

[Neil Deakin]

(In reply to Timothy Nikkel (:tn) from comment #20)
> (In reply to Neil Deakin from comment #19)
> > This fixes bug 433037 too?
> 
> The testcase in bug 433037 worked fine for me in Nightly (without the patch
> from this bug of course), so either I can't reproduce or it's already fixed.

I could reproduce when I resized the window, however, with this patch I no longer can. Let's close it!

Comment 24

[Timothy Nikkel (:tnikkel)]

Landed a test
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c4030c686d6

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1c4030c686d6