Closed Bug 941409 Opened 6 years ago Closed 6 years ago

Bookmarks menu can be incorrectly positioned

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: mjh563, Assigned: enndeakin)

References

(Blocks 4 open bugs)

Details

(Whiteboard: [Australis:P2])

Attachments

(3 files, 2 obsolete files)

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.
See Also: → 940265
Attached image Chopped-off menu arrow
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.
Component: Toolbars and Customization → XUL Widgets
OS: Linux → All
Product: Firefox → Toolkit
Hardware: x86_64 → All
Mark, do you think the arrow could switch sides to be closer to the anchor in this case?
Component: XUL Widgets → XP Toolkit/Widgets: Menus
Flags: needinfo?(mhammond)
Product: Toolkit → Core
(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...
Flags: needinfo?(mhammond)
Whiteboard: [Australis:P2]
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.
(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 ;)
Component: XP Toolkit/Widgets: Menus → Bookmarks & History
Product: Core → Firefox
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.
Depends on: 941051
I think Enn's got this one under control.
Assignee: nobody → enndeakin
Attached patch bmarksarrow (obsolete) — Splinter Review
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/
Attached patch bmarksarrow (obsolete) — Splinter Review
Fixes test on Windows
Attachment #8361681 - Attachment is obsolete: true
Attachment #8362540 - Flags: review?(neil)
Attachment #8362540 - Flags: review?(mconley)
Blocks: 961031
No longer depends on: 961031
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?
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.
Forget what I wrote - I didn't fully re-build, so my experience might not reflect reality. Re-building properly now.
Bah, original observation still stands despite full re-build. It anchors incorrectly on first click.
Comment on attachment 8362540 [details] [diff] [review]
bmarksarrow

Tentatively r-'ing due to the first-click anchoring thing I mentioned.
Attachment #8362540 - Flags: review?(mconley) → review-
Status: NEW → ASSIGNED
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.
Attachment #8363746 - Flags: review?(neil)
Attachment #8363746 - Flags: review?(mconley)
Attachment #8362540 - Attachment is obsolete: true
Attachment #8362540 - Flags: review?(neil)
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!
Attachment #8363746 - Flags: review?(mconley) → review+
(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.
> 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.
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.
Attachment #8363746 - Flags: review?(neil) → review+
https://hg.mozilla.org/mozilla-central/rev/71361416a124
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Depends on: 965238
Depends on: 965255
Blocks: 974824
You need to log in before you can comment on or make changes to this bug.