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

RESOLVED FIXED in Firefox 3 beta4

Status

()

Firefox
Bookmarks & History
P3
normal
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: WG9s, Assigned: WG9s)

Tracking

Trunk
Firefox 3 beta4
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

11 years ago
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.
Flags: blocking-firefox3?
Sounds pretty bad. Reed/Ventnor, can you look into this and see if you can find a regression range?
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P3
(Assignee)

Comment 2

11 years ago
I might have some time to try to narrow this down tonight.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → NEW
(Assignee)

Comment 3

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

Comment 4

11 years ago
I have a patch to fix this.  I will attach it here as soon as I make sure it works under windows as well.
Status: NEW → ASSIGNED
Component: Menus → Places

Comment 5

11 years ago
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

11 years ago
for "it should not fire" i mean "it should not set closeTime"
(Assignee)

Comment 7

11 years ago
Created attachment 304522 [details] [diff] [review]
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.
(Assignee)

Comment 8

11 years ago
Created attachment 304563 [details] [diff] [review]
patch version 1

This removes the code the workaraound made non-reachable.
Attachment #304522 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #304563 - Flags: review?(mano)

Comment 9

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

Comment 10

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

Comment 11

11 years ago
(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 mean dragging over "bookmarks" in the menubar, then move on the side (toward "history"), sorry i was not clear enough...
(Assignee)

Comment 13

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

Comment 14

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

Comment 15

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

Comment 16

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

Comment 17

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

Comment 18

11 years ago
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.
Assignee: nobody → bill
Status: ASSIGNED → NEW
QA Contact: menus → places
(Assignee)

Updated

11 years ago
Blocks: 419740
(Assignee)

Comment 19

11 years ago
Created attachment 306614 [details] [diff] [review]
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.
Attachment #304563 - Attachment is obsolete: true
Attachment #306614 - Flags: review?
Attachment #304563 - Flags: review?(mano)
(Assignee)

Updated

11 years ago
Attachment #306614 - Flags: review? → review?(mano)
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.
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?
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
Status: NEW → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 beta4
No, I don't mind, too bad i noticed only after checkin...
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.
Attachment #306614 - Flags: approval1.9b4?
Backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 27

11 years ago
Created attachment 306785 [details] [diff] [review]
patch v3

As long as this needs to be re-done, may as well remove _closePopups.
Attachment #306785 - Flags: review?(mano)
Attachment #306614 - Attachment is obsolete: true
Attachment #306614 - Flags: approval1.9b4?
Comment on attachment 306785 [details] [diff] [review]
patch v3

a=beltzner for 1.9b4, please land it tonight to get it into nightlies
Attachment #306785 - Flags: approval1.9b4? → approval1.9b4+
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
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
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.
(Assignee)

Comment 31

11 years ago
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.
Blocks: 407633
No longer blocks: 407633
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.
Severity: normal → minor
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

10 years ago
Flags: blocking-firefox3+
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.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago10 years ago
Resolution: --- → FIXED
Severity: minor → normal
Flags: blocking-firefox3?

Updated

10 years ago
Flags: blocking-firefox3?
Flags: blocking-firefox3+
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.