Closed Bug 1130400 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: XUL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: dw-dev, Assigned: tnikkel)

References

Details

(Keywords: reproducible, testcase)

Attachments

(4 files, 2 obsolete files)

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.
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.
Flags: needinfo?(dw-dev)
Flags: needinfo?(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.
Attachment #8561566 - Attachment mime type: text/plain → application/x-xpinstall
Confirmed the problem, but I'm not sure if it's really a FF bug.
Gijs, could you please take a look?
Flags: needinfo?(gijskruitbosch+bugs)
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.
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?
Component: Menus → XP Toolkit/Widgets: Menus
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(tnikkel)
Product: Firefox → Core
Version: Firefox 38 → Trunk
(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...)
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 8.1 → All
Hardware: x86_64 → All
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.
Blocks: 1033052
Duplicate of this bug: 1139435
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.
Seems like this was an oversight in bug 562740.
Assignee: nobody → tnikkel
Flags: needinfo?(tnikkel)
Attachment #8577093 - Flags: review?(enndeakin)
Use a reflow callback to position the popup after reflow, because we may not have our final position during reflow.
Attachment #8577095 - Flags: review?(enndeakin)
Blocks: 1055702
Attachment #8577093 - Flags: review?(enndeakin) → review+
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.
(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.
Attached patch Part 2 v2 (obsolete) — Splinter Review
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.
Attachment #8577095 - Attachment is obsolete: true
Attachment #8577095 - Flags: review?(enndeakin)
Attachment #8579224 - Flags: review?(enndeakin)
Attached patch Part 2. v3Splinter Review
I made the struct a little nicer to use.
Attachment #8579224 - Attachment is obsolete: true
Attachment #8579224 - Flags: review?(enndeakin)
Attachment #8579889 - Flags: review?(enndeakin)
Comment on attachment 8579889 [details] [diff] [review]
Part 2. v3

OK, well let's try it.

This fixes bug 433037 too?
Attachment #8579889 - Flags: review?(enndeakin) → review+
(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.
Depends on: 1145522
No longer depends on: 1145522
https://hg.mozilla.org/mozilla-central/rev/4867d8464f88
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(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!
Duplicate of this bug: 1055702
Component: XP Toolkit/Widgets: Menus → XUL
You need to log in before you can comment on or make changes to this bug.