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

VERIFIED FIXED in Firefox 13

Status

()

Firefox
Bookmarks & History
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: stef, Assigned: mak)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

13 Branch
Firefox 14
All
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox12 unaffected, firefox13+ verified, firefox14 verified)

Details

(Whiteboard: [qa!])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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)
(Assignee)

Comment 1

5 years ago
it's due to the native menubar, popup views don't work correctly on it
Component: Menus → Bookmarks & History
QA Contact: menus → bookmarks
(Assignee)

Updated

5 years ago
Assignee: nobody → mak77
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.
Depends on: 733419
(Assignee)

Comment 3

5 years ago
Created attachment 614793 [details] [diff] [review]
patch v1.0

This makes things better, and it's not that sucky.
Attachment #614793 - Flags: review?(mano)
(Assignee)

Comment 4

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

ehr, never read things often enough, typo!
Attachment #614793 - Attachment is obsolete: true
Attachment #614793 - Flags: review?(mano)
(Assignee)

Comment 5

5 years ago
Created attachment 614869 [details] [diff] [review]
patch v1.1
Attachment #614869 - Flags: review?
(Assignee)

Comment 6

5 years ago
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 :)
Attachment #614869 - Flags: review? → review?(mano)
Comment on attachment 614869 [details] [diff] [review]
patch v1.1

Thanks!
Attachment #614869 - Flags: review?(mano) → review+
(Assignee)

Comment 8

5 years ago
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.
hrm, not sure how you tested, but note that you're not checking which popup (event.target) got hidden.
(Assignee)

Updated

5 years ago
Summary: livemark items not clickable almost all the time → Removing nodes while a bookmarks popup is open in OS X native menubar zombifies them.
(Assignee)

Comment 10

5 years ago
Created attachment 615021 [details] [diff] [review]
patch v1.2

This one works much better, and no further listeners needed.
Attachment #614869 - Attachment is obsolete: true
Attachment #615021 - Flags: review?(mano)
(Assignee)

Comment 11

5 years ago
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 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.
Attachment #615021 - Flags: review?(mano) → review+
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfa2bf38f209
Target Milestone: Firefox 13 → Firefox 14
(Assignee)

Comment 14

5 years ago
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
Attachment #615025 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/dfa2bf38f209
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
status-firefox13: --- → affected
status-firefox14: --- → fixed
tracking-firefox13: --- → +
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.
Attachment #615025 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Updated

5 years ago
status-firefox12: --- → unaffected
(Assignee)

Comment 17

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/af82e3d9c229
status-firefox13: affected → fixed
(Assignee)

Updated

5 years ago
Blocks: 749112
Whiteboard: [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.
status-firefox13: fixed → verified

Comment 19

5 years ago
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
(Assignee)

Comment 20

5 years ago
(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

5 years ago
Considering comment19 and also comment20, setting the resolution to Verified Fixed
Status: RESOLVED → VERIFIED
status-firefox14: fixed → verified
Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.