Closed Bug 193568 Opened 22 years ago Closed 22 years ago

Can't drag the selected text that is not anchor

Categories

(SeaMonkey :: General, defect)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: masayuki, Assigned: john)

References

Details

(Keywords: regression, Whiteboard: fixed1.3)

Attachments

(1 file, 5 obsolete files)

User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b) Gecko/20030216 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b) Gecko/20030216 I can't drag the text in any pages. Reproducible: Always Steps to Reproduce: 1. Select this text. www.mozilla.org 2. Drag the selected text 3. Actual Results: I can't drag the selected text and drop to tabber or other application. Expected Results: The selected text is able to drag and drop to tabber or other application. I could drag with 2003021408-trunk/WinXP. But, I can't drag with 2003021504 and 2003021604/WinXP and Win2000. But, If I selected the text between some elements, I can drag the text when mouse downed at center of the selected text.
Summary: Can't drag the text that is not anchor → Can't drag the selected text that is not anchor
This bug is regression of Bug 103055 ?
Severity: normal → major
This problem isn't convenience for BBS of Japanese web site. When we write url other site, the url is not anchor text. Because many people don't like the other web site master to get HTTP_REFERER. -> blocking1.3?
Flags: blocking1.3?
To jkeiser
Assignee: asa → jkeiser
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Test cases, here. http://bugzilla.mozilla.gr.jp/attachment.cgi?id=1503&action=view I can drag with Case2 and 4. but this is not always. 1. Must select the center span element. 2. Must select the left and right side text of span element. 3. Must down button of the mouse on the center span element.
Backing out the portion of the bug 103055 patch that retargets mouse events at the nearest element parent fixes this. Not that that's the best end solution necessarily, mind you.
Status: NEW → ASSIGNED
Blocks: 185889
John, this is a major loss of functionality. We need this fixed for 1.3. What's the right thing to do for 1.3? Backout offending portion of 103055 or come up with a better fix?
Flags: blocking1.3? → blocking1.3+
text dragging is Broken in Mozilla for BeOS as well
If I don't come up with a fix very soon, we will back out the offending portion of bug 103055.
The problem here is that nsContentAreaDragDrop::BuildDragData is not starting the drag, because the target node (the parent of the text) is not in the selection. This is a legitimate bug with the way this stuff works: if you set the mouse near the edge of the text, and drag, it will perform selection instead of dragging, because the mouse gesture event does not happen until you have left the selection. The fix here is to make sure we start the drag on the first mouse gesture.
OK, I have a patch for this coming up that really makes drag and drop more reponsive and fixes this bug to boot.
Attached patch Patch (obsolete) (deleted) — Splinter Review
This patch makes us create the drag (almost) no matter what, as long as we have not already started a drag. Before we were only doing that (inexplicably) if there was special content in the selection. The assumption, however, that the "node being dragged" is the node under the cursor, is still a wrong one and may even lead to wrong selections. We really need to get some node out of the selection itself in order to deal with this.
Attached patch Patch v1.0.1 (obsolete) (deleted) — Splinter Review
Oops, I didn't attach the one with if (BuildDragData). This is the real one.
Attachment #114963 - Attachment is obsolete: true
OS: Windows XP → All
Hardware: PC → All
Comment on attachment 114964 [details] [diff] [review] Patch v1.0.1 This patch is bad for several reasons. We just need to store whether the mouse was in the selection when we moused down. Post-1.3 we should also have this code grab what content the mouse was over when it was moused down as well (this is stored in ESM) but we'll need to be surgical for now.
Attachment #114964 - Attachment is obsolete: true
Attached patch Patch v2.0 (obsolete) (deleted) — Splinter Review
The problem is simply that the node the selection needs to look for is really the textnode. This is definitely less than perfect-looking. We have no really easy way of getting at that except to look at the frame and grab the content from it. IMO OriginalTarget should get set to this but that would require more restructuring than we should do at the moment.
Attachment #114992 - Flags: review?(bryner)
Comment on attachment 114992 [details] [diff] [review] Patch v2.0 Yeah, this should be ok. But you're right, there really should be a way to get at this information from the event.
Attachment #114992 - Flags: review?(bryner) → review+
Attachment #114992 - Flags: superreview?(kin)
Comment on attachment 114992 [details] [diff] [review] Patch v2.0 Looks ok to me. Do we need this same type of fix in the editor's nsHTMLDataTransfer::CanDrag() ccode? I think it does this same type of check to ssee if a node is the sselection.
Attachment #114992 - Flags: superreview?(kin) → superreview+
Attachment #114992 - Flags: approval1.3?
Comment on attachment 114992 [details] [diff] [review] Patch v2.0 a=asa (on behalf of drivers) for checkin to 1.3
Attachment #114992 - Flags: approval1.3? → approval1.3+
Yep, we need this fix in the editor too (it is also broken). I am going to consolidate this function into one place (nsIDOMNSEvent). I had a patch tonight to put this info in originalTarget, but if people grab originalTarget there is no way to distinguish between XBL anonymous content and a normal textnode retargeting. The patch attached to this bug has the same problem, but it shouldn't be a problem just for checking if the node is in the selection.
Attached patch Patch v3.0 (obsolete) (deleted) — Splinter Review
This version of the patch fixes the bug in a similar way, *and* moves this stuff over to nsIDOMNSEvent so that the other blockers can take advantage of it (I have a fix for the double-click in composer that depends on this). This fixes the drag in browser window problem. It also makes originalTarget always be set initially when the nsDOMEvent is created so that the mouse event target will be in it (this is the definition of originalTarget). This fixes the drag in composer problem.
Attachment #114992 - Attachment is obsolete: true
Attachment #115182 - Flags: review?(bryner)
Attached patch Patch v3.1 (obsolete) (deleted) — Splinter Review
This version of the patch doesn't touch originalTarget at all, which leads to a much lower chance of further regression. (i.e. this patch only really affects the codepaths we are directly trying to fix.)
Attachment #115182 - Attachment is obsolete: true
Attachment #115202 - Flags: review?(bryner)
Attachment #115182 - Flags: review?(bryner)
Comment on attachment 115202 [details] [diff] [review] Patch v3.1 - In nsDOMEvent::GetExplicitOriginalTarget() + if (mExplicitOriginalTarget) { + *aRealEventTarget = mExplicitOriginalTarget; + return NS_OK; Add an NS_ADDREF(*aRealEventTarget) here, or you'll crash and burn... :-) sr=jst
Attachment #115202 - Flags: superreview+
Attached patch Patch v3.1.1 (deleted) — Splinter Review
With addref ;)
Attachment #115202 - Attachment is obsolete: true
Attachment #115202 - Flags: review?(bryner)
Attachment #115209 - Flags: review?(bryner)
Attachment #115209 - Flags: superreview+
Comment on attachment 115209 [details] [diff] [review] Patch v3.1.1 Looks good.
Attachment #115209 - Flags: review?(bryner) → review+
Attachment #115209 - Flags: approval1.3?
Comment on attachment 115209 [details] [diff] [review] Patch v3.1.1 a=asa (on behalf of drivers) for checkin to 1.3
Attachment #115209 - Flags: approval1.3? → approval1.3+
This was checked in last night and should be in today's builds.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
If I'm not mistaken it was checked in after branching for 1.3. It needs to be checked in to the branch too and I can't find such a checkin.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Yes, it does. I can't check it in until the branch opens, however.
I can't reproduce with 2003022304-trunk/WinXP. Thank you! But, another problem has a dragging. See Bug 194459.
Comment on attachment 115209 [details] [diff] [review] Patch v3.1.1 I'm guessing dragging in Plaintext widgets such as textfields and textareas are still broken since nsPlaintextEditor::CanDrag() is still calling GetOriginalTarget(): http://lxr.mozilla.org/seamonkey/source/editor/libeditor/text/nsPlaintextDataTr ansfer.cpp#386 jkeiser, have you done checked all places that called GetOriginalTarget() via LXR to see there are other modules that need this fix?
Sigh. The GetOriginalTarget -> GetExplicitOriginalTarget() part of this fix is wrong. It will not grab XBL content. This means smilies and hrs will not be dragged currently. We need to either set originalTarget to the real original target (this caused problems for some odd reason, perhaps because of my implementation of it) or have another new method. I would rather give the former approach a try.
Note that I *am* going to check in this patch to both trunk and branch because it makes things better for most cases.
Patch checked in to trunk and 1.3 both. bug 194802 filed for the text drag in textarea issue.
Status: REOPENED → RESOLVED
Closed: 22 years ago22 years ago
Resolution: --- → FIXED
Whiteboard: fixed1.3
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: