228688 - Unable to paste html to beginning of node

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Kathleen :Brade]

With recent builds of mozilla, I see a problem pasting html.
Steps to reproduce:
  Create new composer window
  Type:  this is a test
  Double-click "this"
  Copy
  Paste
Notice that the selection disappears but no text is pasted.
Press right-arrow and then Paste (succeeds).

Copying text from another app and pasting into html composer is fine.
Copy/pasting within text fields in the browser is fine.
Mozilla 1.4 / Netscape 7.1 works fine.
I had thought it was a bug in my patch for bug 32768 but a download build from
today also has the same problem.  :-(

I can only test on Mac; hopefully someone else can check windows and/or linux.

Comment 1

[Kathleen :Brade]

mkaply saw this on a recent OS2 build
I've narrowed it down to between June 18 and July 20 3am.
I'm guessing it's a regression from some of the editor cleanup I was doing at
that point but I'm not sure what phase of cleanup caused the regression.

Debugging is very painful on this Mac, I managed to track down the insertion
failure to this vicinity:
 
http://lxr.mozilla.org/seamonkey/source/editor/libeditor/html/nsHTMLEditRules.cpp#4241

The left node is null so an error is returned and insertion doesn't continue in
InsertHTMLWithContext:
 
http://lxr.mozilla.org/seamonkey/source/editor/libeditor/html/nsHTMLDataTransfer.cpp#442

Joe, Daniel, Kin:  any particular guidance to help me track down this regression?

Comment 2

[Andrew Schultz]

regression between linux trunk builds 2003070107 and 2003070205

Comment 3

[Mike Kaply [:mkaply]]

The only thing that jumps out at me between these dates is some deCOMtamination
by roc...

Comment 4

[Kathleen :Brade]

cc roc since he may have introduced this regression

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

All my changes related to frame manipulation. Does this editor code actually
care about frames at all?

Comment 6

[Joe Francis]

Kathy, I think that coment went in at some point where we knew that getting
there meant the original selection wasn't collapsed.  If you know the original
selection had something in it, and you split at the start, and then split again
at the end, you know the "end" split has a non-null left node.  That left node
represents all (or a portion) of the contents that were originally selected.

However looking at the routine as it stands now, I see no reason why selection
couldn't be collapsed.  I bet we removed some code that was handling the
collapsed case and returning early.  

I would cvs blame my comment, and then pull the file from when I first checked
in that comment.  Then compare the routine then to the routine now and see what
we changed for handling collapsed selections.  Hope this helps...

Comment 7

[Joe Francis]

For the most part the editor doesn't care about frames.  But the editor cares
dearly about selection, and selection may be affected by changes to frames.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is a pretty bad bug. I think the way to go here is to start pulling by date
to figure out exactly which patch caused the regression.

Comment 9

[Andrew Schultz]

*** Bug 226897 has been marked as a duplicate of this bug. ***

Comment 10

[Andrew Schultz]

backing out the patch from bug 176709 fixed the problem for me.

Comment 11

[Andrew Schultz]

this bug doesn't bite in an unoptimized Linux build.
When pasting the first time, valgrind sees this:

Conditional jump or move depends on uninitialised value(s)
 CNavDTD::WillBuildModel() (CNavDTD.cpp:425)
 nsParser::WillBuildModel() (nsParser.cpp:1227)
 nsParser::ResumeParse() (nsParser.cpp:1732)
 nsParser::Parse() (nsParser.cpp:1644)
 nsHTMLEditor::ParseFragment() (nsHTMLDataTransfer.cpp:2269)
 nsHTMLEditor::CreateDOMFragmentFromPaste() (nsHTMLDataTransfer.cpp:2173)
 nsHTMLEditor::InsertHTMLWithContext() (nsHTMLDataTransfer.cpp:265)
 nsHTMLEditor::InsertFromTransferable() (nsHTMLDataTransfer.cpp:1043)

Conditional jump or move depends on uninitialised value(s)
 CNavDTD::WillBuildModel() (CNavDTD.cpp:430)
 nsParser::WillBuildModel() (nsParser.cpp:1227)
 nsParser::ResumeParse() (nsParser.cpp:1732)
 nsParser::Parse() (nsParser.cpp:1644)
 nsHTMLEditor::ParseFragment() (nsHTMLDataTransfer.cpp:2269)
 nsHTMLEditor::CreateDOMFragmentFromPaste() (nsHTMLDataTransfer.cpp:2173)
 nsHTMLEditor::InsertHTMLWithContext() (nsHTMLDataTransfer.cpp:265)
 nsHTMLEditor::InsertFromTransferable() (nsHTMLDataTransfer.cpp:1043)

Comment 12

[Andrew Schultz]

> Conditional jump or move depends on uninitialised value(s)
>  CNavDTD::WillBuildModel() (CNavDTD.cpp:425)
>  nsParser::WillBuildModel() (nsParser.cpp:1227)
>  nsParser::ResumeParse() (nsParser.cpp:1732)

I thought I had seen this before... I reported the same thing in bug 204283 in
May.  The testcase there (attachment 122367 [details]) is probably doing something similar
to what happens here.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Just to be sure, I rolled back to 7/2/2003 3:40am and then backing out my
checkin does not fix the bug. So Andrew's on the right track...

Comment 14

[chris hofmann]

what is the next step here?  it would be good to figure this one out for 1.6...

Comment 15

[Kathleen :Brade]

roc (comment 13)--when you backed it out, did you see the bug or did it work
fine?  I'm trying to understand if the tree was broken at the point of your
checkin or still working correctly...

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The tree was broken both before and after my checkin on 7/2/2003.

Comment 17

[chris hofmann]

looks like a fix for this is not going to make 1.6.  renominate if something
appears and lets try to figure out for 1.7

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

bryner points out that nsHTMLFragmentContentSink::IsEnabled doesn't fill in aReturn.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

comment 11 and comment 18 sounds like a nice thing to fix, but perhaps not the
cause of the bug...

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I tested that the UMR is unrelated, and filed bug 230536 on it.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

argh. this bug is really embarrassing.

Comment 22

[Brian Rogers]

Yes, this bug is very bad, and if 1.6 is released without this fixed, it should
go in the release notes.  This can make paste look totally broken.  I would also
advocate making a 1.6.1 release to fix this when a fix does become available.

I'm trying debug this problem myself, too.

Comment 23

[Mark Shackelford]

Quick note from user-land: Is this really a bug? It looks like normal copy/paste
behaviour to me. From "Steps to reproduce:"

  Create new composer window
  Type:  this is a test    <-- Add some text to the window
  Double-click "this"     <-- Selects and highlights "this "
  Copy       <-- Copies "this " to clipboard, leaves "this " selected
  Paste  <-- Pastes "this " over "this ", removes selection, cursor before "is"

You don't see the text change because you're pasting the same text over it.

Try this instead:

  Create new composer window
  Type:  this is a test    <-- Add some text to the window
  Double-click "this"     <-- Selects and highlights "this "
  Copy            <-- Copies "this " to clipboard, leaves "this " selected
  Ctrl-Shift-RightArrow  <-- Extends selection over "is "
  Paste      <-- Pastes "this " over "this is ", removes selection

You should now have "this a test", cursor right before "a".

Or try this:

  Create new composer window
  Type:  this is a test    <-- Add some text to the window
  Double-click "this"     <-- Selects and highlights "this "
  Copy            <-- Copies "this " to clipboard, leaves "this " selected
  LeftArrow  <-- Removes selection, cursor is at beginning of line
  Paste      <-- Pastes "this " at cursor location, removes selection

You should now have "this this is a test", cursor at beginning of second "this".

Hope that helps.

Comment 24

[Kathleen :Brade]

from comment 23:
>Quick note from user-land: Is this really a bug? It looks like normal
>copy/paste behaviour to me. From "Steps to reproduce:"
>
>  Create new composer window
>  Type:  this is a test    <-- Add some text to the window
>  Double-click "this"     <-- Selects and highlights "this "
>  Copy       <-- Copies "this " to clipboard, leaves "this " selected
>  Paste  <-- Pastes "this " over "this ", removes selection, cursor before "is"
>
>You don't see the text change because you're pasting the same text over it.

This is not what I intended to describe; it's not what I see.
Here are the steps again:
  Edit www.mozilla.org/editor
  double-click "Mozilla"
  copy
  paste
What I see is "Mozilla" is gone and the line only has " Editor" on it.

At the moment, I'm not running my debug build so I can't test it with a new
document.  I'm going to be unavailable for about 10 days.  Perhaps after that I
can do more testing.  I am suspicious that the bug *may* be specific to certain
doctypes but I haven't done any specific testing in that area to confirm or
disprove that.  Hopefully someone else can look into it in the meantime.
  

Comment 25

[Brian Rogers]

I noticed that this only happens with html clipboard content.  If just plain
text is in the clipboard (copied from another application or a plain-text field
within Mozilla such as the URL bar), pasting works fine at the beginning of a node.

Comment 26

[Sam Katz]

This is a production machine, but it might be useful to see whether "Paste 
Special" as Plain Text works as expected.

This would provide a workaround.

all the evidence seems to point, as andrew said, to this bug:
http://bugzilla.mozilla.org/show_bug.cgi?id=176709

This bug does affect HTML, but does not affect plain text.

Comment 27

[Arend Freije]

Just a hunch: reading all this, it seams to me that the HTML clipboard has got a
'functional lazy' implementation. The Paste operation prunes the selected word
before inserting, and thus the clipboard gets pruned as well. So nothing gets
inserted...

Comment 28

[chris hofmann]

anyone come up with what they thing a good patch might look like?

Comment 29

[Kathleen :Brade]

Backing out (#if'ing actually) the code for bug 176709 in my somewhat hacked
tree does indeed impact this bug.  #if 0'd allows me to paste, #if 1 does not.

seeking reconsideration for 1.7a

Comment 30

[Kathleen :Brade]

Attachment: proposed fix for this bug

Comment 31

[Kathleen :Brade]

By the way, the first parts of the diffs are not really related to this bug; I
made them a long time ago.  I leave it to reviewers and approvers whether to
actually land the minor optimizations or not.

Comment 32

[Kathleen :Brade]

Attachment: editor patch (alternate to parser backout)

This patch is an alternative to the backout.  I think this approach is more
correct than the backout but I would like jfrancis or kin to review / ok it. 
(I'll remove the #if and related code if one of them is ok with removing the
#if'd out code.)

Comment 33

[Kathleen :Brade]

Comment on attachment 141361 [details] [diff] [review]
editor patch (alternate to parser backout)

seeking feedback on this tiny patch

Comment 34

[chris hofmann]

joe/kin,  can you look at it soon..  getting down to the wire on 1.7a

Comment 35

[Johnny Stenback (:jst)]

So I've been playing around with this a bit today and I've yet to be able to
reproduce this. I've followed the initial description in this bug, and I've
followed what brade explained in comment #24, and I've tried other select+paste
on the first text of a node etc, but I'm always able to paste. What am I missing?

I'm on WinXP, using my own debug build from late last week.

Comment 36

[Brian Rogers]

It doesn't take anything that complicated.  Just open Composer with a blank
page, and type "test".  Then copy the text, go to the beginning of the line, and
paste.

Comment 37

[Johnny Stenback (:jst)]

Tried that, and it works here. What build are you using?

Comment 38

[Brian Rogers]

I saw this on Mozilla 1.6 and I still see the problem with the latest build today.
All on FreeBSD.  I haven't tested this on Windows.

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

This fixes the underlying problem and the reason things are different on
different platforms.  If you want to call strcmp on a string, that string needs
to be null-terminated.	Windows is probably doing something that makes it
null-terminated.

Comment 40

[Johnny Stenback (:jst)]

Comment on attachment 141649 [details] [diff] [review]
patch

Nice, sr=jst.

/me feels silly for forgetting that in the first place.

Comment 41

[Johnny Stenback (:jst)]

Comment on attachment 141649 [details] [diff] [review]
patch

Nice, sr=jst.

/me feels silly for forgetting that in the first place.

Comment 42

[chris hofmann]

Comment on attachment 141649 [details] [diff] [review]
patch

a=chofmann for 1.7a

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fixed.

Comment 44

[Brian Ryner (not reading)]

Attachment: one more place to fix this

Comment 45

[Kathleen :Brade]

Comment on attachment 141361 [details] [diff] [review]
editor patch (alternate to parser backout)

this should not be needed now