Closed Bug 73605 Opened 23 years ago Closed 23 years ago

nsHTMLContentSerializer outputs <textarea> contents as attributes

Categories

(Core :: DOM: Editor, defect, P2)

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.2

People

(Reporter: kinmoz, Assigned: anthonyd)

References

Details

(Keywords: dataloss, Whiteboard: [html][textarea], patch attached, need r= and sr=)

Attachments

(3 files, 1 obsolete file)

new patch with kin's changes
1.38 KB, patch
Details | Diff | Splinter Review
Reverse of previous patch, since bug 17003 is fixed now
1.35 KB, patch
sicking
: review+
jst
: superreview+
Details | Diff | Splinter Review
Same as previous patch, but this one applies in today's trunk build
1.98 KB, patch
Details | Diff | Splinter Review 
I noticed that if I load the following content into Composer

<html>
<body>
<form>
<textarea>TextArea</textarea>
</form>
</body>
</html>


and switch to the ViewSource panel (or do a SaveAs), the resulting output from 
nsHTMLContentSerializer will look like this:


<html>
<body>
<form>
<textarea defaultvalue="TextArea" value="TextArea"></textarea>
</form>
</body>
</html>


This means that if you load the output into the browser or composer, the 
contents of the textarea will not be displayed and appears lost to the user!

Vidur please reassign if you no longer work on the content serializer module.
Summary: nsHTMLContentSerializer outputs <textarea> contents out as attributes → nsHTMLContentSerializer outputs <textarea> contents as attributes
Anthony owns the serializers now.
Assignee: vidur → anthonyd
moving to mozilla1.0
Priority: -- → P2
Target Milestone: --- → mozilla1.0
*** Bug 82543 has been marked as a duplicate of this bug. ***
Depends on: 17003
moving this back into 9.2, this really needs to be fixed
Severity: normal → major
Keywords: correctness
Whiteboard: [html]
Target Milestone: mozilla1.0 → mozilla0.9.2
Whiteboard: [html] → [html][textarea]
Keywords: dataloss
Attached patch patch for this...YUCK (obsolete) — — Splinter Review 

    
    

    
  
this bug sucks, and here is why:
there is no underlying dom support for the content between the open element and 
the close element.  so there isnt a way to correct this through the dom.  so 
what has to be done is special case the serializer to check for textarea, and 
then do not set value and default value.  then take value and shove it in 
between the open and close of the textarea.  all other attributes are written 
out correctly.  this means that if someone DOES set value or default value for a 
textarea, it will be stripped. c'est le vie.

anthonyd
Status: NEW → ASSIGNED
Whiteboard: [html][textarea] → [html][textarea], patch attached, need r= and sr=

    
    

    
  
Harish is working on a fix for the broken textarea handling in mozilla, once
that's done there shouldn't need to be any special code in the serializers...

    
    

    
  
Hi Anthony,
got a mid-air collision with jst, but here's what I was going to say:
I have found one HTML bug that would be concerned by this :
bug 17003. (which appears to be the bug harish is working on)
Two other bugs are connected, even though they concern more XUL.
However it is possible that they suffer from the same problem, since xul
probably uses the html textarea.
bug 53673
bug 42646
If this patch addresses none of the issues above please ignore this comment. But
I would be interested to get your feedback, whether they have anything to do
with this.
Thanks,
Fabian.

    
    

    
  
So when is harish's fix supposed to land? Before 0.9.3 closes? If not, do we 
want this to go in as a workaround?

In case this has to go in as a temporary workaround, here are some comments on 
the patch:


The code added in the following should be moved after the BR case so that the 
comment and the BR code can be kept together.


@@ -258,6 +258,11 @@
     // Filter out special case of <br type="_moz"> or <br _moz*>,
     // used by the editor.  Bug 16988.  Yuck.
     //
+    if ( (aTagName == nsHTMLAtoms::textarea) &&
+         ((attrName.get() == nsHTMLAtoms::value) || 
+         (attrName.get() == nsHTMLAtoms::defaultvalue)) ){
+        continue;
+    }
     if ((aTagName == nsHTMLAtoms::br) &&
         (attrName.get() == nsHTMLAtoms::type) &&
         (valueStr.EqualsWithConversion(kMozStr, PR_FALSE, sizeof(kMozStr)-1))) 
{


In this part of the diff, the code added should be put at the end of the method 
it's in, or at least after the LineBreakAfterOpen() call just in case people 
want breaks automatically inserted after the <textarea> tag.


@@ -342,6 +347,12 @@
 
   AppendToString(kGreaterThan, aStr);
 
+  if (name.get() == nsHTMLAtoms::textarea)
+  {
+    nsAutoString valueStr;
+    content->GetAttribute(kNameSpaceID_HTML, nsHTMLAtoms::value, valueStr);
+    AppendToString(valueStr, aStr);
+  }
   if (LineBreakAfterOpen(name, hasDirtyAttr)) {
     AppendToString(mLineBreak, aStr);
     mColPos = 0;


I'd also like to request that you add a comment that precedes the code you are 
adding (in both places) that says something to the effect:

    // XXX: This special cased textarea code should be
    //      removed when bug #17003 is fixed.

So that this workaround is easily flagged, and can be removed when the bug is 
fixed.

If you can convince jst and harishd that this should go in, you can have my 
r=kin@netscape.com with those changes.

    
    

    
  
With kin's suggested changes made, sr=jst

    
    

    
  
If you're serializing the document might it make more sense to use the
defaultValue property than the value property?  (I guess it depends how the
serializer is being used... if it's always used from an editor instantiated from
parsing the page then it would probably make sense as is, but if it's used in
other cases then the defaultValue property would make more sense and would be
what you would get once bug 17003 is fixed.)

But anyway,

a=dbaron for trunk checkin (on behalf of drivers)

    
    

    
  
I have made kins changes.  I will checkin.
r=kin
sr=jst
a=dbaron

anthonyd

    
    

    
  


  

    
    

    
  
its in.

anthonyd
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED

    
    

    
  
verified in 6/21 build.
Status: RESOLVED → VERIFIED

    
  

        Attachment #126854 -
        Flags: superreview?(jst)

        Attachment #126854 -
        Flags: review?(bugmail)

    
    

    
  
Comment on attachment 126854 [details] [diff] [review]
Reverse of previous patch, since bug 17003 is fixed now

sr=jst. Should this bug be reopened now?

        Attachment #126854 -
        Flags: superreview?(jst) → superreview+

    
    

    
  


  
This patch is the same as the previous patch, but this one applies again in
today's trunk build.

    
    

    
  
Martijn, have you tested that the behavior doesn't change with that patch?

    
    

    
  
Thanks, patch checked in now.

    
    

    
  
Oops, I didn't test this either, figured it was all good. I'll test once I have
a fresh seamonkey build with this change. I'll back out the backout if there are
problems :)

    
    

    
  
Ok, tested (using a 1.7 branch tree) and this change doesn't break textarea
serialzation.

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: