Cutting and pasting of links fails in composer edit pages

RESOLVED FIXED in seamonkey2.3

Status

SeaMonkey
Composer
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: René PBA Meijer, Assigned: neil@parkwaycc.co.uk)

Tracking

({regression})

SeaMonkey 2.0 Branch
seamonkey2.3
regression

SeaMonkey Tracking Flags

(seamonkey2.3 fixed, seamonkey2.4 fixed, seamonkey2.5 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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
Sure, I can confirm this bug. Same thing happened with BlueGriffon.
(Assignee)

Comment 2

6 years ago
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.
Blocks: 520189
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
OS: Mac OS X → All
Hardware: Other → All
Version: unspecified → SeaMonkey 2.0 Branch
(Assignee)

Comment 3

6 years ago
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).
(Assignee)

Comment 4

6 years ago
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?)
(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?
(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?
(Assignee)

Comment 7

6 years ago
Created attachment 526380 [details] [diff] [review]
Possible patch
Attachment #526380 - Flags: feedback?(ehsan)
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.
Attachment #526380 - Flags: feedback?(ehsan) → feedback+
(Assignee)

Updated

6 years ago
Attachment #526380 - Flags: feedback?(bzbarsky)
Comment on attachment 526380 [details] [diff] [review]
Possible patch

Yeah, I guess this works.  God, we need to rationalize our docshell interfaces.
Attachment #526380 - Flags: feedback?(bzbarsky) → feedback+
(Assignee)

Comment 10

6 years ago
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 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.
(Assignee)

Comment 12

6 years ago
Created attachment 528432 [details] [diff] [review]
Proposed patch

With extra error checking as requested.

Hopefully r=ehsan f=bz suffices.
Assignee: nobody → neil
Attachment #526380 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #528432 - Flags: review?(ehsan)
Attachment #528432 - Flags: feedback+
Comment on attachment 528432 [details] [diff] [review]
Proposed patch

Yes, they do suffice.  :-)
Attachment #528432 - Flags: review?(ehsan) → review+
(Assignee)

Comment 14

6 years ago
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?
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(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.

Updated

6 years ago
Duplicate of this bug: 639147

Updated

6 years ago
Duplicate of this bug: 639577

Updated

6 years ago
Duplicate of this bug: 670554
The fix for this propagated to current mozilla-beta, so it's fixed for 2.3. Don't know about earlier versions.
status-seamonkey2.3: --- → fixed
status-seamonkey2.4: --- → fixed
status-seamonkey2.5: --- → fixed
Target Milestone: --- → seamonkey2.3
You need to log in before you can comment on or make changes to this bug.