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

RESOLVED FIXED

Status

()

Core
Serializers
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: Jamie Zawinski, Assigned: Tanu Mutreja)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

16 years ago
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.
to DOM-to-text
Assignee: attinasi → harishd
Component: Layout → DOM to Text Conversion
QA Contact: petersen → sujay
(Assignee)

Comment 2

16 years ago
CC'ing Akkana & Daniel, Assigning -> myself.
Assignee: harishd → t_mutreja
(Assignee)

Comment 3

16 years ago
Created attachment 91333 [details] [diff] [review]
PatchV1.0

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

Comment 4

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

This patch caused some regression in nested lists. I'll try for some other
solution.
Attachment #91333 - Attachment is obsolete: true
(Assignee)

Comment 5

16 years ago
Created attachment 91476 [details] [diff] [review]
Patch to fix this...

Comment 6

16 years ago
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.
Attachment #91476 - Flags: review+
(Assignee)

Comment 7

16 years ago
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

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

Comment 9

16 years ago
Created attachment 92135 [details] [diff] [review]
Modified patch with Akkana's suggestions.
Attachment #91476 - Attachment is obsolete: true
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.
Attachment #92135 - Flags: superreview+

Updated

16 years ago
Attachment #92135 - Flags: review+

Comment 11

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

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

Comment 12

16 years ago
Created attachment 92912 [details] [diff] [review]
With JST's suggestions.
Attachment #92135 - Attachment is obsolete: true

Comment 13

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

a=asa (on behalf of drivers) for checkin to 1.1
Attachment #92912 - Flags: approval+
(Assignee)

Comment 14

16 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.