984455 - 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

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Ada [:adalucinet]]

Attachment: Image showing this issue

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

[:Gijs (he/him)]

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

Comment 2

[Alice0775 White]

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.

Comment 3

[:Gijs (he/him)]

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.

Comment 4

[Alice0775 White]

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

[:Gijs (he/him)]

(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.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Not tracking for now (based on Gijs comment #3 & #5)

Comment 7

[Mike Conley (:mconley) (:⚙️)]

I'll take a look at this today.

Comment 8

[Mike Conley (: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?

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 10

[:Gijs (he/him)]

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?

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8397126 [details] [diff] [review]
Patch v1

Something like this?

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.1

Updated based on mak's feedback. Regression test coming.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Attachment: Regression test

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Attachment: Regression test v1.1

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.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8398026 [details] [diff] [review]
Patch v1.1

Is this what you meant, Marco?

Comment 20

[:Gijs (he/him)]

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.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 22

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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 23

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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 :(

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Attachment: Regression tests v1.2

Going to do a quick self-review before requesting it from Gijs.

Comment 25

[Mike Conley (:mconley) (:⚙️)]

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

Alright, ready for you to kick the tires.

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.2

(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?

Comment 27

[:Gijs (he/him)]

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");

Comment 28

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Attachment: Regression tests v1.3 (r+'d by Gijs)

(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!

Comment 30

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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?

Comment 31

[Mike Conley (:mconley) (:⚙️)]

(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

Comment 32

[Ryan VanderMeulen [:RyanVM]]

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

Comment 33

[Mike Conley (:mconley) (:⚙️)]

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

Comment 34

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 35

[Mike Conley (:mconley) (:⚙️)]

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

Comment 36

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 37

[Mike Conley (:mconley) (:⚙️)]

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

Comment 38

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

uh, why is there a context menu at all there?, is one of the tests opening it?

Comment 39

[Mike Conley (:mconley) (:⚙️)]

(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.

Comment 40

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

are we waiting for popupshown before we right click?

Comment 41

[Mike Conley (:mconley) (:⚙️)]

(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

Comment 42

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I don't understand
yield checkContextMenu(chevronPopup);
chevronPopup doesn't sound like a context menu...

Comment 43

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 44

[Mike Conley (:mconley) (:⚙️)]

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. :)

Comment 45

[Mike Conley (:mconley) (:⚙️)]

Attachment: 984455 (r+'d by Gijs, mak, mikedeboer)

Comment 46

[Mike Conley (:mconley) (:⚙️)]

I'll reland once the tree re-opens. It's currently closed - apparently because of a leaking test I wrote. Lovely.

Comment 47

[Mike Conley (:mconley) (:⚙️)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/97a6af89afd0

Comment 48

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/97a6af89afd0

Comment 49

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 50

[Mike Conley (:mconley) (:⚙️)]

Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/c1fe4d4bfcec

Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/3f2d6f68c415

Comment 51

[Cornel Ionce [:noni] [Hubs QA]]

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