mjh563 Reporter
	Description • 11 years ago

Steps to Reproduce:

1. Click the bookmarks button

Expected result:

The bookmarks menu is displayed with the menu arrow pointing at the button.

Actual result:

Most of the time, the expected result happens. But sometimes the menu can be displayed incorrectly, as shown in the screenshot.

By default the menu is left-aligned with the button, but if there's not enough space to show it left-aligned (ie. the browser window is too close to the right edge of the screen), then it gets right-aligned instead.

The buggy positioning happens for a few pixels at the changeover point between it being left-aligned and right-aligned.

	Dmitry Medvinsky
	Comment 1 • 11 years ago

Moreover, on my system, when reproducing this issue I see the menu arrow get chopped off partly. See the attachment for illustration.

Also, if I start moving my mouse over the affected area, the arrow gets redrawn pixel by pixel to the desired state.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 2 • 11 years ago

Mark, do you think the arrow could switch sides to be closer to the anchor in this case?

	Mark Hammond [:markh] [:mhammond]
	Comment 3 • 11 years ago

(In reply to Jared Wein [:jaws] from comment #2)
> Mark, do you think the arrow could switch sides to be closer to the anchor
> in this case?

Yeah, the popup layout code is getting confused.  I'm struggling to nail exactly what is going on, but it seems related to:

http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/menu.xml#593

(which is: this.popupBoxObject.moveToAnchor(anchor, this.alignmentPosition ...)

and possibly the timing of reflows.  this.alignmentPosition is a value calculated based on the last time the panel was positioned - which was based on the original anchor, not the new anchor this function is trying to setup.

Re the reflows - another problem with this approach seems to be that while onpopupshown is replacing the "old" anchor with a new one, a reflow seems to replace the anchor back to the dropmarker it originally was.

Sorry this is vague, but I'm having trouble getting tracing exactly what is going on here, other than to say that the panel certainly can fit correctly in the cases where this shows up but seems to be getting confused by this onpopupshown handler...

	Justin Dolske [:Dolske]
	Comment 4 • 11 years ago

Are these special steps needed to reproduce this? It WFM on OS X.

/cc Enn, since there have been arrow-panel bugs like this before.

	Mark Hammond [:markh] [:mhammond]
	Comment 5 • 11 years ago

(In reply to Justin Dolske [:Dolske] from comment #4)
> Are these special steps needed to reproduce this? It WFM on OS X.

Comment 0 mentions "The buggy positioning happens for a few pixels at the changeover point between it being left-aligned and right-aligned." and this is indeed how to repro it on Windows.  No idea if the same problem exists on OSX though.

> /cc Enn, since there have been arrow-panel bugs like this before.

TBH, I don't think this is a panel bug per-se, but related to the fact that http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/menu.xml#593 attempts to change the anchor in the popupshown event.  I could be wrong though (and it obviously wouldn't be the first time ;)

	Mike Conley (:mconley) (:⚙️)
	Comment 6 • 11 years ago

Gijs, mikedeboer and I are pretty sure this is all related to the custom styling that's being applied to the menu *after* it's shown to make it look like a panel. We have another bug on file about that, so marking dependency.

	Mike Conley (:mconley) (:⚙️)
	Comment 7 • 11 years ago

I think Enn's got this one under control.

	Neil Deakin Assignee
	Comment 8 • 11 years ago

This patch removes the popupshown hackery from the menu and adds an anchor feature to menu frames.

The new feature works as follows:

On any menu type of element, anchor="something" can be used to specify the anchor for the menu, instead of using the menu itself. It can be set to either the anonid of an anonymous child or, if that doesn't exist, the id of another element in the document. Otherwise, the menu itself it used.

It works on the following:

<menu>
<button type="menu">
<button type="panel">
<button type="menu-button">
<toolbarbutton type="menu">
<toolbarbutton type="panel">
<toolbarbutton type="menu-button">
<menulist>

and any other element with a binding with display="xul:menu"

Builds will be at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/neil@mozilla.com-8c6080044e47/

	Neil Deakin Assignee
	Comment 9 • 11 years ago

Fixes test on Windows

	neil@parkwaycc.co.uk
	Comment 10 • 11 years ago

Comment on attachment 8362540 [details] [diff] [review]
bmarksarrow

>+nsIContent*
>+nsMenuFrame::GetAnchor()
>+{
>+  mozilla::dom::Element* anchor = nullptr;
>+
>+  nsAutoString id;
>+  mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::anchor, id);
>+  if (!id.IsEmpty()) {
>+    nsIDocument* doc = mContent->OwnerDoc();
>+
>+    anchor =
>+      doc->GetAnonymousElementByAttribute(mContent, nsGkAtoms::anonid, id);
>+    if (!anchor) {
>+      anchor = doc->GetElementById(id);
>+    }
>+  }
>+
>+  // Always return the menu's content if the anchor wasn't set or wasn't found.
>+  return anchor ? anchor : mContent;
Is it possible for the anchor element to not have a frame? What happens in this case?

>-    popupFrame->LayoutPopup(aState, this, sizeToPopup);
>+    popupFrame->LayoutPopup(aState, mContent->GetPrimaryFrame(),
Why not stick with "this"?

> nsresult
>-nsMenuPopupFrame::SetPopupPosition(nsIFrame* aAnchorFrame, bool aIsMove)
>+nsMenuPopupFrame::SetPopupPosition(nsIFrame* aAnchorFrame, bool aIsMove, bool aSizedToPopup)
...
>-  bool sizedToPopup = false;
>-  if (aAnchorFrame->GetContent()) {
>-    // the popup should be the same size as the anchor menu, for example, a menulist.
>-    sizedToPopup = nsMenuFrame::IsSizedToPopup(aAnchorFrame->GetContent(), false);
>-  }
I take it that this is needed because that might now be a different frame and it's only needed for this one boolean so you might as well pass that instead.

>-                      xbl:inherits="align,dir,pack,orient,disabled,label,open"/>
>+                      anonid="dropmarker" xbl:inherits="align,dir,pack,orient,disabled,label,open"/>
Any particular reason to change just this one binding?

	Mike Conley (:mconley) (:⚙️)
	Comment 11 • 11 years ago

Hm, the code looks OK to me (minus some trailing whitespace in the test file), but when testing, I did notice that on OS X, the first click on the bookmarks menu dropdown resulted in the arrow being anchored incorrectly:

http://i.imgur.com/tKS5AhH.png

Subsequent clicks anchored properly. I can reproduce this reliably.

	Mike Conley (:mconley) (:⚙️)
	Comment 12 • 11 years ago

Forget what I wrote - I didn't fully re-build, so my experience might not reflect reality. Re-building properly now.

	Mike Conley (:mconley) (:⚙️)
	Comment 13 • 11 years ago

Bah, original observation still stands despite full re-build. It anchors incorrectly on first click.

	Mike Conley (:mconley) (:⚙️)
	Comment 14 • 11 years ago

Comment on attachment 8362540 [details] [diff] [review]
bmarksarrow

Tentatively r-'ing due to the first-click anchoring thing I mentioned.

	Neil Deakin Assignee
	Comment 15 • 11 years ago

This patch addresses the comments above and updates the menu.xml binding to be consistent with popup.xml. It also changes the arrow container to be a vertical box, which avoids an issue where the width is computed incorrectly the first time. It made this change to popup.xml  as well since most arrow popups are vertical.

The menu.xml changes should also reduce the number of extra reflows that happen.

	Mike Conley (:mconley) (:⚙️)
	Comment 16 • 11 years ago

Comment on attachment 8363746 [details] [diff] [review]
Anchors for menus, address comments and update menu.xml

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

The changes under browser/ look good to me! Thanks Neil!

	Neil Deakin Assignee
	Comment 17 • 11 years ago

(In reply to Neil Deakin from comment #15)
> It made this change to popup.xml  as well since most arrow popups are vertical.
> 
I should say I think it might be a good idea to make this change to popup.xml as well, but it isn't in this patch.

	Neil Deakin Assignee
	Comment 18 • 11 years ago

> I take it that this is needed because that might now be a different frame and it's only
> needed for this one boolean so you might as well pass that instead.

That is correct.

	neil@parkwaycc.co.uk
	Comment 19 • 11 years ago

Comment on attachment 8363746 [details] [diff] [review]
Anchors for menus, address comments and update menu.xml

>+          var position = this.alignmentPosition;
>+          var offset = this.alignmentOffset;
>+
>           // if this panel has a "sliding" arrow, we may have previously set margins...
Sorry for the delay; you'd confused me by including these changes ported from bug 874792. In the interests of consistency, there's an extra blank line here that isn't in the popup.xml version. I don't mind which way around you fix it, but if you change popup.xml then you might as well add the vbox fix too.

	Neil Deakin Assignee
	Comment 20 • 11 years ago

https://hg.mozilla.org/integration/fx-team/rev/71361416a124

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/71361416a124