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

Status: VERIFIED FIXED

(Firefox :: Keyboard Navigation, defect)

Description

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

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.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As what a quick looks show me it has been regressed between beta 4 and recent builds. I'll have a closer look now.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Regressed between the build 09050404 and 09050504:

pass: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c77b7d273dfb
fail: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e9e27c40d0af

Changesets: http://hg.mozilla.org/releases/mozilla-1.9.1/pushloghtml?fromchange=c77b7d273dfb&tochange=e9e27c40d0af

Could this have been regressed by the patch on bug 490401? CC'ing Dao.

Comment 3

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

This is Windows and Linux only, right?

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 5

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

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();
}

Comment 6

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

ops nevermind i misread that code, the problem is about mouse events clearly

Comment 7

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

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!

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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.

Comment 9

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

Anyone working on this bug? Should we re-assign?

Comment 10

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

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.

Comment 11

[[e-mail]]

Reassigning to gavin as he has written a patch.

As for the event handling, I was trying to achieve consistency.

Comment 12

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

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.

Comment 13

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

Attachment: browser-chrome test for dragging bookmarks on the toolbar

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.

Comment 14

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

Attachment: tentative patch used to test the above browser-chrome test

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.

Comment 15

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

Attachment: patch

Talked to Neil on IRC, we don't need to revert the mouseup changes (they aren't relevant to drag events).

Comment 16

[[e-mail]]

Comment on attachment 381389 [details] [diff] [review]
patch

Comment why we don't prevent default perhaps? (Deleted lines show up really badly in annotate!)

Comment 17

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

Attachment: with comment+test

Comment 18

[Dietrich Ayala (:dietrich)]

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

Comment 19

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

(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.

Comment 20

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

This is ready to land, yes?

Comment 21

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

Yes: https://hg.mozilla.org/mozilla-central/rev/55e06f106d73

Comment 22

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

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.

Comment 23

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

Also fails on Linux: https://hg.mozilla.org/mozilla-central/rev/9d6f5ab54933

Filed bug 496277 for that.

Comment 24

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

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.

Comment 25

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

https://hg.mozilla.org/releases/mozilla-1.9.1/rev/b35ad456cbf6

Comment 26

[Marcia Knous [:marcia]]

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.

Comment 27

[Marcia Knous [:marcia]]

Restoring the fixed collision as I think the mid air blew it away.

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+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