Last Comment Bug 647682 - Cutting and pasting of links fails in composer edit pages
: Cutting and pasting of links fails in composer edit pages
Status: RESOLVED FIXED
: regression
Product: SeaMonkey
Classification: Client Software
Component: Composer (show other bugs)
: SeaMonkey 2.0 Branch
: All All
: -- normal (vote)
: seamonkey2.3
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
: 639147 639577 670554 (view as bug list)
Depends on:
Blocks: CVE-2010-2769
  Show dependency treegraph
 
Reported: 2011-04-04 07:45 PDT by René PBA Meijer
Modified: 2011-08-08 09:45 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed


Attachments
Possible patch (2.47 KB, patch)
2011-04-15 14:25 PDT, neil@parkwaycc.co.uk
ehsan: feedback+
bzbarsky: feedback+
Details | Diff | Review
Proposed patch (2.48 KB, patch)
2011-04-26 14:00 PDT, neil@parkwaycc.co.uk
ehsan: review+
neil: feedback+
Details | Diff | Review

Description René PBA Meijer 2011-04-04 07:45:17 PDT
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_7; nl-nl) AppleWebKit/533.20.25 (KHTML, like Gecko) Version/5.0.4 Safari/533.20.27
Build Identifier: 2.0.12 and 2.0.13

I have reported this to the SeaMonkey Council, but they couldn't reproduce it. Me and a friend of mine on another Mac OSX 10.6.7 mac mini snowleopard, her in the Netherlands  have the same problem with the Dutch version of the program. Cutting and pasting links results in the clipboard not keeping the link.  So when copying texts we have to redo all the links. Therefore we have reverted to version 2.0.11 which works fine on our machine and in our language. Can anybody reproduce and or fix this problem?

Reproducible: Always

Steps to Reproduce:
1.Mac OS 10.6.7 minimac (old version)
2.Open html page with composer
3.Trying to cut and paste a link within the same page and between pages, results in failure of clipboard to keep the link. 
Actual Results:  
no links in the text pasted

Expected Results:  
keep the link of the the text pasted

version 10.0.11 works fine, from then on we have this problem
Comment 1 Daniel Glazman (:glazou) 2011-04-04 08:14:07 PDT
Sure, I can confirm this bug. Same thing happened with BlueGriffon.
Comment 2 neil@parkwaycc.co.uk 2011-04-07 07:24:58 PDT
Thanks to Daniel I can reproduce this now. What I hadn't realised was that the target of the link needs to be a local file, or an anchor in a local file (including a relative anchor within the document itself...)

I'm guessing that this is another regression from bug 520189. It might be dependent on bug 606117.
Comment 3 neil@parkwaycc.co.uk 2011-04-07 07:26:11 PDT
As a workaround you can copy and paste the source of the link, either via the HTML source view (which is easier to copy from) or the Insert HTML dialog (which is easier to paste source with).
Comment 4 neil@parkwaycc.co.uk 2011-04-07 07:32:33 PDT
And the reason this fails is because pasting doesn't provide a document to InsertFromTransferable so that it's always considered to be unsafe. (What do you do when you want to paste a link copied from an external application?)
Comment 5 Daniel Glazman (:glazou) 2011-04-07 07:45:08 PDT
(In reply to comment #4)
> And the reason this fails is because pasting doesn't provide a document to
> InsertFromTransferable so that it's always considered to be unsafe. (What do
> you do when you want to paste a link copied from an external application?)

yeah... If that bit of code is understandable in the browser, it makes little
sense in the editor where js and plugins are disabled by default.
We should check the docshell to see if it's an editorshell and in that case
assume the link is always safe. What do you think?
Comment 6 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-14 14:06:24 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > And the reason this fails is because pasting doesn't provide a document to
> > InsertFromTransferable so that it's always considered to be unsafe. (What do
> > you do when you want to paste a link copied from an external application?)
> 
> yeah... If that bit of code is understandable in the browser, it makes little
> sense in the editor where js and plugins are disabled by default.
> We should check the docshell to see if it's an editorshell and in that case
> assume the link is always safe. What do you think?

Yeah, we already have a similar treatment for images <http://mxr.mozilla.org/comm-central/source/mozilla/content/base/src/nsContentUtils.cpp#2440>, so I guess I could live with that.  Are you going to write the patch, or should I?
Comment 7 neil@parkwaycc.co.uk 2011-04-15 14:25:26 PDT
Created attachment 526380 [details] [diff] [review]
Possible patch
Comment 8 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-15 15:37:23 PDT
Comment on attachment 526380 [details] [diff] [review]
Possible patch

This look good to me.  Having some comment there describing what the purpose is would make it even better.

You should probably ask Boris to review it.
Comment 9 Boris Zbarsky [:bz] 2011-04-21 21:45:54 PDT
Comment on attachment 526380 [details] [diff] [review]
Possible patch

Yeah, I guess this works.  God, we need to rationalize our docshell interfaces.
Comment 10 neil@parkwaycc.co.uk 2011-04-22 04:11:54 PDT
Comment on attachment 526380 [details] [diff] [review]
Possible patch

ehsan, I know you asked for extra comments; what about extra error-checking? Or indeed is there anything else you'd like me to do to the patch?
Comment 11 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-26 13:00:06 PDT
Comment on attachment 526380 [details] [diff] [review]
Possible patch

Review of attachment 526380 [details] [diff] [review]:

Since you asked me to be stricter...  ;-)

::: editor/libeditor/html/nsHTMLDataTransfer.cpp
@@ +1317,5 @@
+    nsCOMPtr<nsIDocShellTreeItem> root;
+    dsti->GetRootTreeItem(getter_AddRefs(root));
+    nsCOMPtr<nsIDocShell> docShell(do_QueryInterface(root));
+    PRUint32 appType;
+    docShell->GetAppType(&appType);

This will leave appType uninitialized if GetAppType fails, I think.
Comment 12 neil@parkwaycc.co.uk 2011-04-26 14:00:27 PDT
Created attachment 528432 [details] [diff] [review]
Proposed patch

With extra error checking as requested.

Hopefully r=ehsan f=bz suffices.
Comment 13 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-26 14:01:48 PDT
Comment on attachment 528432 [details] [diff] [review]
Proposed patch

Yes, they do suffice.  :-)
Comment 14 neil@parkwaycc.co.uk 2011-04-29 16:51:51 PDT
Pushed changeset 7893933cf5d0 to mozilla-central.

Actually I pushed it yesterday but I forgot to update the bug.

As it's a Gecko 2 regression, do we want this on any branches?
Comment 15 :Ehsan Akhgari (busy, don't ask for review please) 2011-05-01 13:59:00 PDT
(In reply to comment #14)
> Pushed changeset 7893933cf5d0 to mozilla-central.
> 
> Actually I pushed it yesterday but I forgot to update the bug.
> 
> As it's a Gecko 2 regression, do we want this on any branches?

I don't know if we're planning another dot-release on 2.0, but since this patch doesn't affect Firefox, I don't think anybody would mind.  Not sure if you need to go through with the approval2.0 flag or not though.
Comment 16 Philip Chee 2011-05-06 11:48:22 PDT
*** Bug 639147 has been marked as a duplicate of this bug. ***
Comment 17 Philip Chee 2011-06-19 11:28:33 PDT
*** Bug 639577 has been marked as a duplicate of this bug. ***
Comment 18 Ian Neal 2011-08-06 05:43:27 PDT
*** Bug 670554 has been marked as a duplicate of this bug. ***
Comment 19 Jens Hatlak (:InvisibleSmiley) 2011-08-08 09:45:11 PDT
The fix for this propagated to current mozilla-beta, so it's fixed for 2.3. Don't know about earlier versions.

Note You need to log in before you can comment on or make changes to this bug.