647682 - Cutting and pasting of links fails in composer edit pages

Status: RESOLVED FIXED

(SeaMonkey :: Composer, defect)

Description

[René PBA Meijer]

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) (not active in Mozilla any more)]

Sure, I can confirm this bug. Same thing happened with BlueGriffon.

Comment 2

[[e-mail]]

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

[[e-mail]]

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

[[e-mail]]

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) (not active in Mozilla any more)]

(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

[(no longer active)]

(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

[[e-mail]]

Attachment: Possible patch

Comment 8

[(no longer active)]

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 [:bzbarsky]]

Comment on attachment 526380 [details] [diff] [review]
Possible patch

Yeah, I guess this works.  God, we need to rationalize our docshell interfaces.

Comment 10

[[e-mail]]

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

[(no longer active)]

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

[[e-mail]]

Attachment: Proposed patch

With extra error checking as requested.

Hopefully r=ehsan f=bz suffices.

Comment 13

[(no longer active)]

Comment on attachment 528432 [details] [diff] [review]
Proposed patch

Yes, they do suffice.  :-)

Comment 14

[[e-mail]]

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

[(no longer active)]

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

[Jens Hatlak (:InvisibleSmiley)]

The fix for this propagated to current mozilla-beta, so it's fixed for 2.3. Don't know about earlier versions.