Closed Bug 17003 Opened 25 years ago Closed 22 years ago

Textarea fails to expose child textnode

Categories

(Core :: DOM: Core & HTML, defect, P3)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.8

People

(Reporter: mitchgould, Assigned: sicking)

References

Details

(Keywords: dom1, testcase, Whiteboard: [TESTCASE][DIGBug])

Attachments

(6 files, 6 obsolete files)

Simplified test for childNodes of a textarea (failure to have any)
317 bytes, text/html
Details
Testcase demonstrates: textarea has no child nodes
237 bytes, text/html
Details
strip "\n" in the contentsink
15.36 KB, patch
jst
: superreview+
Details | Diff | Splinter Review
newline testcase
650 bytes, text/html
Details
Patch w/o newline stripping
14.79 KB, patch
john
: review+
jst
: superreview+
Details | Diff | Splinter Review
testcase showing failure
407 bytes, text/html
Details 
I believe this is M9. Build ID: 1999082412. MSIE5 handles the following test
correctly but Moze "can't find the properties" of the TextArea's text:

<HTML>
<HEAD>
<TITLE>Reggie3</TITLE>
<script>
function textareanodevalue() {
  alert(document.getElementById('tx1').childNodes[0].nodeValue)
 }
</script>
</HEAD>
<BODY>
<FORM name="form1" id="form1">
<TEXTAREA id="tx1" NAME="TextArea" ROWS="7" COLS="35">You can paste text
here.</TEXTAREA></TD>
<button onclick="javascript: textareanodevalue();">OK</button></FORM>
</BODY>
</HTML>
Assignee: don → vidur
Component: Browser-General → DOM Level 1
Well, the test included in the report gives the same error on Win95 with build
ID 1999101808, but..

Now I don't know all that much about DOM, but from my reading of the DOM 1 spec
I can't see what you would expect the textarea's childNodes to be, and the error
seems reasonably accurate. Changing ".childNodes[0].nodeValue" to just ".value"
gets the textarea text just fine.

Anyways, since this is clearly a DOM issue, changing component and reassinging,
I'm sure vidur can educate me on this..
Status: NEW → ASSIGNED
The problem is that we're not creating a text node as the child of a textarea
element. This should be done by the sink.
QA Contact: leger → gerardok
Resetting QA contact from leger.
In an attempt to get my bug list in order again, marking all the bugs I have
currently as ASSIGNED.
Whiteboard: [TESTCASE]
Already has a testcase. Marking as such.
Bulk moving [testcase] code to new testcase keyword. Sorry for the spam!
Keywords: testcase
*** Bug 22818 has been marked as a duplicate of this bug. ***
Keywords: beta1
is there a top 100 site with this problem?
Whiteboard: [TESTCASE] → [PDT-][TESTCASE]
Not even close to being a beta 1 problem. Moving out to M18.
Target Milestone: M18
This bug has been marked "future" because the original netscape engineer
working on this is over-burdened. If you feel this is an error, that you or
another known resource will be working on this bug,or if it blocks your work
in some way -- please attach your concern to the bug for reconsideration.
OS: Windows 98 → All
Hardware: PC → All
Target Milestone: M18 → Future
Mass update of qa contact
QA Contact: gerardok → janc
Keywords: dom1
Component: DOM Level 1 → DOM Core
QA contact Update
QA Contact: janc → desale
-Changing description
-Adding minimal testcase
Summary: DOM Fails to get nodeValue from TextArea → DOM fails to expose child textnode
mitchgould@mindspring.com, please note that your method of accessing the
textarea text through the DOM1 Core interfaces will only give you the text in
the source (X)HTML file, not the current value.

The current value has to be accessed through HTML DOM interfaces
(HTMLTextAreaElement's "value" method.). The bug is valid though, since you
don't get the source text value either.
Summary: DOM fails to expose child textnode → Textarea fails to expose child textnode
HarishD--could you help out with this bug?
(was assigned to Vidur)

Composer is mangling textareas (bug #82503) so we need to clean up textareas 
(including this bug).

other related bugs:
  http://bugzilla.mozilla.org/show_bug.cgi?id=50418
  http://bugzilla.mozilla.org/show_bug.cgi?id=58089
Assignee: vidur → harishd
Blocks: 82503
Status: ASSIGNED → NEW
Target Milestone: Future → ---
fix dependency bug #; should be bug #82543
Blocks: 82543
No longer blocks: 82503
Blocks: 73605
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.2
Attached patch Patch v1.1 [ content sink fix ] (obsolete) — — Splinter Review 

    
    

    
  
I have attached the first half of the patch. Giving bug to Ericto fix the the
second half :)
Assignee: harishd → pollmann
Status: ASSIGNED → NEW

    
    

    
  
Harish, why do we need HTMLContentSink::ProcessTEXTAREATag()?



    
    

    
  
ProcessTEXTAREATag() would allow textarea to behave as a container, in the sink, 
even though the parser treats textarea as a leaf. IMO, making this change at the 
parser level could be error prone. 

    
    

    
  
I try to come up with a parser/sink fix.

    
    

    
  
It looks like changing the DTD would be simpler after all ( though I haven't 
done enough testing! ).

    
    

    
  
Ok, Harish, it's your call if you wanto make the change in the parser or not
this late in the game, we can always leave this new method in the sink and file
a new post mozilla1.0 bug on removing the method from the sink and fixing the
parser. Either way works for me.

    
    

    
  
Updating QA contact to Shivakiran Tummala.
QA Contact: desale → stummala
Target Milestone: mozilla0.9.2 → mozilla0.9.3

    
    

    
  
When this is fixed will you also remove the value and defaultvalue attributes
from the content model (since I don't think they should magically appear in the
content model -- they should just be DOM properties).

    
    

    
  
Missed 0.9.3.
Target Milestone: mozilla0.9.3 → mozilla0.9.4

    
    

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

    
    

    
  
Adding DIGBug to status per kieffer@netscape.com
Whiteboard: [PDT-][TESTCASE] → [PDT-][TESTCASE][DIGBug]

    
    

    
  
eric is no longer around.  
we need some more time to figure out who can own this bug.
anybody have ideas?

unlikley this will make 0.9.4

    
    

    
  
eric is no longer around.  
we need some more time to figure out who can own this bug.
anybody have ideas?

unlikley this will make 0.9.4
Target Milestone: mozilla0.9.4 → mozilla0.9.5

    
    

    
  
Taking.
Assignee: pollmann → jst
Whiteboard: [PDT-][TESTCASE][DIGBug] → [TESTCASE][DIGBug]

    
    

    
  
*** Bug 98176 has been marked as a duplicate of this bug. ***
Target Milestone: mozilla0.9.5 → mozilla0.9.6

    
  
Blocks: 104166

    
    

    
  
Pushing to mozilla0.9.7
Target Milestone: mozilla0.9.6 → mozilla0.9.7

    
    

    
  
I have this mostly working in my tree, some string(?) problem left

    
    

    
  
Ok, this first attempt seems to work. However i'm *really* unsure about the 
htmlparser changes, so it would be great if somebody more parsercompatible then 
me could have a look at it.

The patch gives textareas a childnode containing the text, and makes 
myTextArea.defaultValue use that child, both for get and set.

The changes to nsHTMLTextAreaElement makes textareas behave like <input 
type="text"> wrt interaction between .value and .defaultValue, so it might need 
to be changed depending on the outcome of bug 108198

    
    

    
  
Comment on attachment 56471 [details] [diff] [review]
first attempt

arg! seems like a broke parsing of textarea content when it contains special chars like '<'

        Attachment #56471 -
        Attachment is obsolete: true

    
    

    
  
I tried to make <textarea> behave just like <plaintext> in the htmlparser which 
seems to work fine. Still would be nice if somebody whos parser foo is bigger 
then mine could have a look at it.

    
    

    
  
sorry everyone, the loop that deletes all childnodes of the textarea in 
SetDefaultValue is broken. I have it fixed in my tree

    
    

    
  
Comment on attachment 56481 [details] [diff] [review]
This one seems to work better

Talked to harish, and the parserchanges are not quite right.

        Attachment #56481 -
        Flags: needs-work+

    
    

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

    
    

    
  
Jonas, any more progress on this one?

    
    

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

    
    

    
  
actually, i'm kind'a holding this one off for harish. He said that he had some parser-changes that should make the parser-changes for this one smoother.

Harish: or should i just try to get it right without your changes?

    
    

    
  
Jonas: Are you sure that your changes don't break anything? 

Attaching a part of our email discussion
-----------------------------------------

Jonas Sicking wrote:
I would guess that the parser is scanning text until if finds something
similar to a "</textarea>". So whitespace and "<!--" are treated just like
any other character.

Harish wrote:
That is not completely true!  Currently, textarea content is not consumed as
text. It is so because textarea content is PCDATA.
The content within the textarea would get tokenized similar to any other
content. In the tokenizer we have something called RecordTrailingContnet() which
records whitespace and stuff for elements such as textarea and we  make use of
that information in the NavDTD to generate text content.

So, may be your change should not include

@@ -2418,7 +2418,7 @@
       // start token's GetSource();
       if(eToken_attribute!=theTokenType) {
         if ((eToken_entity==theTokenType) &&
-           ((eHTMLTag_textarea==theNodeTag) || (eHTMLTag_title==theNodeTag))) {
+            (eHTMLTag_title==theNodeTag)) {
             mScratch.Truncate();
About a couple of months ago I did some work on PCDATA parsing but couldn't
finish it due to other priorities. Will see if I still have those changes...if I
do then it should work perfectly with your changes and will also eliminate the
need for recording trailing content.

---- EOM --

Jonas: I'm not sure if I still have the PCDATA changes that I worked on a while
ago. I'll look for it. 


    
    

    
  
I don't see why this couldn't be fixed w/o touching the parser. We might still
need a hack in the sink that gets the skipped content and turns that into a text
node that's added as a child of the textarea, but that can be done w/o touching
the parser.

    
    

    
  
Harish: yeah, that's the stuff i ment.. please let me know if you can't find it 
and i'll try to do without it

Jst: the reason that i wanna change the parser is simply that it seems like the 
RightThing to do. It seems unneccesary to have the parser not forward the 
textnode in the <textarea> to the contentsink just to have the contentsink add 
the node back

    
    

    
  
Sicking, true, but given our resource constraints it's faster for us to live
with what we have in the parser and fix this bug. Once that's done we can open a
new bug on the parser to do the right thing, the sink changes at that point
should be trivial.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        the easy way out (obsolete)
        — 
          — Splinter Review
      

    


  
ok, so i took the easy way out. I simply failed to edit the htmlparser to
output what i want... oh well...

Anyway, this one works and should have all the bells and whistles that <input
type="text"> has.

        Attachment #36442 -
        Attachment is obsolete: true

        Attachment #56481 -
        Attachment is obsolete: true

    
    

    
  
oh, i forgot to mention: the patch also contains a small perf-improvement from 
jst (the |CBufDescriptor bd| stuff)
Assignee: jst → sicking

    
    

    
  

      
      
      
      

        Attached patch
          
          
        easy way out v2 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #61418 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        easy way out v3 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #61425 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 61426 [details] [diff] [review]
easy way out v3

sr=jst

        Attachment #61426 -
        Flags: superreview+

    
    

    
  
jkeiser should review this. Will you, please? Thanks in advance.

    
    

    
  
Comment on attachment 61426 [details] [diff] [review]
easy way out v3

Two things:

1. Please remove the XXX comments where people lament that textarea is not a
container :)

2. Do you not need the "line break normalization" that used to be done in
SetDefaultValue()?

With that answered (I suspect it will be answered without having to change the
patch), it looks great, r=jkeiser, yippee!

    
    

    
  
(i never ment this for 0.9.7, hope not anybody mind that i push it)

1. I still think we should have some XXX comment, since IMHO we really shouldn't
   need to have that extrahandling of the textnode. But I agree the current
   comment is wrong, i'll change it to something like "if the parser treated the
   text in a textarea like a normal textnode we wouldn't need to do this", sounds
   good?

2. I do that in GetDefaultValue instead. That way |.defaultValue=myString| is the
   same as |.appendChild(doc.createTextNode(myString)|. However I
   realized that I need to add back the stripped "\n" somewhere. Otherwise doing
   |.defaultValue = .defaultValue| might actually change the default value. Not
   sure where I should do it though...
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.7 → mozilla0.9.8

    
    

    
  
I think I would like to add the "\n" in SetDefaultValue since I think that is
the only way inStr would be == outStr in

myTextarea.defaultValue = inStr;
myTextarea.form.reset();
outStr = myTextarea.value;

The downside of that is that then the htmlparser would need to mask away the
leading "\n" before calling SetDefaultValue :(

ideas welcome

    
    

    
  
Hmm.  Hate to say it, but I think this is a job for parser (whatever part of
parser grabs the text inside TEXTAREA and puts it in there).  The real problem
lies with people expecting the linefeed just after <TEXTAREA> to not show up
when they code a web page that way.  textarea itself should not screw with that
because people who insert linefeeds at the beginning in JS obviously mean
it--which rules out .defaultValue and .value.  AppendChild/InsertChildAt() isn't
an appropriate place either, since you could use JS to create a new text node
with leading LF's and then insert it, and there too you obviously mean it to
have a leading LF.

At the very least parser should be the place where the leading CRLF/LF is lopped
off.  JS .value might be an OK place for CRLF conversion--we can just make that
part of the contract of textarea, that we never output CRLF's (apparently it
already is part of the contract).

But perhaps That's Another Bug :)  If it comes down to making a nasty decision
for the sake of expedience, I'd say put go ahead and put leading LF stuff into
SetDefaultValue(), put an XXX and open bug.

    
    

    
  
Actually, people putting a LF right after the starttag should be able to assume
that it's cut off, see bug 2750. But I agree that it should be the job of the
parser to strip the leading LF, so I'll make a patch that does that.

However if the parser removes that leading "\n" then the serializer should add
it back, not sure if that is really a biggie though...

    
    

    
  


  
stripped the "\n" in the contentsink. Note that this will not strip the leading
newline in xhtml, which IMHO is a good thing.

        Attachment #61426 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 61685 [details] [diff] [review]
strip "\n" in the contentsink

- In nsHTMLContentSink.cpp:

>+    if (aSkippedContent && (!aSkippedContent->IsEmpty())) {
>+      // Strip only one leading newline if there is one (bug 40394)
>+      nsString::const_iterator start, end;
>+      aSkippedContent->BeginReading(start);
>+      aSkippedContent->EndReading(end);

nsString::const_iterator should be nsAString::const_iterator. You don't know
here that aSkippedContent always will be a nsString since the API lets anyone
pass in any nsAString implementation.

Other than that, sr=jst.

        Attachment #61685 -
        Flags: superreview+

    
    

    
  
Comment on attachment 61685 [details] [diff] [review]
strip "\n" in the contentsink

Hmm, are you sure CR/LF stripping should be in GetDefaultValue()?  Does that
cover all possible cases (like if .value is changed by editor or by JS)?  When
we submit we do .value.

Everything else looks good.

    
    

    
  
you mean "normalization" and not "strip", right? (since no more stripping is 
done in the textarea now). The only problem i can see is that if somebody does

myTextarea.defaultValue = "foo\r\nbar";
myTextarea.form.reset();
outStr = myTextarea.value;

this should make outStr = "foo\nbar". However I don't see any way around that, 
but suggestions are always welcome...

    
    

    
  
actually, from some aspects i kind of like that we normalize linebreaks. People
shouldn't store binary data in a textarea anyway...

    
    

    
  
I agree with you 100%.  In fact, IMO we should do it on SetValue() as well for
consistency.

What I was concerned about was that you were normalizing in GetDefaultValue()
instead of SetDefaultValue().  I was concerned that it wouldn't get copied to
.value.  But now I remember why it works--because of our trickery in GetValue()
/ GetDefaultValue().

Change it to SetDefaultValue() (or argue me out of it) and you have r=jkeiser :)
 Optionally you might want to make SetValue() do the same thing--but while I
prefer that, it's nothing that should hold up the patch.

    
    

    
  
Oh yeah, my reasoning for doing it in SetDefaultValue(): readability.  If *I*
took a while to realize why / how it worked, then damn sure most other people
won't :)

    
    

    
  
I would be ok with normalizing in SetValue too. Jst, oppinions?

However if we normalize in SetDefaultValue we won't cache people doing
|myTextarea.appendChild(doc.createTextnode("foo\r\nbar"));|. Which is what the
xhtml parser does (and the html parser should do)

    
    

    
  
Hmmm.  I see the rationale then.  I still feel safer with doing it on Set()
since that's really what the code means to do ... it would have to be put into
AppendChildTo() and InsertChildAt() (since GetDefaultValue() calls them).

But GetDefaultValue() makes a strange sort of sense as well... do what you think
best then, but if you go with GetDefaultValue(), just put a comment in
GetDefaultValue() explaining how it works with the trickery (.value and the text
control element will be set using GetDefaultValue() up until the first time it
is set).  I'll r= with that.

In any case, SetValue() still makes sense IMO, but again, no need to hold up
patch for it if others disagree.

    
    

    
  
testcase is modified one from bug 16813. Pressing the button *should* show
'%0A%0A' as linebreaker for all textareas. It works fine on windows, but i want
to make sure all platforms work before checking in.

jkeiser, could you try to remove the newline-normlization lines and see if the
above testcase works for you. 

    
    

    
  


  
Yep, the testcase shows the same thing for me before/after this patch.	All
four lines show up the same.

    
    

    
  
oh, sorry, i didn't mean that you should create a new patch without the 
normilization, just test to remove those lines in your local tree.

But thanks for the patch!! :-)

Ok, since this seems to work fine i don't think there are any further 
changes/comments needed? So how about r and sr on attachment 61898 [details] [diff] [review]?

    
    

    
  
Comment on attachment 61898 [details] [diff] [review]
Patch w/o newline stripping

r=jkeiser

        Attachment #61898 -
        Flags: review+

    
    

    
  
Comment on attachment 61898 [details] [diff] [review]
Patch w/o newline stripping

sr=jst

        Attachment #61898 -
        Flags: superreview+

    
    

    
  
checked in, thanks for reviews!
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED

    
    

    
  
Reopening bug, Always returns the defaultValue.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
the childnodes are supposed to always be the same as .defaultValue

the spec says on .defaultValue:
Represents the contents of the element

and on .value:
Changing this attribute changes the contents of the form control, but does not 
change the contents of the element
Status: REOPENED → RESOLVED
Closed: 23 years ago → 22 years ago
Resolution: --- → FIXED

    
    

    
  
verified. The .defaultValue is returned properly. Agree with Jonas comments
Status: RESOLVED → VERIFIED

    
  
Component: DOM: Core → DOM: Core & HTML
QA Contact: stummala → general

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: