Closed Bug 519049 Opened 11 years ago Closed 11 years ago

Bookmarks Manager, Toolbar grays out after BM Search is entered

Categories

(SeaMonkey :: Bookmarks & History, defect)

defect
Not set

Tracking

(status1.9.2 .4-fixed)

RESOLVED FIXED
Tracking Status
status1.9.2 --- .4-fixed

People

(Reporter: therubex, Assigned: neil)

Details

(Keywords: fixed-seamonkey2.0, regression)

Attachments

(7 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.4pre) Gecko/20090925 SeaMonkey/2.0pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.4pre) Gecko/20090925 SeaMonkey/2.0pre

 
The Bookmarks Manager /Toolbar/ (New Folder | ... Properties | Rename | Delete) may gray out after you have performed a Search operation.
 


Reproducible: Always

Steps to Reproduce:
 
1. place focus on a bookmark
2. jump to the search box
3. search for that bookmark
(you will note that the search results has the same focus)
4. clear the search box
5. focus in bookmarks manager is (usually) back to where you left off
 
Actual Results:  
 
Bookmarks Manager /Toolbar/ is grayed out.

Expected Results:  
 
Bookmarks Manager /Toolbar/ should not be grayed out.
Confirming. What's really nasty is that the items keep being disabled even if you change the selection afterwards. :-(
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: wanted-seamonkey2.0?
OS: Windows XP → All
Hardware: x86 → All
Version: unspecified → Trunk
<InvisibleSmiley> the commands are updated correctly but the dependent menu items and buttons are not... do you have an idea?
After more testing it looks like selection is key here:
1. If something is selected when you enter or leave search mode all toolbar items are disabled. Selecting any other bookmark or separator makes no difference.
2. If you click a folder (in normal mode), the Delete button is enabled once you switch back to a bookmark or separator.
3. If you click the bookmarks root (in normal mode), the Move button is enabled.
4. If you remove the selection (Ctrl+click on the selected item), everything works again afterwards (!).
At least the three items under File>New can be fixed by simply replacing "observes" by "command" (I was wondering why those were not updated while Edit>Delete was and that's the outcome). Any patch added here should contain that change.

<http://mxr.mozilla.org/comm-central/source/suite/common/bookmarks/bookmarksManager.xul#120>
Is this a regression? If yes, what is the regression range?
(Now why didn't I think of that?)

07-21 works
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.2pre) Gecko/20090721 SeaMonkey/2.0b2pre

07-22 no windows build that I saw

07-23 mostly broken.  all Toolbar menu items are grayed out, except for Delete.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.2pre) Gecko/20090723 SeaMonkey/2.0b2pre

08-16 fully broken.  all Toolbar menu items are grayed out.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.3pre) Gecko/20090815 SeaMonkey/2.0b2pre
(In reply to comment #6)
> (Now why didn't I think of that?)
> 
> 07-21 works
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.2pre) Gecko/20090721
> SeaMonkey/2.0b2pre
> 
> 07-22 no windows build that I saw
> 
> 07-23 mostly broken.  all Toolbar menu items are grayed out, except for Delete.
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.2pre) Gecko/20090723
> SeaMonkey/2.0b2pre

Nice! A bit closer (with seamonkey-2.0b2pre.en-US.linux-i686.tar.bz2):
2009-07-22-00-comm-1.9.1 works
2009-07-23-01-comm-1.9.1 fails (Delete is activated after returning from search mode but all the other buttons are grayed out after entering/leaving search mode with a bookmark selected in the previous step)

-> maybe bug 495444 (I'm mostly blind-guessing here)? Neil, Olli?
Keywords: regression
Does SM bookmarks manager use nsIXULBuilderListeners? Or does it have some
strange assumptions about Rebuild time?
I don't see any safe way to change the functionality from what bug 495444 did.
(In reply to comment #8)
> Does SM bookmarks manager use nsIXULBuilderListeners?

Yes: <http://mxr.mozilla.org/comm-central/source/suite/common/bookmarks/bookmarksTree.xml#664>
(props to whoever added that comment!)

I have no idea at all whether that is related, though. By "blind guessing" I actually meant it could just as well be any other bug that fits the regression window. It wasn't really an educated guess, and if it was right that's mostly luck. ;-) Anyway, someone needs to verify this (through backout?).
(In reply to comment #9)
> (In reply to comment #8)
> > Does SM bookmarks manager use nsIXULBuilderListeners?
> 
> Yes:
> <http://mxr.mozilla.org/comm-central/source/suite/common/bookmarks/bookmarksTree.xml#664>

I'm pretty sure now this is involved in the problem. If you e.g. break on bookmarks.js line 867 (code after case "cmd_bm_newseparator" which is for the first of the buttons that do not correctly lose their disabled attribute), select a bookmark and search for a term that is included in the bookmark's title you will get this call stack:

#0: function anonymous(aTarget=Object:{2}, aSelection=Object:{12}, aCommand=string:"cmd_bm_newbookmark") in <chrome://communicator/content/bookmarks/bookmarks.js> line 868
#1: function anonymous(aTarget=Object:{2}, aSelection=Object:{12}) in <chrome://communicator/content/bookmarks/bookmarks.js> line 990
#2: function onCommandUpdate() in <chrome://communicator/content/bookmarks/bookmarksTree.xml> line 555
#3: function anonymous() in <chrome://communicator/content/bookmarks/bookmarksTree.xml> line 638
#4: function selectionChanged in x-jsd:native-code
#5: function onselect(event=Event:{0}) in <chrome://communicator/content/bookmarks/bookmarksManager.xul> line 1
#6: function selectEventsSuppressed in x-jsd:native-code
#7: function restoreSelection() in <chrome://communicator/content/bookmarks/bookmarksTree.xml> line 361
#8: function anonymous(aBuilder=XULTreeBuilder:{16}) in <chrome://communicator/content/bookmarks/bookmarksTree.xml> line 674
#9: function searchBookmarks(aInput=string:"acid") in <chrome://communicator/content/bookmarks/bookmarksTree.xml> line 465
#10: function oncommand(event=XULCommandEvent:{0}) in <chrome://communicator/content/bookmarks/bookmarksManager.xul> line 1
#11: function doCommand in x-jsd:native-code
#12: function _fireCommand(me=XULElement:{10}) in <chrome://global/content/bindings/textbox.xml> line 440

Here the anonymous function in bookmarksTree.xml line 674 is actually buildListener.didRebuild which calls the local function restoreSelection which finally sets selection.selectEventsSuppressed = false after restoring the selection.

Until this point, all buttons are still enabled correctly. If you continue three times from there (for the three cases, same call stack) the problem described here will show up: all the buttons are disabled.

Olli, Neil, is this something that could help you come up with a fix? Or in light of the imminent SM 2.0 code freeze, maybe a workaround?
Flags: wanted-seamonkey2.0? → wanted-seamonkey2.0+
(In reply to comment #4)
> At least the three items under File>New can be fixed by simply replacing
> "observes" by "command"
Which makes it look more like a broadcaster bug; menuitems and keys are special in that they don't use the broadcaster mechanism for command attributes. Instead keys check the disabled attribute on the command element and menupopups manually update the disabled attribute on menuitems when they open.
Yeah, I just debugged this; what happens is that there's a SetAttribute quickly followed by a RemoveAttribute; unfortunately as it's not safe to run scripts both broadcasts get delayed; even more unfortunately the attribute isn't set on the listener so that the removal gets "optimised" away...
(In reply to comment #12)
> even more unfortunately the attribute isn't set on
> the listener so that the removal gets "optimised" away...
I don't understand this part. What gets "optimized" away?
Or do you mean we detect "possible cycle" and don't do anything?
Neil said the patch doesn't work.

Perhaps http://mxr.mozilla.org/comm-central/source/suite/common/bookmarks/bookmarks.js#976 could set/remove attribute asynchronously using setTimeout?
Attached patch workaround (obsolete) — Splinter Review
(In reply to comment #16)
> Perhaps
> http://mxr.mozilla.org/comm-central/source/suite/common/bookmarks/bookmarks.js#976
> could set/remove attribute asynchronously using setTimeout?

That works. :-) Attached patch for discussion, not taking the bug since this is really just a workaround and I wouldn't be able to fix the root cause. Not obsoleting previous patch for the same reason. Not requesting review just yet to see which direction Neil wants to head.
Comment on attachment 405670 [details] [diff] [review]
workaround

I don't think we need to delay every single attribute change. The bug only surfaces when the selection changes twice because of a search, so the minimum fix would be to delay the command update triggered by the selection changing.
Attached patch Possible patchSplinter Review
This fixes it for me, but I have no idea whether this has any regressions.

Also, sorry for setting l to null, this is just a prototype patch ;-)
Attachment #405672 - Flags: review?(Olli.Pettay)
Attached patch Possible patchSplinter Review
This is an alternative idea I had, which also fixes this bug. (Note: -w diff. Usual disclaimers apply.)
Attachment #405687 - Flags: review?(Olli.Pettay)
Neither seem to regress bug 470687 or bug 471416.

Attachment 405672 [details] [diff] wouldn't work well if some mutation listener changes the value
which was just set because of broadcasting. The change wouldn't add new
item to mDelayedAttrChangeBroadcasts, but just modifying the existing one, which
is just being broadcasted.

Attachment 405687 [details] [diff] looks better, but it might lead to problems in the loop in
nsXULDocument::MaybeBroadcast(). Removing items from 
mDelayedAttrChangeBroadcasts during iteration would mess with indexing.
Attachment #405672 - Flags: review?(Olli.Pettay) → review-
Attachment #405687 - Flags: review?(Olli.Pettay) → review-
Attached patch untestedSplinter Review
So if attachment 405687 [details] [diff] [review] works, could this work too?
Anyone with Seamonkey build at hand, want to test?
(In reply to comment #22)
> Created an attachment (id=405698) [details]
> untested
> 
> So if attachment 405687 [details] [diff] [review] works, could this work too?
> Anyone with Seamonkey build at hand, want to test?

Unfortunately it doesn't work. I can confirm that Neil's patch (attachment 405687 [details] [diff] [review]) works, though, so at least I'm confident that my test environment is OK. :-)

My own try in comment 17 (attachment 405670 [details] [diff] [review]) was wrong BTW. Not only because I forgot to change the variable name in enableCommand (passed in: aCommand, used: aCommandNode)--I corrected that locally--but also because it regressed the normal view which now didn't correctly disable buttons anymore (e.g. Delete should be disabled for the bookmarks root and the Personal Toolbar Folder and Move for the former). I tried to come up with a way to only use setTimeout if coming from selectionChanged but it seems to normal view also goes through that... In the end I decided to abandon that approach and let you guys pursuit the correct fix. Better no improvement for 2.0 than something worse than what we have now (which can be worked around by clearing the selection).
(In reply to comment #23) 
> Unfortunately it doesn't work. I can confirm that Neil's patch (attachment
> 405687 [details]) works
Thanks for testing.
The result hints that Neil's patches actually do break something, which is why
they just happen to fix the bug here. (if my analysis even close to right.)
Attached patch Possible patchSplinter Review
I can confirm that attachment 405698 [details] [diff] [review] is no better than attachment 405667 [details] [diff] [review].

This patch improves on attachment 405687 [details] [diff] [review] by checking the value of the mHandlingDelayedAttrChange flag. If a delayed broadcast is found while the flag is set then a broadcast loop is assumed and the broadcast is aborted. Otherwise it is safe to remove the existing delayed broadcast, thus fixing the bug.
Attachment #405714 - Flags: review?(Olli.Pettay)
Comment on attachment 405714 [details] [diff] [review]
Possible patch

This is again -w patch?

But yeah, I think this should work.
Attachment #405714 - Flags: review?(Olli.Pettay) → review+
Attached patch Proposed patchSplinter Review
Correct, attachment 405714 [details] [diff] [review] was with -w. This is a normal diff.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #405717 - Flags: superreview?(bzbarsky)
Attachment #405717 - Flags: review+
Attachment #405717 - Flags: superreview?(bzbarsky) → superreview+
Pushed changeset 3007520289b2 to mozilla-central.
Attached patch Workaround (obsolete) — Splinter Review
Attachment #405670 - Attachment is obsolete: true
Attachment #406030 - Flags: review?(iann_bugzilla)
Comment on attachment 406030 [details] [diff] [review]
Workaround

>diff -r 0d3261f63a0e suite/common/bookmarks/bookmarksMenu.js
>--- a/suite/common/bookmarks/bookmarksMenu.js	Mon Oct 12 20:42:32 2009 +0200
>+++ b/suite/common/bookmarks/bookmarksMenu.js	Tue Oct 13 17:20:25 2009 +0100

I wonder how you created that patch... Correct file is bookmarksTree.xml. ;-)

Other than that: Nice work, patch fixes the problem. :-)
Attached patch WorkaroundSplinter Review
D'oh!
Attachment #406030 - Attachment is obsolete: true
Attachment #406038 - Flags: review?(iann_bugzilla)
Attachment #406030 - Flags: review?(iann_bugzilla)
Attachment #406038 - Flags: review?(iann_bugzilla)
Attachment #406038 - Flags: review+
Attachment #406038 - Flags: approval-seamonkey2.0+
(In reply to comment #31)
> Created an attachment (id=406038) [details]

This was pushed as <http://hg.mozilla.org/comm-central/rev/3262ccf994a9>.
Yeah, yeah, so I didn't get around to updating Bugzilla...
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment on attachment 405717 [details] [diff] [review]
Proposed patch

Let's see how many branches we can get this regression fixed on.
Attachment #405717 - Flags: approval1.9.2?
Comment on attachment 405717 [details] [diff] [review]
Proposed patch

approval1.9.2 requests aren't currently being monitored, since we're nearing RC freeze and there are too many outstanding requests, so I'm clearing this request. Feel free to re-request approval if you are confident that it's worth drivers' time to consider whether this non-blocker needs to land for 1.9.2 at this stage.
Attachment #405717 - Flags: approval1.9.2?
Comment on attachment 405717 [details] [diff] [review]
Proposed patch

Fixes another regression that SeaMonkey would appreciate the possibility to remove its workaround for.
Attachment #405717 - Flags: approval1.9.2?
Comment on attachment 405717 [details] [diff] [review]
Proposed patch

Pushing approval request for this regression to the next branch release.
Attachment #405717 - Flags: approval1.9.2.1?
Attachment #405717 - Flags: approval1.9.2?
Comment on attachment 405717 [details] [diff] [review]
Proposed patch

Moving to 1.9.2.3 - what's the test coverage like for this?
Attachment #405717 - Flags: approval1.9.2.2? → approval1.9.2.3?
I might be able to write a test for this, but it depends on how easy it is to set an attribute when it's unsafe to run script as a result.
Comment on attachment 405717 [details] [diff] [review]
Proposed patch

Approved for 1.9.2.4, a=dveditz for release-drivers
Attachment #405717 - Flags: approval1.9.2.4? → approval1.9.2.4+
Pushed changeset 82758e3efb8f to releases/mozilla-1.9.2
Does someone from the Seamonkey community want to verify that this is fixed with a nightly 1.9.2 build?
Actually we don't have 1.9.2 builds any more. (We did when the request was originally made.)
You need to log in before you can comment on or make changes to this bug.