15204 - hidden inputs strip line feeds from value attribute

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect, P3)

Description

[Chris McAfee]

Using apprunner to change the tinderbox message of the day
blurb inserts weird HTML'isms that mess up the message.
Working on a test case.

Comment 1

[Akkana Peck]

Accepting bug and will work on along with all the other output bugs for M12.
This is a case where the user types html source into a plaintext widget, submits
it through a form as plaintext (expecting it to look like what he typed), then
the cgi on the other end of the form displays it as html.

We do need a test case; the easiest way might be a javascript page that does
document.write of whatever it reads in from the text area.

Comment 2

[Chris McAfee]

Attachment: simple text area test for HTML stuff

Comment 3

[Chris McAfee]

Compare the text area contents on 4.5 & apprunner.
Document.write is echoing content below the textarea,
this isn't working on apprunner, but this is just extra stuff,
text area content is enough to show the bug.

Comment 4

[Akkana Peck]

Changing Component to Output.

Comment 5

[Akkana Peck]

This is a dogfood candidate since it blocks mcafee's ability to use mozilla for
being sherriff.

Comment 6

[Akkana Peck]

Originally, the problem was the improper handling of entities, particularly <
and >.  But this has been fixed, and the plaintext output of the text in the
text field is correct (modulo whitespace) now.  I'm guessing now that this is
due to other problems with html form controls.

Comment 7

[Chris McAfee]

Agreed, I need this for sheriff, that's dogfood for me
and the other sheriffs.

Comment 8

[Akkana Peck]

On second thought, this may not be a form submission bug; this may be a parser
bug.

Try running
viewer "http://bugzilla.mozilla.org/showattachment.cgi?attach_id=1935"
and do a Dump Content.  Look at what it thinks the content of the text field is,
and compare that to what 4.6's View Source gives you.  They're quite different.

Cc'ing Harish and Rick; maybe they can shed some light on this.

Comment 9

[leger]

Putting on PDT+ radar.

Comment 10

[harishd]

The wierdism in the textarea ( such as extra semicolons ) does look like a
parser problem.  Investigating the problem.  Assigning bug to myself and CCing
pollmann.

Comment 11

[Eric Pollmann]

In addition to the extra semi's, fetching the textarea's value on
nsGfxTextControlFrame is returning an empty string.  This is why nothing is
document.written.  I'll take a look...

Comment 12

[Eric Pollmann]

Got a fix for the content part of this bug - a one line change to
nsGfxTextControlFrame::GetTextControlState

Index: nsGfxTextControlFrame.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/html/forms/src/nsGfxTextControlFrame.cpp,v
retrieving revision 3.87
diff -r3.87 nsGfxTextControlFrame.cpp
814c814
<       nsFormControlHelper::GetInputElementValue(mContent, &aValue, PR_TRUE);
---
>       GetText(&aValue, PR_TRUE);

I'll check this in for M12.

Comment 13

[harishd]

Might take a day or two ( atmost ) to fix this bug.

Comment 14

[Eric Pollmann]

Checked in the fix I mentioned above.

Comment 15

[rickg]

Eric -- giving this back to you, per a discussion with harishd. Please enter a
date into this bug when you think this will be fixed.

Comment 16

[Eric Pollmann]

This currently works for me on Linux.  Testing on NT as soon as my build
finishes.

Estimate to have this working: tonight.

Comment 17

[Eric Pollmann]

Harish, is the parser portion of this fix checked in?

Comment 18

[Eric Pollmann]

My NT build finished - the fix I checked in earlier also works on NT.

Comment 19

[racham]

works fine. verified with dec 14th's commercial build.

Comment 20

[Chris McAfee]

Reopening, this is happening again.

Comment 21

[Chris McAfee]

This broke before 3/3, still looking.

Comment 22

[Chris McAfee]

verified with akkana that test case shows that yeah, this isn't working right.

Comment 23

[Chris McAfee]

beta1

Comment 24

[Akkana Peck]

Also a regression.

Comment 25

[Eric Pollmann]

Has not been reviewed by PDT+ team yet this time around, removing PDT+.  Also,
can you please provide a simple test case.  There were two issues fixed with
this bug report, which one is broken again?

Comment 26

[Akkana Peck]

Just click on the attachment to see a test case.  Then do it in 4.x and see the
difference.

Comment 27

[leger]

Putting on PDT+ radar for beta1.  But please fix by 03/10.

Comment 28

[Chris McAfee]

2/23 build test case fails on linux, so it regressed before then.?

Comment 29

[Eric Pollmann]

Reduced test case:

<HTML>
 <BODY>
  <TEXTAREA NAME="txt"> 
   <B>Go home</B>                               
  </TEXTAREA> 
 </BODY>
</HTML>

B doesn't show up in textarea.  CC'ing buster and rods.

Comment 30

[Eric Pollmann]

Passing off to Harish, looks like he has a good start on this.

Comment 31

[harishd]

Have a fix..need to do some testing though. 

Comment 32

[Chris McAfee]

can you attach a patch for testing?

Comment 33

[harishd]

Attachment: Patch...

Comment 34

[harishd]

 Html Tokens used to contain the name of the tag.
 But for the sake of performance we now rely on the tagID
 rather than tag name.  This caused the bug to reincarnate. 
 
 FIXED this problem by including the tag name when recording trailing content ( 
used only for TEXTAREA).

Comment 35

[harishd]

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

Comment 36

[gerardok]

Verified on 2000-03-13-18-M15-nb1b build on Win95 and Linux, and 
2000-03-13-17-M15-nb1b build on MacOS.

Comment 37

[Chris McAfee]

it's baaaaaaaaack.
Just wiped out tinderbox status using mozilla to admin
tinderbox page.  Clearing status, starting over.  again.

Comment 38

[Chris McAfee]

note: this was on trunk, with linux.
I may have mistyped tinderbox password, sending me to
the page to reask password.  Content lost all carriage returns,
and was truncated, about 1/2 of the HTML disappeared.

I resubmitted a skeleton html replacement and that seemed to work.
Any ideas?  Something's amiss in cache/submit land.

Comment 39

[leger]

Putting on [dogfood+]FOR THE TRUNK

Comment 40

[Eric Pollmann]

See bug 38057.  I have a fix for the truncation part of this bug, but not for
the loss of carriage returns.  The fix is to tbglobals.pl, which is part of the
tinderbox cgi - it is escaping quotes wrongly, and also messes up in Nav or IE.

Comment 41

[Eric Pollmann]

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

Comment 42

[ckritzer (gone)]

Attachment: reduced testcase courtesy of pollman

Comment 43

[ckritzer (gone)]

Tested on:
MacOS9 2000-08-02-04-M17 Commercial build
LinRH6 2000-08-02-04-M17 Commercial build
Win98  2000-08-02-05-M17 Commercial build

And the bug exists for all three platforms.

Comment 44

[ckritzer (gone)]

Additional info:

The original bug was that html source was getting it's CR's stripped and 
replaced by garbage/misc. characters.  Today, the CR's aren't stripped out, but 
closing angle brackets ( '>' ) are being replaced by semicolon's.  Chris, if I 
left anything out, please update/correct/etc.

Comment 45

[Eric Pollmann]

The truncation problem with tinderbox's message has been fixed.  Steve was kind
enough to both check in the patch and update mozilla.org's copy of tinderbox -
thanks Steve! :)

Comment 46

[leger]

Where was the patch checked in.  Trunk?  Branch? Trunk and Branch?

Comment 47

[Chris McAfee]

leger: patch to tinderbox server, not part of build.

This bug still open for carriage return lossage (and other
scrambling problems..)

Comment 48

[Kevin McCluskey (gone)]

Should this be downgraded from dogfood+?

Comment 49

[Chris McAfee]

I agree, and vote for dogfood-

Comment 50

[Eric Pollmann]

ETA (if still +) 08-Aug-2000

Comment 51

[Eric Pollmann]

The linefeeds are being stripped out of the value attribute of the hidden input  
in CAttributeToken::Consume at nsHTMLTokens.cpp:1387

if(!aRetainWhitespace)
  mTextValue.StripChars("\r\n"); //per the HTML spec, ignore linefeeds...

aRetainWhitespace will only be true when we are viewing the source of the 
document, so I don't know the solution to this one.

Harish, is there a way to specify that we want to preserve linefeeds for a case 
like this?

Comment 52

[Eric Pollmann]

Revising fix estimate - I'll be away 10-Aug and 11-Aug.

Comment 53

[Eric Pollmann]

Attachment: test case: hidden inputs loose linefeeds in value attribute

Comment 54

[Hixie (not reading bugmail)]

Line feeds in hidden inputs *should* be stripped (and converted to a single space)
just like the comment says.

INVALID? If we change our behaviour, then it can only be changed for quirks mode.

Comment 55

[Eric Pollmann]

See bug 47078, which is directly related to the same line of code.  By stripping 
out the CR and LF characters we are also violating the spec and breaking some 
additional class of sites:

--------(quoting bug 47078:)
This is what HTML4 spec says about the contents of CDATA attributes such as
value, alt, title, href, longdesc...:

# CDATA is a sequence of characters from the document character set and
# may include character entities. User agents should interpret attribute
# values as follows:
#  o Replace character entities with characters,
#  o Ignore line feeds,
#  o Replace each carriage return or tab with a single space.
# User agents may ignore leading and trailing white space in CDATA attribute
# values (e.g., "   myval   " may be interpreted as "myval").
  -- http://www.w3.org/TR/REC-html40/types.html#h-6.2  
--------(end quote)

This suggest some code more like:

if(!aRetainWhitespace) {
  if (<we have a CDATA attribute>) {
    mTextValue.StripChars("\n"); //per the HTML spec, ignore linefeeds...
    mTextValue.ReplaceChar("\t"," "); // replace tabs with spaces and...
    mTextValue.ReplaceChar("\r"," "); // replace carriage returns with spaces...
  } else {
    mTextValue.StripChars("\r\n"); //per the HTML spec, ignore linefeeds...
  }
}

I tested (a hack of) this patch and it actually kinda makes things worse in 
terms of all reported bugs.  For example, now the same page served off of a UNIX 
server may not preserve a line break (\n stripped) but on an NT server may (\r\n 
=> ' ').  

It seems like what Nav and IE are doing is preserving all whitespace inside the 
attribute (but not leading or trailing whitespace).  Is the spec out of line 
with industry practice here?  Should we do what Nav and IE do?  Is that more 
like this:

if(!aRetainWhitespace) {
  if (! <we have a CDATA attribute>) {
    mTextValue.StripChars("\r\n"); //per the HTML spec, ignore linefeeds...
  }
}
???

Note that there is already an exception in the spec as to how <style> and 
<script> content is handled - it should be passed through without stripping line 
breaks, so maybe this isn't to radical of a suggestion...

Comment 56

[Eric Pollmann]

Ian, yes, this should only for quirks mode.  That would probably include all of 
the sites affected by this bug.

Comment 57

[Eric Pollmann]

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

Comment 58

[Eric Pollmann]

15204 was dogfood+ and is virtually the same problem in the same area.

Comment 59

[Hixie (not reading bugmail)]

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

Comment 60

[Hixie (not reading bugmail)]

Marking this bug 'mostfreq' so that it appears on my radar. Most of the DUPs
have actually been marked INVALID rather than dups of this bug, so it is not
immediately obvious that this is actually a frequently reported bug. But if
you don't believe me, I'm happy to forward you my bugmail...! ;-)

Comment 61

[John Keiser (jkeiser)]

Yes, this is a rather important bug, as I noted when I reported it elsewhere.  
There will be many sites that require this, as it is a common case to pass 
hidden inputs with carriage returns in them.  CGI.pm, in fact, has this 
behavior.  I will talk to them too.

Comment 62

[Hixie (not reading bugmail)]

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

Comment 63

[John Keiser (jkeiser)]

A URL for those who are interested in testing (I don't have permissions to set 
that field): http://home.earthlink.net/~johnkeiser/mozbug/hidden-field.html

Comment 64

[John Keiser (jkeiser)]

Wanted to let people know that CGI.pm has fixed this problem on the server side 
in v2.71, which should come out sometime soon.  When it does, you can get it at 
http://stein.cshl.org/WWW/software/CGI/.

I figured this was relevant because a lot of people use that lib.

Comment 65

[Kevin McCluskey (gone)]

Talking to Eric it sounds like the remaining problem is a parser issue.
Reassign to harishd

Clearing the dogfood+ since it no longer truncates the message. It only looses 
carriage returns now.
Also clearing nsbeta3+. 
Clearing [Aug 15] date from status as well.

Comment 66

[leger]

Putting on [dogfood-]...we let eng mgr set nsbeta + or -

Comment 67

[harishd]

Since it's not a dogfood anymore and the risk in fixing, even though the fix 
would be simple, this bug would be great I'm inclined to mark this bug FUTURE.

If it's an error please renominate it for beta3. Thanx.

Comment 68

[John Keiser (jkeiser)]

harishd: this is an important bug, affecting many CGI scripts which will start 
failing in a hard-to-track way on Netscape 6.0.  CGI.pm, the main CGI lib used 
by developers up through 2.70 (2.71 is not even out yet) does not escape the 
values, and even those using the lib often don't use its facility to print 
hidden fields, instead printing them directly.  The many bugs that have 
appeared on this subject are most likely from many of these developers who are 
testing or developing their apps using Mozilla.

One big reason why it affects so much is a common technique among CGI 
developers doing "Wizard"-like interfaces.  They pass their values around the 
app in hidden fields until the user hits "Finish" or something similar.  many 
such programs have text areas in them.

This bug will be especially prevalent on intranets, where the Web is used for a 
lot of data entry.

I think this needs to get into Netscape before final, given that it affects so 
much.

Comment 69

[harishd]

Marking beta3+.

Comment 70

[leger]

Putting on [dogfood-] radar.

Comment 71

[Nisheeth Ranjan]

Harish has the fix in hand and will try to check it in by tomorrow.

Comment 72

[harishd]

Not stripping newlines/carriage return anymore. This bug should be fxd.

Marking FIXED.

Comment 73

[John Keiser (jkeiser)]

Yep, it works for me.

Comment 74

[Vladimir Ermakov]

Looks like the bug has returned. When there is a link in the body of the 
text area like:
     <A href="http://www.yahoo.com">Yahoo.com</A>
The text area displays:
     <A href="http://www.yahoo.com">href="http://www.yahoo.com"Yahoo.com</A>

Comment 75

[John Keiser (jkeiser)]

Could you elaborate a bit?  Do you mean if you put something like

<TEXTAREA COLS=40>
<A HREF="http://www.yahoo.com">Yahoo</A>
</TEXTAREA>

then it shows up wrong in the textarea?  When I do that it works fine for me 
(see http://home.earthlink.net/~johnkeiser/mozbug/textarea-href.html) ...

Also not sure how it relates to this bug.  The problem does not appear to have 
to do with newlines at all.

Comment 76

[Chris McAfee]

this is a dup, rickg please check this fix in, this is
dogfood for people running mozilla.org with .. mozilla.

Comment 77

[Chris McAfee]

rickg checked in, I think this is a dup of bug 49278,
someone please verify & mark it so.

Comment 78

[harishd]

If 49278 is fixed then this should be fixed. Giving bug to rickg to verify if
this bug is fixed.

Comment 79

[rickg]

This does appear to be a dup. The both pollmann's attachment and the inline
testcase: <A href="http://www.yahoo.com">Yahoo.com</A> are working now.

Comment 80

[Vladimir Ermakov]

Alright its fixed now. Verifying on
 -Windows 98 build 2000-09-26-08-M18
 -Linux RedHat6.2 build 2000-09-27-08-M18

Comment 81

[Blake Kaplan (:mrbkap) (inactive)]

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