741506 - Removing nodes while a bookmarks popup is open in OS X native menubar zombifies them.

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Stefan Plewako [:stef]]

When I'm trying to use live bookmarks via global/app menu (on mac, 10.6.8) all folders almost all the time have the grayed out "live bookmarks loading..." message.

When this loading message is visible, I could click on any already loaded item but nothing will happen… (ie related webpage won't open)

Comment 1

[Marco Bonardo [:mak]]

it's due to the native menubar, popup views don't work correctly on it

Comment 2

[Asaf Romano (gone)]

It appears that this happens if & when the callback in _populateLivemarkPopup cleans the popup while the menu is open.  Because the menuitem isn't updated lively, the new items don't show up, and the old items don't go away, but they're non-functional at this point. They're removed from the document and thus are no longer children of the bookmarks menu, on which the command-listener is set.

There are few possible workaround, non of which is simple enough:
1. For native menus, check in the callback that the menu is not open. If it is, do the work in a popuphide listener.
2. For native menus, always build the contents (if they're already available) in popupshowing, and, in getLivemarks' callback, only cache the new data.
3. Set the command listener on each livemark-item. I would expect this to work k given the way the command event is dispatched from native menus, but I'm not sure.

Comment 3

[Marco Bonardo [:mak]]

Attachment: patch v1.0

This makes things better, and it's not that sucky.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 614793 [details] [diff] [review]
patch v1.0

ehr, never read things often enough, typo!

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 614869 [details] [diff] [review]
patch v1.1

ehr, clicked too fast.
This is a better/more generic approach.
The defect is that if the popups should at once be working properly (so someone fixes them) there will be double entries, but it's a feature, so someone will file a bug and we'll know about that :)

Comment 7

[Asaf Romano (gone)]

Comment on attachment 614869 [details] [diff] [review]
patch v1.1

Thanks!

Comment 8

[Marco Bonardo [:mak]]

hm, sigh, after testing again, I was able to reproduce the bug even with the patch, not sure why it was working before, the random behavior of this sucks.
Back to the drawing board.

Comment 9

[Asaf Romano (gone)]

hrm, not sure how you tested, but note that you're not checking which popup (event.target) got hidden.

Comment 10

[Marco Bonardo [:mak]]

Attachment: patch v1.2

This one works much better, and no further listeners needed.

Comment 11

[Marco Bonardo [:mak]]

now, if I could use popup.state, would be a more generic fix :) Btw, I thought you had a patch somewhere to add frames to these popups?

Comment 12

[Asaf Romano (gone)]

Comment on attachment 615021 [details] [diff] [review]
patch v1.2

For Fx-13, you might want to take out the status-menu-item changes. r=me beforehand for that.

About popupstate: Even if we added frames (I did that, back then, for the purpose of binding attachment and style-changes tracking), it probably wouldn't fix popup-state, it's actually quite strange that native popups have popup-box-objects.

What a nightmare. Thanks again for fixing this.

Comment 13

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfa2bf38f209

Comment 14

[Marco Bonardo [:mak]]

Attachment: Reduced Aurora patch

reduced version for Aurora

[Approval Request Comment]
Regression caused by (bug #): bug 613588
User impact if declined: Some bookmarks generated from the OS X menu bar are zombified, so when clicked, do nothing.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): The patch is limited to the problematic case, acts only on the os x menubar and only if the popup is open.  So changes are basically limited to the already broken cases.
String changes made by this patch: none

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/dfa2bf38f209

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 615025 [details] [diff] [review]
Reduced Aurora patch

[Triage Comment]
Sounds like a really nasty user experience, so approving for uplift to Aurora - can you please confirm that this is not present on 12 since there's no regression mention in the comments I'm not sure how to tell if this is a completely new issue or something we need to look at backporting.

Comment 17

[Marco Bonardo [:mak]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/af82e3d9c229

Comment 18

[Simona Badau, Desktop QA]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20100101 Firefox/13.0

Verified on Firefox 13 beta 3 that the feed items are loading when accessed via the native menu bar.

Comment 19

[Vlad [QA]]

I've verified this on:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:14.0) Gecko/20100101 Firefox/14.0 beta 6

I've subscribed to cnn.com and when accessing the rss feed from the menu (Bookmarks>Bookmarks Toolbar>cnn.com) the  "Live Bookmark loading..." message appears and it's grayed out.
Only after I repeat the steps above, I can actually see the rss feeds. This is happening every time when I restart the browser.
Is this intended?

And one other thing, there is no context menu on right click. This is intended also?

Thanks

Comment 20

[Marco Bonardo [:mak]]

(In reply to Vlad [QA] from comment #19)
> I've subscribed to cnn.com and when accessing the rss feed from the menu
> (Bookmarks>Bookmarks Toolbar>cnn.com) the  "Live Bookmark loading..."
> message appears and it's grayed out.
> Only after I repeat the steps above, I can actually see the rss feeds. This
> is happening every time when I restart the browser.
> Is this intended?

Not intended, but it's bug 733419, basically on Mac menupopus update only on popupshowing, they never live-update.

> And one other thing, there is no context menu on right click. This is
> intended also?

yes, on Mac.

Comment 21

[Vlad [QA]]

Considering comment19 and also comment20, setting the resolution to Verified Fixed