156203 - copy/paste doesn't always copy numbers/letters in <OL>

Status: RESOLVED FIXED

(Core :: DOM: Serializers, defect)

Description

[Jamie Zawinski]

For example:

    <OL>
      <B><LI> one</B>
      <LI> two
      <LI> three
    </OL>

What appears on the screen is

    1. one.
    2. two.
    3. three.

What you get when you select and paste that HTML is

    #  one.
    2. two.
    3. three.

Comment 1

[Boris Zbarsky [:bzbarsky]]

to DOM-to-text

Comment 2

[Tanu Mutreja]

CC'ing Akkana & Daniel, Assigning -> myself.

Comment 3

[Tanu Mutreja]

Attachment: PatchV1.0

Though given example is sheerly an invalid HTML yet this patch may make the
code more robust.

Comment 4

[Tanu Mutreja]

Comment on attachment 91333 [details] [diff] [review]
PatchV1.0

This patch caused some regression in nested lists. I'll try for some other
solution.

Comment 5

[Tanu Mutreja]

Attachment: Patch to fix this...

Comment 6

[Akkana Peck]

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

>+    if(mTagStack[i-1] == eHTMLTag_ol)
>+      return PR_TRUE;
>+    else if ((mTagStack[i-1] == eHTMLTag_ul) || (mTagStack[i-1] == eHTMLTag_dl)) {
>+      // If a Dl or UL is reached first, LI does not belong to OL.
>+      return PR_FALSE;

The "else" here isn't needed, but it's a stylistic thing -- leave it in if you
find the code easier to follow that way.

Either way, r=akkana.

For the sample html code, I get the output
   1.	one
   2. two
   3. three 

but the extra spaces before "one" were there before the patch, too, and I think
that's covered in another bug.

Comment 7

[Tanu Mutreja]

I think 'else' or a condition in while is required for cases where an "li" 
belongs to "ul" that is nested in "ol". However, I think "dl" should be removed 
from the else part i.e. following change should be fine:

+    if(mTagStack[i-1] == eHTMLTag_ol)
+      return PR_TRUE;
+    else if (mTagStack[i-1] == eHTMLTag_ul){
+      // If UL is reached first, LI does not belong to OL.
+      return PR_FALSE;
+    }
+    --i;
+  }
+

Comment 8

[Akkana Peck]

I was only calling the else unnecessary because you've returned if the "if" is
satisfied, so you'll only get to the next line if the "else" is satisfied.  Not
a big deal.

You're right about dl, though.  r=akkana on that modification.

Comment 9

[Tanu Mutreja]

Attachment: Modified patch with Akkana's suggestions.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 92135 [details] [diff] [review]
Modified patch with Akkana's suggestions.

- In nsPlainTextSerializer::IsInOL():

+  while(i > 0) {
+    if(mTagStack[i-1] == eHTMLTag_ol)
+      return PR_TRUE;
+    if (mTagStack[i-1] == eHTMLTag_ul) {
+      // If a UL is reached first, LI belongs the UL nested in OL.
+      return PR_FALSE;
+    }
+    --i;
+  }

How about decrementing i in the while statement to avoid duplicating doing that
in the looop? I.e.:

+  while(--i >= 0) {
+    if(mTagStack[i] == eHTMLTag_ol)
+      return PR_TRUE;
+    if (mTagStack[i] == eHTMLTag_ul) {
+      // If a UL is reached first, LI belongs the UL nested in OL.
+      return PR_FALSE;
+    }
+  }

sr=jst with that change.

Comment 11

[Akkana Peck]

Comment on attachment 92135 [details] [diff] [review]
Modified patch with Akkana's suggestions.

Still r=akkana (with or without jst's suggestion).

Comment 12

[Tanu Mutreja]

Attachment: With JST's suggestions.

Comment 13

[Asa Dotzler [:asa]]

Comment on attachment 92912 [details] [diff] [review]
With JST's suggestions.

a=asa (on behalf of drivers) for checkin to 1.1

Comment 14

[Tanu Mutreja]

Checked-in to trunk:
09/15/2002 6:14, nsPlainTextSerializer.h, 1.25                          
09/15/2002 06:14 nsPlainTextSerializer.cpp 1.64 

Can be verified only after bug# 167893 is closed.