133735 - Copying from a list <li> always copies the position in the list

Status: VERIFIED FIXED

(SeaMonkey :: General, defect)

Description

[Valerio Capello]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.9+) Gecko/20020325
BuildID:    2002032503

Probably related to Bug 133728 but not a duplicate.

When you copy a part of an item in a list, its position in the list is copied in
the clipboard as well.

It applies to Ordered Lists (including with parameters) and Unordered lists.

Example:

<OL>
<LI> Item 1
<LI> Item 2
<LI> Item 3
</OL>

<UL>
<LI> Item 1
<LI> Item 2
<LI> Item 3
</UL>

<OL TYPE=a START=3>
<LI> Item 1
<LI> Item 2
<LI> Item 3
</OL>


Let's say you try to copy only "tem 2" from the second element in every list.

You should get
tem 2
in all cases

instead:

In the first list you'll get:
   2. tem 2

In the second list you'll get:
# tem 2
Note that if you select the whole line you get:
*  Item 3 
( * instead of # )

In the third list you'll get:
   4. tem 2
This is interesting because you don't get the letter (d), nor its position (2),
but the number related to letter d.

You can make further tests at this URL:
http://www.elfqrin.com/Sample.html#Lists


Reproducible: Always
Steps to Reproduce:
1. Go to any webpage with a list, or create your own
2. Select a part of an item in a list (not from the beginning of the line, or
however excluding the list marker)
3. Copy it in a text editor or somewhere else

Actual Results:  The position of the element in the list is pasted at the
beginning of the pasted text even if you didn't select/copy it.

Expected Results:  should paste only the selected/copied text.

This is could be a feature, not a bug, yet it should apply only when you
select/copy the whole element in a list, not just a part of it.

Comment 1

[Valerio Capello]

Confirmed on v.9.9+ build 2002032708 win98

Comment 2

[Akkana Peck]

Joe, they shouldn't be getting any list info in the sample case (when only part
of the list item is selected), should they?  I thought that only the text was
copied in that case.

Tanu has done some work recently regarding conversion of ordered lists to
plaintext; cc'ing, but I think there are two issues here: whether the <li> and
<ol> or <ul> should be getting copied to the context (Joe), and the issue about
ol order (Tanu).

Comment 3

[Joe Francis]

Akkana is correct that this behavior is a bug.  It is likely a document encoder 
bug.  I am the mamajama (doh!).

Comment 4

[Joe Francis]

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

Comment 5

[Joe Francis]

Ya, document encoder has been altered in a bad way.  Someone added "li" to 
IncludeInContext().  This is very bad, and casues this bug.  Tanu, did you do 
this?  I don't remember approving any such change to doc encoder, but maybe I 
missed it in a review?

This will need to be undone, so I'm wondering what previous bugfix is dependent 
on it.

Comment 6

[Joe Francis]

Attachment: undoing earlier patch of nsDocumentEncoder.cpp

cvsblame shows it was [e-mail], sr'd by jst.  That patch has to be
backed out.  patch to do that is here.

Comment 7

[Joe Francis]

Nominating for EDITORBASE and nsbeta1 since this is 
a) fixinhand
b) a backout of a recent patch, so there is no regression risk (other than bug
original patch was for: 62188, which is not nearly as serious as this bug).

Comment 8

[Akkana Peck]

It was from Tanu's bug 62188, and I reviewed an earlier version of the patch for
that bug which also had this problem.  Whoops, sorry!

I'm hoping that if we back out just this part, that the rest of the fix for bug
62188 will continue to work in the case where multiple list items are chosen
(i.e. if you select part of a list item, the list item and list don't go into
the copy context, but if you select a complete list item, they do).  Yes?  No? 
We should probably go ahead and back it out, but let's make sure we know whether
we're regressing bug 62188 or not.

Comment 9

[Christopher Aillon (sabbatical, not receiving bugmail)]

Well, It just looks like a typo -- nsHTMLAtoms::ul probably was meant to be used
instead of nsHTMLAtoms::li

Making that change (below) fixes this bug and does not regress bug 62188.

Index: base/src/nsDocumentEncoder.cpp
===================================================================
RCS file: /cvsroot/mozilla/content/base/src/nsDocumentEncoder.cpp,v
retrieving revision 1.66
diff -u -1 -r1.66 nsDocumentEncoder.cpp
--- base/src/nsDocumentEncoder.cpp      23 Mar 2002 23:54:44 -0000      1.66
+++ base/src/nsDocumentEncoder.cpp      29 Mar 2002 00:47:09 -0000
@@ -1307,3 +1307,3 @@
       tag.get() == nsHTMLAtoms::ol       ||
-      tag.get() == nsHTMLAtoms::li) {
+      tag.get() == nsHTMLAtoms::ul) {
     return PR_TRUE;

Comment 10

[Tanu Mutreja]

Attachment: Patch to fix this...

The bug is a regression caused my patch for bug#62188. This patch is to fix it
in the same way as discussed above. It carries few more changes to resolve the
inconsistency in indentation of elements in OL and UL. More specifically,
earlier if I copied "b" from <OL>abc</OL>, on pasting it to some Text Editor, I
was getting(with indentation)
      b     
but for <UL>abc</UL> I was getting(without indentation)
b
With the patch both are pasted as:
b

Instead of opening a new bug, here I'm including a fix for that too.

Comment 11

[Joe Francis]

Neither ol or ul (or li) can remain in IncludeInContext().  That will force even
one letter selections in the middle of a list item to emit a list into the
clipboard data.  And now that will be a list without a list item, with text
directly in the list: not even legal html.

Any patch that goes in for this has to have no list elements in
InludeInContext().  If that regresses 62188, that's ok - this bug is worse.  You
get the indention of "b" in Tanu's example above because of the text directly in
a list (with no list item) - that's how browsers have historically rendered that
illagal html, but it doen't mean we should generate it.

I need r's and sr's for the original patch I attached here.  

kin, can u sr this? 

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Tanu Mutreja]

>Neither ol or ul (or li) can remain in IncludeInContext().

Without these tags, my fix for 62188 has no meaning. 

>That will force even one letter selections in the middle of a list item to emit 
>a list into the clipboard data. And now that will be a list without a list 
>item, with text directly in the list: not even legal html.

This is true. So we need to decide if we want to avoid generating an illegal 
html as mentioned above or loose a feature that 62188 adds. 

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

>So we need to decide if we want to avoid generating an illegal 
>html as mentioned above or loose a feature that 62188 adds.

Tanu, that decision is easy.  Mozilla is all about standards compliance.  We
need to output valid HTML, much as I would hate to see 62188 backed out.

>>Neither ol or ul (or li) can remain in IncludeInContext().
>Without these tags, my fix for 62188 has no meaning.

jfrancis, I didn't investigate further when I posted comment 9, but I do see
now.  However we should probably be looking at a full backout of 62188 and not
just this one file, since Tanu says the other changes are void without the
nsDocumentEncoder changes.

I'd hate to see 62188 regress, but in order to fix this bug, it is the right
thing unless an alternate solution to 62188 is found quickly.

Comment 16

[Valerio Capello]

I couldn't imagine it would be so hard to implement the copy of list markers as
in Bug 62188

I've tried to examine the behaviour of Internet Explorer 6 and Opera 6 for
curiosity and in fact they both only copy the texts ignoring <li> tags.

Anyway I think there could be a workaround. For example, neither ol or ul (or
li) would be included in IncludeInContext() if only a single line has been
selected and there's at least another character before than the first character
selected, so that ol or ul would always be included as it happens now with an
exception.

Comment 17

[Joe Francis]

In response to Tanu's comment #14, this is not really a choice between just 
illegal html or a fix for 62188.  If we take Tanu's patch, you will get bad 
results whenever you copy from inside a list item and do a plaintext paste.  It 
would paste on a new line, indented.  

Plaintext serialization basically needs some extra info on the clipboard in order 
to solve 62188.  When we needed extra info for html paste inside composer, we 
added some extra data flavors on the clipboard to hold that info (see 
EncodeToStringWithContext()).  

Ironically, this is just the kind of problem that XIF would have been good at 
solving.

Comment 18

[Joe Francis]

akkana, can i get an r= here?  this is fairly serious bug with simple fix.  kin
or jst, can i get sr?

Comment 19

[Akkana Peck]

Comment on attachment 76729 [details] [diff] [review]
Patch to fix this...

Reluctant r=akkana because it is a regression.	Sigh.

We really need to find a solution to this problem, though!  The inability to
list any of these items in the copy context is causing a large number of other
bugs like the one Tanu was fixing -- we need to find a way to include that
information somehow.

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

Comment 21

[Akkana Peck]

Comment on attachment 76663 [details] [diff] [review]
undoing earlier patch of nsDocumentEncoder.cpp

This was actually the patch I was reviewing.

Comment 22

[Akkana Peck]

Comment on attachment 76729 [details] [diff] [review]
Patch to fix this...

Mistakenly marked this patch as reviewed ... un-marking (though I'd love to be
able to keep this one).

Comment 23

[kinmoz]

Comment on attachment 76663 [details] [diff] [review]
undoing earlier patch of nsDocumentEncoder.cpp

sr=[e-mail]

Please reopen bug 62188
 if you check this in.

Comment 24

[Jesse Ruderman]

See also bug 78676, "selection inside a single <li> shouldn't include bullet". 
The recent change makes bug 78676 trigger more often, but that doesn't mean it's
at fault.

Comment 25

[Tanu Mutreja]

Jfrancis, I think in nsDocumentEncoder, if we can do something like the parent 
tag is included only if the child node is completely selected, we would get the 
right thing. This is something that Akkana has always suggested for all the tags 
in general. It would resolve quite some table related bugs and many more. As I 
recall, it could take quite some time, which we all agree and we may not like to 
do that for 1.0. 

So can we make a compromise at this stage? I think we can, because even though 
we know that something like <ol>abc</ol> is incorrect yet we know that most of 
the browsers are able to resolve this and do the right thing with this and such 
situations are much lesser than the ones where we are doing copy/paste with the 
list items.

Comment 26

[Asa Dotzler [:asa]]

Comment on attachment 76663 [details] [diff] [review]
undoing earlier patch of nsDocumentEncoder.cpp

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 27

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) approval for checkin into 1.0.

Comment 28

[Joe Francis]

Tanu, the idea of having certain tags force certain ancestors into the copy
stream is probably a good one, but we can not do that for moz1.0.  In the
meatime, I can't leave any list structure in IncludeInContext().  Even if you
excluded li, and only did ol/ul, it would still break paste.

Comment 29

[Joe Francis]

Comment on attachment 76729 [details] [diff] [review]
Patch to fix this...

we cant do this

Comment 30

[Joe Francis]

fix landed on trunk

Comment 31

[R.K.Aa.]

Still pastes as # when copying from middle of a list: bug 137482

Comment 32

[Joe Francis]

This landed before branch cut.  Hence: fixed1.0.0

Comment 33

[rubydoo123]

using trunk build from 2003040708 on win2K, this works correctly. I created a
test case with the initial list descriptions, viewed the test case in the
browser, selected partial list item content and pasted into notepad. Only the
text was pasted, not the list item marker.

Comment 34

[Max Waterman]

I confirm that the list item marker is not copied (OS X 2003040708).

However, I do note that if you select a word manually, an extra space gets copied.

eg on the originator's web page, select 'Item' by placing the cursor to the
right of the 'm', click and hold the left mouse button, and moving left until
the cursor has just moved to the left of the 'I', such that the whole word
'Item' is selected, so as you can be sure that no 'space' before the 'I' has
been selected.

Copy and paste that into another window, and it will have a space before the 'I'.

I also notice that when you cut and paste the entire list, an extra space is
inserted between the '#' character and the 'I' of 'Item'.

Also wondering why the '#'s aren't numbers, since I was copying an ordered list
(from the first list on http://www.elfqrin.com/Sample.html#Lists)

Different bug(s) perhaps?

Max.