Cannot drag anymore folders on bookmarks toolbar (regression from bug 488752)

VERIFIED FIXED in Firefox 3.6a1

Status

()

defect
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: mak, Assigned: Gavin)

Tracking

({regression, verified1.9.1})

3.5 Branch
Firefox 3.6a1
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3.5 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Something must have regressed this recently, actually i'm unable to drag folders on toolbar, no way.

Asking blocking to make drivers aware, not sure if it should block but it's primary ui interaction.
Flags: blocking-firefox3.5?
As what a quick looks show me it has been regressed between beta 4 and recent builds. I'll have a closer look now.
This is Windows and Linux only, right?
Flags: blocking-firefox3.5? → blocking-firefox3.5+
No, it also stops working on OS X. I did some hg bisect with the following investigations:

pass: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/99009fc6ca75
fail: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/02dcf89388a5

Marco's patch doesn't touch any event handling so we think this should be a regression from Neil's patch on bug 488752.
Blocks: 488752
Yeah i think that patch touches menu frame so that it does not consume default event anymore for VK_DOWN. that event is probably the dragstart.

if ((keyCode == NS_VK_F4 && !keyEvent->isAlt) ||
    ((keyCode == NS_VK_UP || keyCode == NS_VK_DOWN) && keyEvent->isAlt)) {
  *aEventStatus = nsEventStatus_eConsumeNoDefault;
  ToggleMenuState();
}
Flags: blocking-firefox3.5+ → blocking-firefox3.5?
ops nevermind i misread that code, the problem is about mouse events clearly
So is this all platforms? The checkin implicates Mac-only, but the patch on bug 488752 affects all systems.

Assigning to Neil Rashbrook. Enn, if you get to this first, I won't complain. Also, a test to make sure we don't regress this again sure would be nice!
Component: Bookmarks & History → Keyboard Navigation
Flags: blocking-firefox3.5? → blocking-firefox3.5+
QA Contact: bookmarks → keyboard.navigation
Summary: Cannot drag anymore folders on bookmarks toolbar → Cannot drag anymore folders on bookmarks toolbar (regression from bug 488752)
(In reply to comment #7)
> So is this all platforms? The checkin implicates Mac-only, but the patch on bug
> 488752 affects all systems.

Yes, it's not OS X only. Tested also on Windows XP and Ubuntu 8.10.

> Assigning to Neil Rashbrook. Enn, if you get to this first, I won't complain.
> Also, a test to make sure we don't regress this again sure would be nice!

Seems that get lost by moving to another component. Restoring assignee.
Assignee: nobody → neil
Anyone working on this bug? Should we re-assign?
The problematic parts are the changes to mouse events, i can confirm that commenting out those parts solves the problem, but i can't guess why those changes have been pushed with a bug about keyboard navigation, so i would still need to ask to one of Neil's the reasons. Mousedown and mouseup have far more implications than key down and key up due to drag & drop, to me sounds like even if we open a menupopup we should not avoid default handling.
Reassigning to gavin as he has written a patch.

As for the event handling, I was trying to achieve consistency.
Assignee: neil → gavin.sharp
I'm working on a browser chrome test, not sure when it will be ready since these D&D tests are cumbersome. but hope to attach it here soon. Just removing changes to mouse events handling is enough to restore things here, btw, waiting on gavin's patch.
this doesn't want to be a fully complete test, i started investigating this kind of tests for Places, so it's a first approach that can be used to test this.

Gavin if you want to review this, i'm fine with that, since it lives in Places i'm asking dietrich for now.
Attachment #381371 - Flags: review?(dietrich)
just for reference this is the patch i've used to test that the test is doing its work, basicly a partial backout of bug 488752.
Posted patch patch (obsolete) — Splinter Review
Talked to Neil on IRC, we don't need to revert the mouseup changes (they aren't relevant to drag events).
Attachment #381373 - Attachment is obsolete: true
Attachment #381389 - Flags: superreview?(neil)
Attachment #381389 - Flags: review?(neil)
Comment on attachment 381389 [details] [diff] [review]
patch

Comment why we don't prevent default perhaps? (Deleted lines show up really badly in annotate!)
Attachment #381389 - Flags: superreview?(neil)
Attachment #381389 - Flags: superreview+
Attachment #381389 - Flags: review?(neil)
Attachment #381389 - Flags: review+
Attachment #381389 - Attachment is obsolete: true
Comment on attachment 381371 [details] [diff] [review]
browser-chrome test for dragging bookmarks on the toolbar


>+function synthesizeDragWithDirection(aElement, aExpectedDragData, aDirection) {

would be great to generalize this and move into the test framework

>+  {
>+    desc: "Drag a folder on toolbar",
>+    run: function() {
>+      // Create a test folder to be dragged.
>+      var folderId = PlacesUtils.bookmarks
>+                                .createFolder(PlacesUtils.toolbarFolderId,
>+                                              TEST_TITLE,
>+                                              PlacesUtils.bookmarks.DEFAULT_INDEX);
>+      var element = getToolbarNodeForItemId(folderId);
>+      isnot(element, null, "Found node on toolbar");
>+
>+      isnot(element.node, null, "Toolbar node has an associated Places node.");
>+      var expectedData = getExpectedDataForPlacesNode(element.node);
>+
>+      ok(true, "Dragging left");

not so hot on using ok() for debug... to bad there's not an info().

looks fine otherwise, r=me
Attachment #381371 - Flags: review?(dietrich) → review+
(In reply to comment #18)
> (From update of attachment 381371 [details] [diff] [review])
> 
> >+function synthesizeDragWithDirection(aElement, aExpectedDragData, aDirection) {
> 
> would be great to generalize this and move into the test framework

yes i was thinking in future to group drag tests and provide some common util. but it's too early to have a list of cases that could be handled by common code. EventUtils should probably provide more utils to test dragging, or make synthesizeDragStart more configurable. I would suggest to file a followup to expand it.
This is ready to land, yes?
Whiteboard: [can land]
Yes: https://hg.mozilla.org/mozilla-central/rev/55e06f106d73
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
I had to land https://hg.mozilla.org/mozilla-central/rev/69b64ba05f94 as a temporary bustage fix. The bookmark drag test was causing a page navigation, which in turn was causing a failure in browser_library_left_pane_commands.js (there were 2 history entries rather than 1 to delete):

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/places/tests/browser/browser_library_left_pane_commands.js | Visit has been removed
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/places/tests/browser/browser_library_left_pane_commands.js | Visit has been removed

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1244087343.1244090418.14714.gz&fulltext=1

I filed bug 496266.
Flags: in-testsuite+
thank you for taking care of the test, is the first test trying to synthesize a drag so it could effectively have some glitch. i'll take care of followups on it.
Verified fixed on the trunk using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090604 Minefield/3.6a1pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090604 Minefield/3.6a1pre. I can successfully drag folders on the toolbar.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1
Restoring the fixed collision as I think the mid air blew it away.
Keywords: fixed1.9.1
Verified fixed on 1.9.1 with builds on all platforms like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090605 Shiretoko/3.5pre ID:20090605031243
Target Milestone: --- → Firefox 3.6a1
Depends on: 496266
Depends on: 496277
Whiteboard: [can land]
You need to log in before you can comment on or make changes to this bug.