Last Comment Bug 741506 - Removing nodes while a bookmarks popup is open in OS X native menubar zombifies them.
: Removing nodes while a bookmarks popup is open in OS X native menubar zombifi...
Status: VERIFIED FIXED
[qa!]
:
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: 13 Branch
: All Mac OS X
: -- normal (vote)
: Firefox 14
Assigned To: Marco Bonardo [::mak]
:
:
Mentors:
Depends on: 733419
Blocks: 749112 livemarksIO
  Show dependency treegraph
 
Reported: 2012-04-02 12:25 PDT by Stefan Plewako [:stef]
Modified: 2012-06-13 23:25 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
verified
verified


Attachments
patch v1.0 (4.62 KB, patch)
2012-04-13 08:20 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
patch v1.1 (4.97 KB, patch)
2012-04-13 12:01 PDT, Marco Bonardo [::mak]
asaf: review+
Details | Diff | Splinter Review
patch v1.2 (5.60 KB, patch)
2012-04-14 04:17 PDT, Marco Bonardo [::mak]
asaf: review+
Details | Diff | Splinter Review
Reduced Aurora patch (4.17 KB, patch)
2012-04-14 05:35 PDT, Marco Bonardo [::mak]
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Stefan Plewako [:stef] 2012-04-02 12:25:28 PDT
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] 2012-04-02 13:36:56 PDT
it's due to the native menubar, popup views don't work correctly on it
Comment 2 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-04-03 06:17:06 PDT
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] 2012-04-13 08:20:28 PDT
Created attachment 614793 [details] [diff] [review]
patch v1.0

This makes things better, and it's not that sucky.
Comment 4 Marco Bonardo [::mak] 2012-04-13 08:21:51 PDT
Comment on attachment 614793 [details] [diff] [review]
patch v1.0

ehr, never read things often enough, typo!
Comment 5 Marco Bonardo [::mak] 2012-04-13 12:01:31 PDT
Created attachment 614869 [details] [diff] [review]
patch v1.1
Comment 6 Marco Bonardo [::mak] 2012-04-13 12:03:24 PDT
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-04-13 12:20:02 PDT
Comment on attachment 614869 [details] [diff] [review]
patch v1.1

Thanks!
Comment 8 Marco Bonardo [::mak] 2012-04-13 13:47:46 PDT
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-04-14 00:50:33 PDT
hrm, not sure how you tested, but note that you're not checking which popup (event.target) got hidden.
Comment 10 Marco Bonardo [::mak] 2012-04-14 04:17:34 PDT
Created attachment 615021 [details] [diff] [review]
patch v1.2

This one works much better, and no further listeners needed.
Comment 11 Marco Bonardo [::mak] 2012-04-14 04:20:53 PDT
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-04-14 04:47:29 PDT
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 14 Marco Bonardo [::mak] 2012-04-14 05:35:30 PDT
Created attachment 615025 [details] [diff] [review]
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 (⌚ UTC+1/+2) 2012-04-15 04:35:44 PDT
https://hg.mozilla.org/mozilla-central/rev/dfa2bf38f209
Comment 16 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-16 15:30:05 PDT
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 18 Simona B [:simonab ] 2012-05-09 08:25:47 PDT
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] 2012-06-12 02:59:33 PDT
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] 2012-06-13 07:17:38 PDT
(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] 2012-06-13 23:25:50 PDT
Considering comment19 and also comment20, setting the resolution to Verified Fixed

Note You need to log in before you can comment on or make changes to this bug.