Closed Bug 17003 Opened 25 years ago Closed 23 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)
(deleted), text/html
Details
Testcase demonstrates: textarea has no child nodes
(deleted), text/html
Details
strip "\n" in the contentsink
(deleted), patch
jst
: superreview+
Details | Diff | Splinter Review
newline testcase
(deleted), text/html
Details
Patch w/o newline stripping
(deleted), patch
john
: review+
jst
: superreview+
Details | Diff | Splinter Review
testcase showing failure
(deleted), 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) (deleted) — — 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
Attached patch first attempt (obsolete) (deleted) — — Splinter Review
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) (deleted) — — 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) (deleted) — — Splinter Review
Attachment #61418 - Attachment is obsolete: true
Attached patch easy way out v3 (obsolete) (deleted) — — 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...
Attached patch strip "\n" in the contentsink (deleted) — — Splinter Review
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.
Attached patch Patch w/o newline stripping (deleted) — — Splinter Review
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 → ---
Attached file testcase showing failure (deleted) —
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 → 23 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: