754405 - Drag&Drop doesn't work with Tabs->Bookmarks and other things

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Frank Helk]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120420145725

Steps to reproduce:

a) trying to drag & drop a tab to the bookmark toolbar
b) marking some text and try to drag&drop it into a seach box or similar


Actual results:

Drag & Drop doesn't work - nothing is dragged ... not even picked up.


Expected results:

a) Tab->Bokkmark should create a bookmark
b) Text should be entered into the search box

Comment 1

[Matthias Versen [:Matti]]

Please try it again in the Firefox safemode
http://support.mozilla.com/en-US/kb/Safe+Mode

Comment 2

[David E. Ross]

This is NOT a problem in SeaMonkey 2.9.1.  It thus appears to be a Toolkit regression.

Comment 3

[NoOp]

Confirm.

SeaMonkey 2.10b(1&2) 32bit and 64bit linux, and 32bit Windows: Drag &
Drop icon URL from the address bar to the bookmarks toolbar does not
work. You can get a up/down grey arrow (the location
indicator?), but that's it. I know that the actual url data is being
copied as I can drag & drop to a search bar (duckduckgo or google) in an
open tab. I can also D&D to another application. I also cannot drag &
drop a tab to the bookmarks toolbar (as indicated in 'a) trying to drag & drop a tab to the bookmark toolbar'. No issues in SeaMonkey 2.9.1 (linux or Windows).

Comment 4

[NoOp]

Added note: I cannot replicate on FireFox 12 (linux (32 or 64bit), WinXP, or Win7).

Comment 5

[Philip Chee]

Error: ReferenceError: titel is not defined
Source file: resource://app/modules/PlacesUIUtils.jsm
Line: 312

http://hg.mozilla.org/comm-central/annotate/95c5300d3398/suite/common/src/PlacesUIUtils.jsm#l312

Regression caused by Bug 732027 - Port |Bug 575955 - Replace internal usage of old transactions shim, add a new toolkit test| to SeaMonkey

Comment 6

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Fix 'titel' typo [Checked in: Comment 7 & 14 & 15]

[Approval Request Comment]
Regression caused by (bug #): bug 732027.
User impact if declined: broken features.
Risk to taking this patch (and alternatives if risky):
No risk, trivial.

Comment 7

[Serge Gautherie (:sgautherie)]

Comment on attachment 627534 [details] [diff] [review]
(Av1) Fix 'titel' typo
[Checked in: Comment 7 & 14 & 15]

http://hg.mozilla.org/comm-central/rev/0be35ebd63a2

Comment 8

[Frank Wein [:mcsmurf]]

Thanks for the quick fix!

Frank Helk: Did you test this with Firefox or SeaMonkey? Just wondering if Firefox still has this issue..

Comment 9

[Serge Gautherie (:sgautherie)]

(In reply to Frank Wein [:mcsmurf] from comment #8)
> Just wondering if Firefox still has this issue..

That was the only occurrence (left) in c-c.
You mean it was not a typo of yours? Which changeset did you copy it from?

Comment 10

[Frank Wein [:mcsmurf]]

No, it was a typo. But I wondered as the reporter reported this bug on Firefox (or did he actually meant to file this bug on SeaMonkey?).

Comment 11

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 627534 [details] [diff] [review]
(Av1) Fix 'titel' typo
[Checked in: Comment 7 & 14 & 15]

Note that bug 732027 didn't land on Aurora yet (but will have to, since it landed on Beta already), so we have a landing dependency here.

Speaking of the base bug, I feel we should no longer approve such "fixes" for Beta in the future. The base bug has caused two (!) regressions without much on the plus side of things. Beta should be for stabilization (fixing regressions, improving UX and the like), not jeopardizing the trust of our beta testers. While it's a valid goal to fix tests on all branches, I think we have to draw the line where we need to fix non-test code in order to fix a test. YMMV, but if it does, we should at least require more baking on trunk and more strict review in such cases.

Comment 12

[Frank Wein [:mcsmurf]]

Actually I agree with you, I would not have requested approval for the branches for my patch. But Serge did, I guess I cannot prevent other people from requesting approval ;-)

Comment 13

[Philip Chee]

Comment on attachment 627534 [details] [diff] [review]
(Av1) Fix 'titel' typo
[Checked in: Comment 7 & 14 & 15]

rs=me bustage fix

Comment 14

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 627534 [details] [diff] [review]
(Av1) Fix 'titel' typo
[Checked in: Comment 7 & 14 & 15]

http://hg.mozilla.org/releases/comm-beta/rev/6a1d10115b38

Comment 15

[Philip Chee]

Pushed to comm-aurora:
http://hg.mozilla.org/releases/comm-aurora/rev/95a771b566da

Comment 16

[Serge Gautherie (:sgautherie)]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #11)

> Speaking of the base bug, I feel we should no longer approve such "fixes"

Base bug being SM bug 732027, right?

> for Beta in the future. The base bug has caused two (!) regressions without
> much on the plus side of things. Beta should be for stabilization (fixing
> regressions, improving UX and the like), not jeopardizing the trust of our

Ftr, my rationale is/was to have as similar code to FF (branches) as possible :-|

> beta testers. While it's a valid goal to fix tests on all branches, I think
> we have to draw the line where we need to fix non-test code in order to fix
> a test.

Note that the 2 test patches of that bug had already landed, with no issue.

Comment 17

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #16)
> Note that the 2 test patches of that bug had already landed, with no issue.

I meant "my 2 patches : test + dead code removal".

Comment 18

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Serge Gautherie (:sgautherie) from comment #16)
> Base bug being SM bug 732027, right?

Yes.

> Ftr, my rationale is/was to have as similar code to FF (branches) as
> possible :-|

For test-only changes this is OK, but IMHO for code changes targeting comm-beta landing it's not. As I said, Beta is for stabilizing, and possibly regression normal operation (like happened here) just to fix some test or have more similar code is, IMHO, not a valid goal for that branch. If something breaks on Beta, we should be able to say "We gave our best to avoid such issues but still, this can happen. Sorry.", not "Oh sorry, I thought this was safe. Well, this is how we roll. Deal with it." After all, if the few beta users we have stop using that channel because we, despite the (relatively) long time the code has to stabilize (trunk -> Aurora -> Beta), needlessly regress things in between betas of the same target release, then this is entirely our fault.

> Note that the 2 test patches of that bug had already landed, with no issue.

Call it luck. As we can see from what happened here:
1. Even the smallest mistakes (typos, or shorthands typically used in FF land) can have a big effect.
2. Anyone can make mistakes. Patch authors, reviewers, approvers. I'm not excluding myself here; probably I was just lucky until now and was only partly responsible for bug 739049.