Opening unsorted bookmarks or bookmarks toolbar folders in the bookmarks menu button, resizing the button into overflow menu, and resizing it out again, disables context menu on those folders

VERIFIED FIXED in Firefox 29

Status

()

defect
--
major
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: adalucinet, Assigned: mconley)

Tracking

(Blocks 1 bug)

Trunk
Firefox 31
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox29- verified, firefox30- verified, firefox31 verified)

Details

(Whiteboard: [Australis:P3])

Attachments

(2 attachments, 7 obsolete attachments)

Reporter

Description

5 years ago
Reproducible on:
the latest Aurora (Build ID: 20140317004002):
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
the latest Nightly (Build ID: 20140317030202):
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:30.0) Gecko/20100101 Firefox/30.0

STR:
1. Launch Firefox.
2. Navigate to any URL (eg: http://www.nytimes.com/)
3. Bookmark the page via the Star button.
4. Narrow browser width so that a Bookmarks widget moves to a toolbar chevron.
5. Expand browser so that the Bookmarks widget is visible.
6. Open the Bookmarks widget/Unsorted Bookmarks and right click on the bookmark.

Expected results: The context menu items are grayed out.

Actual results: The context menu items are not grayed out.

Notes:
1. Also reproducible if I right click on Unsorted Bookmarks or Bookmarks Toolbar via the Bookmarks widget.
2. Not reproducible in Library (Ctrl+Shift+B).
3. No errors in the Console.

Comment 1

5 years ago
I can't reproduce with these steps on yesterday or today's nightly...

Comment 2

5 years ago
To reproduce this problem, you need to open the special folder("Unsorted Bookmarks","Bookmarks Toolbar") from the "Bookmarks widget" once before carrying out bookmark.

ie, you need to execute step 2-0 before step2

2-0. Open the "Bookmarks widget"/"Unsorted Bookmarks" once


Actual results:
The context menu items are grayed out.


This problem happens on "Bookmarks Toolbar" too.

Updated

5 years ago
Severity: normal → major

Comment 3

5 years ago
I'm really not sure this warrants tracking. The steps are convoluted enough that I don't think many people will hit this in practice. Still P3 rather than something lower because I have a nagging feeling it might point to a bigger consistency problem which can be exposed in other ways.

We should probably make an automated test for this similar to the other one I wrote.
Summary: Bookmarking via the Star icon makes items from the context menu inactive → Bookmarking via the Star icon makes items in the context menu of the unsorted bookmarks and bookmarks toolbar folders in the bookmarks menu button inactive if they were open before bookmarking
Whiteboard: [Australis:P3]

Comment 4

5 years ago
I do not think the steps are convoluted.


Simple STR:
1. Start Firefox
2. Open "Special folder" once
   ie. Open Bookmarks widget/"Bookmarks Toolbar"

3. Narrow browser width so that a "Bookmarks widget" moves to a toolbar chevron.
4. Expand browser so that the "Bookmarks widget" is visible.

5. Right click on any bookmark in the "Bookmarks widget"/"Bookmarks Toolbar"

Comment 5

5 years ago
(In reply to Alice0775 White from comment #4)
> I do not think the steps are convoluted.
> 
> 
> Simple STR:
> 1. Start Firefox
> 2. Open "Special folder" once
>    ie. Open Bookmarks widget/"Bookmarks Toolbar"

Very few people do this - if they use the bookmarks toolbar, they'd just enable it...

> 3. Narrow browser width so that a "Bookmarks widget" moves to a toolbar
> chevron.

Very few people do this - many users just run their browser maximized. Those that don't don't spend all their time resizing their browser

> 4. Expand browser so that the "Bookmarks widget" is visible.

Even fewer people then do this again.

> 5. Right click on any bookmark in the "Bookmarks widget"/"Bookmarks Toolbar"

Still fewer people even know that there's a context menu on these items. Heck, it doesn't even work on the native mac menu, so for the longest time I didn't think it worked on any other parts of the mac version, either.

So yes, I think the steps are, let's say, "elaborate". There are unfixed issues which affect much larger populations of people than the cross-section of all the small groups of users listed above.

Updated

5 years ago
Summary: Bookmarking via the Star icon makes items in the context menu of the unsorted bookmarks and bookmarks toolbar folders in the bookmarks menu button inactive if they were open before bookmarking → Opening unsorted bookmarks or bookmarks toolbar folders in the bookmarks menu button, resizing the button into overflow menu, and resizing it out again, disables context menu on those folders
Not tracking for now (based on Gijs comment #3 & #5)
I'll take a look at this today.
Assignee: nobody → mconley
Ok, so here's the story:

It looks like when the Bookmarks Menu Button is underflowed, we call _uninitView() on the BookmarkingUI object, which in turn calls this.button._placesView.uninit() on the menu-button.

That's great, because it means the next time that menupopup opens, we reconstruct the view attached to it, and controllers get appended properly, and all is hunky dory.

The bad news is that there are subelements to the menupopup that also have _placesView's that need to be uninit'd. These subelements are the special folders that Alice mentions in comment 4. These views also need to be uninitted, so that they're reconstructed when the popups re-open.

Because they're not uninitted, the menupopups have PlacesViews already constructed for themselves, but the controllers (which were destroyed during the overflow / underflow) don't get re-appended.

mak - does the above make sense? If so, what's the best way to uninit those views? Are "Bookmarks Toolbar" and "Unsorted Bookmarks" the only ones I have to worry about here, or are there likely to be more?
Flags: needinfo?(mak77)
I guess when we remove and reinsert the menu into the document, we do the same with the popup contents, causing the same binding issue
Actually the chevron in the bookmarks-toolbar-items has the same identical issue, it's a view that is never uninit-ed properly. Also the bookmarks-toolbar-items itself may break on overflow if it's on the nav-bar (edge-case, but still...).
I think those views are the only ones subject to breakage on customization/overflow, as of now.
Provided you want the fix in beta, we can't go too deep into refactoring stuff, it would be nice to have a single point where views can register and be uninited but that requires scary changes.
Probably here we should just add code to browser-places that uninits those views when the containing views are uninit-ed (and add overflow handling to PlacesToolbarHelper).
Would be nice to file a bug to investigate a better way to unify these code paths, maybe the view itself on creation should check if it's on a customizable/overflowing area, and in such a case register a listener to suicide when needed.
Flags: needinfo?(mak77)

Comment 10

5 years ago
Something that I remember considering when I worked on the general view case: can the code that checks the enabled state of the commands easily check if the view's controller is still in the controllers collection, and reappend it if necessary? AFAICT if you call: controllers.getControllerId(customController), that'll return something if it's in there, and throw (!) if it isn't. (sadly, nsIController's API is pretty exception-heavy, see https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIControllers )

Could that provide a more robust workaround to issues of this kind?
I think it's not just the controller, field values are lost and in past we also saw leaks in these cases (those may just be evidence of us throwing in the middle of some operation due to a lost controller, but still, we have the old binding around that may keep a Places result alive). The safer way is to destroy and recreate the view completely, so all of its init re-runs. Mostly, I don't think you want to experiment in Beta, so here we need a safer patch regardless.

Actually, apart the toolbar chevron, toolbar and unsorted submenus that are in the bookmarks panel, I also found the 2 views in browser-menubar.inc have the same issue, though that is covered by bug 625778. That bug may do experiments on Nightly since I think it has a lower priority than fixing the new panel.
Posted patch Patch v1 (obsolete) — Splinter Review
Comment on attachment 8397126 [details] [diff] [review]
Patch v1

Something like this?
Attachment #8397126 - Flags: review?(mak77)
Comment on attachment 8397126 [details] [diff] [review]
Patch v1

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

::: browser/base/content/browser-places.js
@@ +977,5 @@
> +        this._viewElt._placesView.uninit();
> +        this.init(true);
> +      }
> +    }
> +  },

yes something like this, though there's more:

1. CustomizeChange may also happen outside CustomizeStart and CustomizeEnd, so this code should also happen in customizeChange if !this._isCustomizing... so probably it's worth to move the reset to a restartView method called by both code points.

2. the chevron is not being restarted here, thus the PlacesToolbar::uninit() method should also uninit() the chevron popup view. if (this._chevron._placesView) this._chevron._placesView.uninit() should be enough
Attachment #8397126 - Flags: review?(mak77)
Posted patch Patch v1.1 (obsolete) — Splinter Review
Updated based on mak's feedback. Regression test coming.
Attachment #8397126 - Attachment is obsolete: true
Posted patch Regression test (obsolete) — Splinter Review
Posted patch Regression test v1.1 (obsolete) — Splinter Review
Disabling for Linux. Local testing showed that this would likely fail intermittently - sometimes the bookmarks menu panel wouldn't open when synthesizing click events on the bookmarks menu button dropmarker.
Attachment #8398027 - Attachment is obsolete: true
Comment on attachment 8398035 [details] [diff] [review]
Regression test v1.1

Cool to review this test, Gijs?

If not, lemme know and I'll redirect.
Attachment #8398035 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8398026 [details] [diff] [review]
Patch v1.1

Is this what you meant, Marco?
Attachment #8398026 - Flags: review?(mak77)

Comment 20

5 years ago
Comment on attachment 8398035 [details] [diff] [review]
Regression test v1.1

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

I'm assuming you're able to use find/replace correctly for the existing users of the popup checking code... :-)

However, there's quite some notes. Additionally, from what I understood from Marco's comments, we should also check the bookmarks toolbar chevron. That'd probably involve creating some bookmarks so we can be sure that it's populated, though... (and then removing them again, obviously). If you want to followup that in order to be able to land this sooner, that'd wfm, but we shouldn't lose track of it.

::: browser/components/customizableui/test/browser_984455_bookmarks_items_reparenting.js
@@ +14,5 @@
> +/**
> + * Helper function that opens the bookmarks menu, and returns a Promise that
> + * resolves as soon as the menu is ready for interaction.
> + */
> +function* bookmarksMenuPanelShown() {

This doesn't yield, so why is it a function* ?

@@ +32,5 @@
> + * Opens the bookmarks menu panel, and then opens each of the "special"
> + * submenus in that list. Then it checks that those submenu's context menus
> + * are properly hooked up to a controller.
> + */
> +function* checkSpecialContextMenus() {

Ditto

@@ +33,5 @@
> + * submenus in that list. Then it checks that those submenu's context menus
> + * are properly hooked up to a controller.
> + */
> +function* checkSpecialContextMenus() {
> +  return Task.spawn(function() {

But then this is a generator and not a function* ? I am so confused.

@@ +52,5 @@
> +    EventUtils.synthesizeMouseAtCenter(dropmarker, {});
> +    info("Waiting for bookmarks menu popup to show after clicking dropmarker.")
> +    yield shownPromise;
> +
> +    for (let menuID in kSpecialItemIDs) {

Nit: for (let [menuID, menuPopupID] of kSpecialItemIDs) {

@@ +54,5 @@
> +    yield shownPromise;
> +
> +    for (let menuID in kSpecialItemIDs) {
> +      let menuItem = document.getElementById(menuID);
> +      let menuPopup = document.getElementById(kSpecialItemIDs[menuID]);

Nit (ctd.): let menuPopup = document.getElementById(menuPopupId);

@@ +64,5 @@
> +
> +      ok(contextMenu._view, "Should have a view attached to the places context menu");
> +      ok(contextMenu._view.controllers, "Should have some controllers on this view");
> +      ok(contextMenu._view.controllers.getControllerForCommand("placesCmd_open"),
> +         "Should get a controller back for the placesCmd_open command.");

IIRC this throws if it doesn't exist, doesn't it? In which case, can you put this in a try catch so we still run the rest of this code?

@@ +65,5 @@
> +      ok(contextMenu._view, "Should have a view attached to the places context menu");
> +      ok(contextMenu._view.controllers, "Should have some controllers on this view");
> +      ok(contextMenu._view.controllers.getControllerForCommand("placesCmd_open"),
> +         "Should get a controller back for the placesCmd_open command.");
> +

More generally, you're checking the implementation details. In the test I added for the original problem ( http://mxr.mozilla.org/mozilla-central/source/browser/components/places/tests/browser/browser_toolbarbutton_menu_context.js ) I just checked if the "new bookmark" item was disabled.

This is essentially based on the idea that m-bc tests are more integration tests than unit tests of the specific code (which would be why we're simulating UI events instead of calling internal bits of code and verifying they produce correct results). Would you agree, and if so, can you adapt these checks? :-)

@@ +86,5 @@
> + * Opens the context menu for Bookmarks Toolbar Items and ensures
> + * that the controller for that view is properly hooked up. Closes
> + * the context menu on completion.
> + */
> +function* checkBookmarksItemsContextMenus() {

Likewise, doesn't yield (just returns a promise, which doesn't matter).

@@ +87,5 @@
> + * that the controller for that view is properly hooked up. Closes
> + * the context menu on completion.
> + */
> +function* checkBookmarksItemsContextMenus() {
> +  return Task.spawn(function() {

But this yields lots! (there's more of this, but I think you get the idea now)

@@ +98,5 @@
> +
> +    ok(contextMenu._view, "Should have a view attached to the places context menu");
> +    ok(contextMenu._view.controllers, "Should have some controllers on this view");
> +    ok(contextMenu._view.controllers.getControllerForCommand("placesCmd_open"),
> +       "Should get a controller back for the placesCmd_open command.");

Same comments here about how we check things and/or throw/catches

@@ +123,5 @@
> +  ok(!gNavBar.querySelector("#" + kBookmarksButton),
> +     "Bookmarks button should no longer be in the gNavBar");
> +  let bookmarksNode = gOverflowList.querySelector("#" + kBookmarksButton);
> +  ok(bookmarksNode, "Bookmarks button should be overflowing");
> +  ok(bookmarksNode.getAttribute("overflowedItem") == "true",

Nit: is(bookmarksNode.getAttribute("overflowedItem"), "true", ...)

(same below in the other test)

::: browser/components/customizableui/test/head.js
@@ +427,5 @@
>  
>    return deferred.promise;
>  }
>  
> +function popupShown(aPopup) {

If you're feeling awesome, we can totally de-duplicate these functions, because they essentially do the same but for different events. Can make the existing things thin wrappers around promisePopupEvent(aPopup, "shown" || "hidden"), which would have all the boring details.
Attachment #8398035 - Flags: review?(gijskruitbosch+bugs) → feedback+
Gah - this is my first time using the * function syntax, and I appear to have misunderstood it. I thought * meant that the function returns a Promise, when in fact * indicates that the function yields. I'll fix that.

Thanks for the other notes. I'll have a new patch up soon.
function* is a nice decorator especially to be able to return from a Task without having to throw new Task.Result. That's the primary usage we had, but we should slowly convert all generators to use it.
Comment on attachment 8398026 [details] [diff] [review]
Patch v1.1

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

::: browser/base/content/browser-places.js
@@ +920,5 @@
>      }
>      this._shouldWrap = this._getShouldWrap();
>    },
>  
> +  customizeChange: function PTH_customizeChange(aInitting=false) {

what's this aInitting thing?

@@ +927,5 @@
> +    // letting us prepare with customizeStart. If that's the case, we
> +    // need to reset the views since they're probably broken from the
> +    // DOM reparenting.
> +    if (!this._isCustomizing) {
> +      this._resetView();

hm, looks like this is going to uninit and recreate this view for each customization change, also the ones not affecting bookmarks-toolbar-items.. that's a little bit overkill :(
Attachment #8398026 - Flags: review?(mak77)
Posted patch Regression tests v1.2 (obsolete) — Splinter Review
Going to do a quick self-review before requesting it from Gijs.
Attachment #8398035 - Attachment is obsolete: true
Comment on attachment 8398602 [details] [diff] [review]
Regression tests v1.2

Alright, ready for you to kick the tires.
Attachment #8398602 - Flags: review?(gijskruitbosch+bugs)
Posted patch Patch v1.2 (obsolete) — Splinter Review
(In reply to Marco Bonardo [:mak] from comment #23)
> Comment on attachment 8398026 [details] [diff] [review]
> Patch v1.1
> 
> Review of attachment 8398026 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser-places.js
> @@ +920,5 @@
> >      }
> >      this._shouldWrap = this._getShouldWrap();
> >    },
> >  
> > +  customizeChange: function PTH_customizeChange(aInitting=false) {
> 
> what's this aInitting thing?

That's leftover garbage that should never have been included. Thanks for catching it. :)

> 
> @@ +927,5 @@
> > +    // letting us prepare with customizeStart. If that's the case, we
> > +    // need to reset the views since they're probably broken from the
> > +    // DOM reparenting.
> > +    if (!this._isCustomizing) {
> > +      this._resetView();
> 
> hm, looks like this is going to uninit and recreate this view for each
> customization change, also the ones not affecting bookmarks-toolbar-items..
> that's a little bit overkill :(

Ah, good point. I've opted to use onWidgetAdded instead, and check the ID of the item being added.

Do you think that covers all of the cases?
Attachment #8398026 - Attachment is obsolete: true
Attachment #8398620 - Flags: review?(mak77)

Comment 27

5 years ago
Comment on attachment 8398602 [details] [diff] [review]
Regression tests v1.2

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

Looks ok to me! Here's some random comments:

::: browser/components/customizableui/test/browser_984455_bookmarks_items_reparenting.js
@@ +99,5 @@
> + *
> + * @param aPopup the popup node to close.
> + */
> +function closePopup(aPopup) {
> +  return Task.spawn(function*() {

You don't need a Task.spawn nesting here; you could just return the hiddenPromise you create directly, for yielding in the calling function.

@@ +121,5 @@
> +    yield shownPromise;
> +
> +    yield checkContextMenu(chevronPopup);
> +    yield closePopup(chevronPopup);
> +  })

Nit: semicolon.

@@ +134,5 @@
> +  return Task.spawn(function*() {
> +    window.resizeTo(kSmallWidth, window.outerHeight);
> +    yield waitForCondition(() => gNavBar.hasAttribute("overflowing"));
> +    ok(gNavBar.hasAttribute("overflowing"), "Should have an overflowing toolbar.");
> +  })

Nit: add semicolon

Without the ok(), you wouldn't need the Task.spawn here. Is the ok() really necessary?

@@ +143,5 @@
> + * and returns a Promise that resolves when the nav-bar is no longer
> + * overflowing.
> + */
> +function stopOverflowing() {
> +  return Task.spawn(function*() {

You don't need Task.spawn here either, can just return the promise returned by waitForCondition();

@@ +248,5 @@
> +  checkNotOverflowing(kBookmarksItems);
> +
> +  yield checkBookmarksItemsChevronContextMenu();
> +
> +  placesToolbarItems.style.maxWidth = "";

Nit: placesToolbarItems.style.removeProperty("max-width");
Attachment #8398602 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8398602 [details] [diff] [review]
Regression tests v1.2

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

::: browser/components/customizableui/test/browser_984455_bookmarks_items_reparenting.js
@@ +77,5 @@
> +    EventUtils.synthesizeMouseAtCenter(dropmarker, {});
> +    info("Waiting for bookmarks menu popup to show after clicking dropmarker.")
> +    yield shownPromise;
> +
> +    for (let menuID in kSpecialItemIDs) {

Just a note that I didn't use for (let [k,v] of kSpecialItemIDs) {} because objects don't have @@iterators - so it can't be done straight-away.
(In reply to :Gijs Kruitbosch (no internet 29 Mar - 6 Apr) from comment #27)
> Comment on attachment 8398602 [details] [diff] [review]
> Regression tests v1.2
> 
> Review of attachment 8398602 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks ok to me! Here's some random comments:
> 
> :::
> browser/components/customizableui/test/
> browser_984455_bookmarks_items_reparenting.js
> @@ +99,5 @@
> > + *
> > + * @param aPopup the popup node to close.
> > + */
> > +function closePopup(aPopup) {
> > +  return Task.spawn(function*() {
> 
> You don't need a Task.spawn nesting here; you could just return the
> hiddenPromise you create directly, for yielding in the calling function.
> 

Good call. Done.

> @@ +121,5 @@
> > +    yield shownPromise;
> > +
> > +    yield checkContextMenu(chevronPopup);
> > +    yield closePopup(chevronPopup);
> > +  })
> 
> Nit: semicolon.
> 

Added.

> @@ +134,5 @@
> > +  return Task.spawn(function*() {
> > +    window.resizeTo(kSmallWidth, window.outerHeight);
> > +    yield waitForCondition(() => gNavBar.hasAttribute("overflowing"));
> > +    ok(gNavBar.hasAttribute("overflowing"), "Should have an overflowing toolbar.");
> > +  })
> 
> Nit: add semicolon
> 

Added.

> Without the ok(), you wouldn't need the Task.spawn here. Is the ok() really
> necessary?

That ok is absolutely useless. Removed, and just returned the promise.

> 
> @@ +143,5 @@
> > + * and returns a Promise that resolves when the nav-bar is no longer
> > + * overflowing.
> > + */
> > +function stopOverflowing() {
> > +  return Task.spawn(function*() {
> 
> You don't need Task.spawn here either, can just return the promise returned
> by waitForCondition();
> 

Good call. Fixed.

> @@ +248,5 @@
> > +  checkNotOverflowing(kBookmarksItems);
> > +
> > +  yield checkBookmarksItemsChevronContextMenu();
> > +
> > +  placesToolbarItems.style.maxWidth = "";
> 
> Nit: placesToolbarItems.style.removeProperty("max-width");

Done!
Attachment #8398602 - Attachment is obsolete: true
Attachment #8399422 - Flags: review+
Comment on attachment 8398620 [details] [diff] [review]
Patch v1.2

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

::: browser/base/content/browser-places.js
@@ +905,5 @@
>      }
> +    this._setupPlaceholder();
> +  },
> +
> +  uninit: function PTL_uninit() {

PTL?
Attachment #8398620 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [:mak] from comment #30)
> Comment on attachment 8398620 [details] [diff] [review]
> Patch v1.2
> 
> Review of attachment 8398620 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser-places.js
> @@ +905,5 @@
> >      }
> > +    this._setupPlaceholder();
> > +  },
> > +
> > +  uninit: function PTL_uninit() {
> 
> PTL?

Whoops - fixed, folded, and landed. Thanks for the review!

remote:   https://hg.mozilla.org/integration/fx-team/rev/8b94363b10f1
Status: NEW → ASSIGNED
Flags: in-testsuite+
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
Backed out for OSX debug mochitest-bc failures.
https://hg.mozilla.org/integration/fx-team/rev/87edfffb393d

https://tbpl.mozilla.org/php/getParsedLog.php?id=37152569&tree=Fx-Team
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
I've pushed a try build that dumps a dataURI with a screenshot of the browser just before the chevron is clicked, which might give me some clues:

https://tbpl.mozilla.org/?tree=Try&rev=2584136aebf6
So far, no luck reproducing this test failure locally, or on try - though it's possible my try push is sidestepping the failure by my additional patch that takes a screenshot just before the point of failure.
Ok, pushed a new patch that doesn't use my homebrewed screenshot, but overrides the popup timeout handler from head.js, so that if we hit this problem, mochitest should detect a timeout. In that case, a screenshot should be uploaded.

remote:   https://tbpl.mozilla.org/?tree=Try&rev=d4550c2dddb8
If I can't figure this out today, I think we should just disable the test for debug OS X builds - I don't really want to invest more time into it.
So this appears to be where we're locking up:

http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/7661388938374f2e65a110d8089bf4467b9eb30617a5a777b9cc6d1e2b47881aa4f199f8abdc09faed12d5ecff2de87a1eedc41fb0c3a73f5e910a84bd1a49f5

It looks like there's a context menu opened on the content.

I _think_ what's happening is that the popupshowing event is firing on the placesChevron context menu, and we immediately attempt to right-click at its center before its even populated itself.

I've pushed a patch to try that waits until one or more of the items under the placesChevron popup are not hidden:

https://tbpl.mozilla.org/?tree=Try&rev=df8b4301c6fd
uh, why is there a context menu at all there?, is one of the tests opening it?
(In reply to Marco Bonardo [:mak] from comment #38)
> uh, why is there a context menu at all there?, is one of the tests opening
> it?

So the test that is locking up is the test that puts the Bookmarks Toolbar Items into the nav-bar, forces it to overflow itself (but not overflow into the nav-bar overflow... ugh), clicks the places chevron, and right-clicks the chevron menupopup.

My hypothesis is that the context menu we're seeing is being opened in this test when we attempt to right-click the chevron menupopup before it's had a chance to un-hide the things inside itself.

I just tested that locally on OS X, and when all items under the menupopup are hidden, the popup itself is not visible.

So I think the right-click goes through to the underlying content, which opens up the contents context menu.

And THEN the placesChevron populates itself - you can see it (barely) in the background behind the content context menu. Why opening the content context menu doesn't hide the placesChevron popup... I have no idea.

Anyhow, that's my current theory.
are we waiting for popupshown before we right click?
(In reply to Marco Bonardo [:mak] from comment #40)
> are we waiting for popupshown before we right click?

Yes:

https://hg.mozilla.org/integration/fx-team/rev/8b94363b10f1#l6.120
I don't understand
yield checkContextMenu(chevronPopup);
chevronPopup doesn't sound like a context menu...
The checkContextMenu function takes a target from which a context menu can be opened from when right-clicking. In this case, that target is the chevronPopup.
Ok, waiting for those elements to be unhidden seems to have done the trick:

https://tbpl.mozilla.org/?tree=Try&rev=df8b4301c6fd

I'm going to rename that checkContextMenu function, because we're overriding it's original definition from head.js, and that's going to break things.

I won't waste anybody's time asking for review. I've got this. :)
Attachment #8398620 - Attachment is obsolete: true
Attachment #8399422 - Attachment is obsolete: true
Attachment #8402034 - Flags: review+
I'll reland once the tree re-opens. It's currently closed - apparently because of a leaking test I wrote. Lovely.
remote:   https://hg.mozilla.org/integration/fx-team/rev/97a6af89afd0
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/97a6af89afd0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 31
Comment on attachment 8402034 [details] [diff] [review]
984455 (r+'d by Gijs, mak, mikedeboer)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

Australis! Specifically, the toolbar overflow thing in the nav-bar.


User impact if declined: 

Users can get into a state where the context menus for some Bookmarks Menu Button folders, as well as Bookmark Toolbar Items items, can be all disabled.


Testing completed (on m-c, etc.): 

Lots of local testing, and a regression test was written. Time to bake on m-c.


Risk to taking this patch (and alternatives if risky): 

Pretty low risk, by my estimation.


String or IDL/UUID changes made by this patch:

None.
Attachment #8402034 - Flags: approval-mozilla-beta?
Attachment #8402034 - Flags: approval-mozilla-aurora?
Attachment #8402034 - Flags: approval-mozilla-beta?
Attachment #8402034 - Flags: approval-mozilla-beta+
Attachment #8402034 - Flags: approval-mozilla-aurora?
Attachment #8402034 - Flags: approval-mozilla-aurora+
Verified fixed on Windows 7 64bit using the STR from the description on:
- latest Nightly, build ID: 20140410030200
- latest Aurora, build ID: 20140410004003
- Fx 29 beta 7, build ID: 20140410150427
You need to log in before you can comment on or make changes to this bug.