418156 - Attempting to Drag and Drop in Bookmarks menu results in broken close windows button

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[Bill Gianopoulos [:WG9s]]

This appears to be Linux only.

If you click on Bookmarks in the main browser window and then left click on an item and start to move it within the bookmarks menu, after about 5 seconds the menu will be dismissed and the selected item will still show until you release the mouse button.  At this point things are hosed in such a manner that the windows close button on the browser window no longer functions.

You can, however, close the window via File -> Quit.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Sounds pretty bad. Reed/Ventnor, can you look into this and see if you can find a regression range?

Comment 2

[Bill Gianopoulos [:WG9s]]

I might have some time to try to narrow this down tonight.

Comment 3

[Bill Gianopoulos [:WG9s]]

I found the checkin that caused the regression, but that is not really very useful information.  It is bug 381281, which is the bug that fixed drag and drop within the bookmarks menu to work at all.

However, prior to that checkin, drag and drop within the bookmarks menu was never available under Linux.  This was a Windows only feature prior to the places landing.  Under places it seems the enable test was changed from windows only to everything except MAC.

SO, although it would be good to fix this, the good news is that if this can't be figured out before Firefox 3 is done, we can always go back to making this be a windows only feature without regressing any Firefox 2 behavior.

Comment 4

[Bill Gianopoulos [:WG9s]]

I have a patch to fix this.  I will attach it here as soon as I make sure it works under windows as well.

Comment 5

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

Bill, i'm about changing much code in menu.xml for the dropmarker (and fixing bogus overfolder code) so please don't rely on that.

About this problem i see that mainly is because onDragExit in PlacesMenuDNDController is completely broken, it should not fire when dragging on a child of the menu (notice that the same code is also serving the toolbar) 

Comment 6

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

for "it should not fire" i mean "it should not set closeTime"

Comment 7

[Bill Gianopoulos [:WG9s]]

Attachment: Workaraound

This is the original patch I made to test to see if this would regress windows.  This is sufficient to avoid the issue here and also avoids the regressions I reported in bug 389931 after trying the patch for that bug.

This makes a lot of code in browser-places.js be essentially dead.  Evidently that code was either never needed in the first place, or a subsequent fix elsewhere made it no longer necessary.  I am working on a patch to remove all the code that is no longer executed with this patch in place, but have yet to get that to work quite right.  I should have something either later today or tomorrow that will be ready for review.

Comment 8

[Bill Gianopoulos [:WG9s]]

Attachment: patch version 1

This removes the code the workaraound made non-reachable.

Comment 9

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

mh, what actually happens if you drag on the menu and then drag out of it on the left/right without moving to the actual bookmarks menu? is the menu dismissed?

Comment 10

[Bill Gianopoulos [:WG9s]]

The menu is dismissed, and the item you were dragging then opens because you dragged it to the content area.  That kind of makes sense to me.  Let me wait for my windows build to complete to make sure it behaves the same way there.

Comment 11

[Bill Gianopoulos [:WG9s]]

(In reply to comment #10)
> The menu is dismissed, and the item you were dragging then opens because you
> dragged it to the content area.  That kind of makes sense to me.  Let me wait
> for my windows build to complete to make sure it behaves the same way there.
> 

Oh dear.  in that case you still end up with the non-functioning close button.  so although I fixed all the premature closing issues, the issue of the non-functional close button under Linux still remains.

Comment 12

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

i mean dragging over "bookmarks" in the menubar, then move on the side (toward "history"), sorry i was not clear enough...

Comment 13

[Bill Gianopoulos [:WG9s]]

It is odder that that.  It seems almost all the time the menu does not dismiss.  In the cases where it does not, the close button works fine.  It is on the rare occasions where it dismisses that the close button gets hosed.  So there is something else amiss here.

Comment 14

[Bill Gianopoulos [:WG9s]]

(In reply to comment #12)
> i mean dragging over "bookmarks" in the menubar, then move on the side (toward
> "history"), sorry i was not clear enough...
> 

That seems to work fine.

Comment 15

[Bill Gianopoulos [:WG9s]]

(In reply to comment #11)
> (In reply to comment #10)
> > The menu is dismissed, and the item you were dragging then opens because you
> > dragged it to the content area.  That kind of makes sense to me.  Let me wait
> > for my windows build to complete to make sure it behaves the same way there.
> > 
> 
> Oh dear.  in that case you still end up with the non-functioning close button. 
> so although I fixed all the premature closing issues, the issue of the
> non-functional close button under Linux still remains.
> 

I seem to be completely unable to duplicate this behavior.  I suspect that when I tried this I was accidentally running a build without my patch.

Comment 16

[Bill Gianopoulos [:WG9s]]

(In reply to comment #13)
> It is odder that that.  It seems almost all the time the menu does not dismiss.
>  In the cases where it does not, the close button works fine.  It is on the
> rare occasions where it dismisses that the close button gets hosed.  So there
> is something else amiss here.
> 

OK.  I have decided that the one time I got the menu closing by itself and the non-functional close button I must have been running the wrong build because I am completely unable to duplicate that now with a build containing my patch.

I also did testing on a windows Firefox 2 build and the behavior that dragging form the bookmarks menu to the content area opens the link without dismissing the bookmarks menu is identical to the Firefox 2 behavior.

So,k everything seems to behave as expected so far in my testing without any of this code.

The code was all originally added to fix bug 329733.

Comment 17

[Bill Gianopoulos [:WG9s]]

(In reply to comment #16)
> So,k everything seems to behave as expected so far in my testing without any of
> this code.
Well this was not very clear what I was trying to say is everything seems to work as expected with this patch installed which removes all of this code.  I did not mean things work without the patch.

Comment 18

[Bill Gianopoulos [:WG9s]]

I figured out what was breaking the close window button here and it is not as bad as it originally seemed.  The problem was that besides this timer code closing the menu in cases when it should not have, it was not doing it correctly either.

It is perfectly normal for the close button to be ignored when a menu is open.  A click on the close button is treated the same as any other click outside the menu, it does not close the window, but merely dismisses the menu.

The bogus close code based on these timers was kind of closing the menu but not clearing all the appropriate menu is open conditions such that the click on the close button was still being ignored because a flag was set saying a menu was open but the click was not clearing the flag since there was not really a menu open.  So, not matter how many times you clicked, the window would not close.

Comment 19

[Bill Gianopoulos [:WG9s]]

Attachment: patch v2

I should have figured it was too good to be true that all of that code could be removed.  Someone noted a regression using my test build.  "if I drag an item from the toolbar/location bar to the menu the menu doesn't open like in trunk."  This is a more conservative patch which merely removes the timer that causes the issue.  I had the tester verify that this cleared the new regression that the previous patch introduced.

Comment 20

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

this is about the same thing i've done in the last month to test bookmarks menus D&D so i can somehow confirm that makes things better.

Comment 21

[Asaf Romano (gone)]

Comment on attachment 306614 [details] [diff] [review]
patch v2

r=mano

Comment 22

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

Mano: should we also remove the _closePopups functions since it's no more used (is called by that timer) or do you want to retain it for future work?

Comment 23

[Asaf Romano (gone)]

mozilla/browser/base/content/browser-menubar.inc 1.134
mozilla/browser/base/content/browser-places.js 1.103
mozilla/browser/base/content/browser.xul 1.438

Comment 24

[Asaf Romano (gone)]

No, I don't mind, too bad i noticed only after checkin...

Comment 25

[Reed Loden [:reed]]

Comment on attachment 306614 [details] [diff] [review]
patch v2

This completely landed without approval. Please see the top of tinderbox or http://wiki.mozilla.org/TreeStatus before landing things without approval.

Comment 26

[Asaf Romano (gone)]

Backed out.

Comment 27

[Bill Gianopoulos [:WG9s]]

Attachment: patch v3

As long as this needs to be re-done, may as well remove _closePopups.

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 306785 [details] [diff] [review]
patch v3

a=beltzner for 1.9b4, please land it tonight to get it into nightlies

Comment 29

[Reed Loden [:reed]]

Checking in browser/base/content/browser-menubar.inc;
/cvsroot/mozilla/browser/base/content/browser-menubar.inc,v  <--  browser-menubar.inc
new revision: 1.136; previous revision: 1.135
done
Checking in browser/base/content/browser-places.js;
/cvsroot/mozilla/browser/base/content/browser-places.js,v  <--  browser-places.js
new revision: 1.105; previous revision: 1.104
done
Checking in browser/base/content/browser.xul;
/cvsroot/mozilla/browser/base/content/browser.xul,v  <--  browser.xul
new revision: 1.440; previous revision: 1.439
done

Comment 30

[Matthew Gregan [:kinetik]]

I ran into this a while ago and filed it as bug #407633. I wonder if I should close that bug as a duplicate of this one and assume my analysis was wrong, or if there's still a core bug lurking.

Comment 31

[Bill Gianopoulos [:WG9s]]

My advice would be to try to run builds not containing the patch you have int hat bug and try to trigger the issue.  If you can document the steps to reproduce in that bug.  If you are unable to trigger the issue, I would then probably close it as a dupe.

Comment 32

[Paul Craciunoiu [:paulc]]

Reproduced on OS: Linux/Ubuntu 8.04
build: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9) Gecko/2008060309 Firefox/3.0

I played around quite a bit before I got it, but if you're not on top of the Bookmarks drop-down while dragging, menu disappears and the browser window doesn't close from the button (but closes from File -> Quit).

Reopened bug + marked as minor.

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Paul: please file a new bug for that issue? It's not clear that it's the same problem as the one fixed here, and we're not going to be backing this patch out.

Comment 34

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv