Closed Bug 114997 Opened 23 years ago Closed 19 years ago

Leading and trailing carriage returns being removed from INPUT TYPE="hidden" elements

Categories

(Core :: DOM: HTML Parser, defect)

Product:
Core
Component:
DOM: HTML Parser
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: gyoung, Assigned: steuard+moz)

References

(
URL
)

Details

(Keywords: fixed1.8.1, testcase)

Attachments

(5 files, 15 obsolete files)

Screen capture of input type=button from testcase after above patch.
(deleted), image/jpeg
Details
More complete tests (with better explanations of expected behavior)
(deleted), text/html
Details
Merged to tip
(deleted), patch
Details | Diff | Splinter Review
Tweak
(deleted), patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
1.8.1 branch patch for this+326250+330469
(deleted), patch
bzbarsky
: approval-branch-1.8.1+
Details | Diff | Splinter Review
The example shows that when leading or traling carriage returns are included in a hidden field, they are removed when the field is sent to the server. This is a major problem when doing comparisons of data that is included in a hidden field (an operation that I need to perform in a web app that identified the bug). Viewing the same page under IE6.0 produces the desired result. Unfortunately I don't have the capability to test this on another OS. Sorry. ASP code for the test page is listed below: <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN"> <html> <head> <title>Mozilla Bug Test - Hidden Form Element</title> </head> <body> <% if isEmpty(request.form("submit")) then response.write("<form action="""" method=""post"">" & vbNewLine) response.write("<input type=""hidden"" name=""Test"" value=""" & vbNewLine & "This is some data" & vbNewLine & "to test this bug" & vbNewLine & """>" & vbNewLine) response.write("<input type=""submit"" name=""submit"" value=""Test"">" & vbNewLine) response.write("</form>" & vbNewLine) else response.write("<pre>'" & request.form("Test") & "'</pre>") end if %> </body> </html>
Confirmed with 2001-12-12-03 under W2K. Both IE5 and Opera 5 work as the reporter suggests. In fact, in my build, a View Source on the resulting page gives the previous page's source, not the result page. Ouch!
Status: UNCONFIRMED → NEW
Ever confirmed: true
kin, could you take a look?
Assignee: rods → kin
Target Milestone: --- → mozilla1.0.1
--> jkeiser
Assignee: kin → jkeiser
Target Milestone: mozilla1.0.1 → ---
*This* is a dup of one of Harish's bugs.
Whiteboard: [DUPEME]
*** Bug 125217 has been marked as a duplicate of this bug. ***
I posted bug 125217 which is a dup of this bug. Here is my original report which contains another example page: From Bugzilla Helper: User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.7) Gecko/20020203 BuildID: 0.9.7 A form <input> field with type set to "hidden" should be able to contain linefeed characters in the value. These appear to work in the middle of the value, but if they are present at the end of the value, then they are removed from the end of the field. I have tried several variations, including turning the literal CR & LF characters into HTML character entities &#13; and &#10; but they are still truncated when they appear at the end. Reproducible: Always Steps to Reproduce: 1. Visit the demonstration URL (http://annexia.org/tmp/linefeed-tag.html) 2. Click the Submit Query button. 3. Examine the URL in the location bar. Actual Results: The URL ends with %3A (a colon character). Expected Results: The URL should be followed by %0D%0A%0D%0A. It is probably helpful to examine the source of the page to see what's going on. -------- Response from Ben Tremblay: NS4.79 produces: http://annexia.org/tmp/linefeed-tag.html?arg=Two+line+feeds+follow+me%3A%0A%0ATw o+line+feeds+follow+me+too%3A%0A%0A 2002020803 Win95 produces: http://annexia.org/tmp/linefeed-tag.html?arg=Two+line+feeds+follow+me%3A%0D%0A%0 D%0ATwo+line+feeds+follow+me+too%3A truncated
*** Bug 152991 has been marked as a duplicate of this bug. ***
Attached file Testcase (obsolete) (deleted) —
This testcase shows that JavaScript is able to set leading and trailing returns, even with setAttribute.
This needs to be fixed in parser, I think. Probably related to bug 92728.
Assignee: jkeiser → harishd
Component: Layout: Form Controls → Parser
Depends on: 92728
QA Contact: madhur → moied
Keywords: testcase
Summary: Leading and traling carriage returns being removed from INPUT TYPE="hidden" elements → Leading and trailing carriage returns being removed from INPUT TYPE="hidden" elements
Severity: normal → major
Priority: -- → P2
QA Contact: moied → dsirnapalli
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.5alpha
*** Bug 208546 has been marked as a duplicate of this bug. ***
--> 1.5b
Target Milestone: mozilla1.5alpha → mozilla1.5beta
Target Milestone: mozilla1.5beta → ---
The problem is that in general the beginning and trailing newlines DO need to be trimmed off. See the code in HTMLContentSink::AddAttributes that does that. I suppose we could just not trim them for the "value" attribute of HTML <input> elements... or something. :(
Yeah, that's probably the way to go :(
Blocks: 322270
(In reply to comment #12) > The problem is that in general the beginning and trailing newlines DO need to > be trimmed off. See the code in HTMLContentSink::AddAttributes that does that. > I suppose we could just not trim them for the "value" attribute of HTML > <input> elements... or something. :( As discussed in bug 322270, it does seem like "value" attributes need to be treated differently than others: we want to keep them absolutely verbatim, with no whitespace processing at all. This apparently violates the HTML specification (though seemingly only "should"s, not "must"s: http://www.w3.org/TR/html4/types.html#h-6.2), but that seems better than the alternative. Can this be implemented just by adding a conditional in HTMLContentSink::AddAttributes (http://lxr.mozilla.org/seamonkey/source/content/html/document/src/nsHTMLContentSink.cpp#813) as follows? // Take value attributes verbatim, without trimming whitespace if (keyAtom != nsHTMLAtoms::value) { const nsAString& v = nsContentUtils::TrimCharsInSet(kWhitespace, aNode.GetValueAt(i)); } else { const nsAString& v = aNode.GetValueAt(i); } (If that seems sensible, I'd be happy to post an actual patch, but someone else would need to check it in anyway.)
yes, please do post a patch. But we probably only want to do that when the value attribute is set on an <input> element.
Attached patch Don't trim whitespace for <input> values (obsolete) (deleted) — — Splinter Review
This patch simply checks whether the current attribute is an <input value="...">. If not, it strips leading and trailing whitespace as usual; otherwise it skips that step. (Say, should the list of whitespace characters here include an ordinary space? That would be an independent bug...) I've tested it on the 1.8 branch, and the attached testcase now works (there are linefeeds immediately next to the ":"s when you click "Peek"; look at the source). I don't know what else to test to make sure nothing else breaks (suggestions for things to test would be welcome, actual independent tests even more so), and I've only tested it on the Mac (again, other testers would be good). I'd like to think that the patch is simple enough that not too much testing will be necessary. I've used _?_:_ syntax rather than an if...else because the value is being assigned to a const reference. I'm open to suggestions on how to do this with if...else, which is generally easier to read; I'm just very out of practice in C programming these days. Also, I have no idea who I should be asking for review of this code (this is a perennial challenge for me). Among the many candidates are harishd (bug assignee), Blake Kaplan (HTML Parser module owner), and Peter Van der Beken or Johnny Stenback (DOM module owners, who control the file being patched). So instead, I'm just listing relevant peers who have actually commented on this bug (Jonas Sicking and Boris Zbarsky). As stated before, if this gets approved I'll need someone else to check it in. Suggestions on whether to ask for branch approval at that point are welcome.
Attachment #207549 - Flags: superreview?(bzbarsky)
Attachment #207549 - Flags: review?(bugmail)
OS: Windows 2000 → All
Hardware: PC → All
I don't see a good way to do this with if/else; unfortunately, using ?: with strings has led to GCC producing bogus code in the past... that might no longer be an issue with the last string rewrite, but I'm not sure. Might be OK if both strings involved are of the same concrete type... is that the case here?
I would advise against this syntax too. Don't remember which platform, but i've had problems with that syntax myself in the past. At the very least this needs to be compiled and tried on mac, linux and windows
(In reply to comment #17) > I don't see a good way to do this with if/else; unfortunately, using ?: with > strings has led to GCC producing bogus code in the past... that might no longer > be an issue with the last string rewrite, but I'm not sure. Might be OK if > both strings involved are of the same concrete type... is that the case here? I had a nasty feeling that the ?: would cause problems. As shown below, the types _don't_ match exactly, though I have relatively little understanding of the myriad string types in the code. For what it's worth, I compiled this using GCC on the Mac and it seemed to work. I don't have the ability to compile on other platforms, so I'll have to leave those tests to others. The types in the assignment are: v (left hand side): const nsAString& v nsContentUtils::TrimCharsInSet(): const nsDependentSubstring nsContentUtils::TrimCharsInSet() aNode.GetValueAt(): const nsAString& GetValueAt() If a different approach is necessary, I'm open to suggestions. Would it work to drop the const and just declare "nsAString v;" before the if/else (and then assign "nsAString& v = ..." differently inside)? Offhand, I don't even recall if that's valid syntax. (Can one just write "&v = ..."?) Unless the right approach is pretty straightforward, I'm afraid I may have contributed as much to this bug as I'm able to.
Try going on irc to see if someone's willing to test windows for you. If all three major platforms are ok then i'm fine with checking this in and see if it works.
maybe you could use a pointer rather than a reference?
Attached patch Correct error in previous patch's conditional (obsolete) (deleted) — — Splinter Review
Yikes! When I tried to limit the fix to input elements only, I forgot to move the "not" outside the "and". Thus, the previous patch blocked whitespace trimming on any attribute of elements other than inputs (in addition to the desired case). I've fixed that now (and I'll get over the embarrassment eventually).
Attachment #207549 - Attachment is obsolete: true
Attachment #207549 - Flags: superreview?(bzbarsky)
Attachment #207549 - Flags: review?(bugmail)
Even if it did seem to work, you've got me nervous about using ?: with string classes, so I'd rather avoid it. This patch takes a different approach: it sets a pointer to the desired string as an intermediate step, and then sets the constant reference v to that value. (I would feel uncomfortable passing the pointer directly; let me know if that's unwarranted.) Because no memory is ever allocated for this pointer and it will never get passed outside the function (I've put strong warnings in the comments), I'm not too worried about memory leaks. But I'm not an expert, so if there's still reason for concern, I want to know it.
Attachment #207619 - Flags: superreview?(bzbarsky)
Attachment #207619 - Flags: review?(bugmail)
Comment on attachment 207619 [details] [diff] [review] Alternate patch: use a temporary pointer to avoid ?: Hmm... The problem is that pointers don't keep stuff alive as far as I know... Would it make more sense to always call TrimCharsInSet but to sometimes pass in an empty set? Using ?: for the set argument should work fine...
(In reply to comment #24) > (From update of attachment 207619 [details] [diff] [review] [edit]) > Hmm... The problem is that pointers don't keep stuff alive as far as I know... I'm very much a novice here, but where in this process are we relying on the pointer to keep anything alive? Is Mozilla aggressive enough about garbage collection that it might delete the space for the returned strings (the memory tracked by vPtr) before it is assigned to v in the next line? > Would it make more sense to always call TrimCharsInSet but to sometimes pass in > an empty set? Using ?: for the set argument should work fine... Inefficient, but clever. That may be the best solution, under the circumstances. Would it be better to set the value of the "kWhitespace" variable change depending on the conditional, or to use ?: in the function argument itself (to decide between kWhitespace and "")?
> Is Mozilla aggressive enough about garbage collection that it might delete the > space for the returned strings Strings are not garbage collected. The returned string there is basically a temporary object that's stack-allocated; it gets deallocated as soon as the reference to it goes out of compiler scope.... and with the pointer change there is no longer a reference to it at all. I'd just use ?: in the function argument, I think.
Attached patch Use ?: in the trim function's argument (obsolete) (deleted) — — Splinter Review
As suggested in comment 24, this patch simply adds a conditional ?: in the first argument of nsContentUtils::TrimCharsInSet() to do a "null trim" for <input value="..."> attributes. It's not as efficient as it could be, but it's no less efficient than the status quo.
Attachment #207617 - Attachment is obsolete: true
Attachment #207619 - Attachment is obsolete: true
Attachment #207640 - Flags: superreview?(bzbarsky)
Attachment #207640 - Flags: review?(bugmail)
Attachment #207619 - Flags: superreview?(bzbarsky)
Attachment #207619 - Flags: review?(bugmail)
Comment on attachment 207640 [details] [diff] [review] Use ?: in the trim function's argument >+ // Using ?: outside the function call would be more efficient, but >+ // we don't trust ?: with our string classes. It's not the string classes but rather references in general that are tricky. So change "our string classes" to "references" > const nsAString& v = >- nsContentUtils::TrimCharsInSet(kWhitespace, aNode.GetValueAt(i)); >+ nsContentUtils::TrimCharsInSet( >+ !(nodeType == eHTMLTag_input >+ && keyAtom == nsHTMLAtoms::value) ? >+ kWhitespace : "", aNode.GetValueAt(i)); How about getting rid of the ! and switching places between kWhitespace and "". Also, put the && on the line above to follow convention. r=me with that Please attach a new file once you get sr comments too since you'll need someone else to check in the patch.
Attachment #207640 - Flags: review?(bugmail) → review+
Comment on attachment 207640 [details] [diff] [review] Use ?: in the trim function's argument sr=me with sicking's comments addressed.
Attachment #207640 - Flags: superreview?(bzbarsky) → superreview+
Though I have to ask... Does IE trim newlines on <input type="text">?
And with this patch, how would we render a <input type=text> with a newline in the value?
Attached patch Update of previous patch after review (obsolete) (deleted) — — Splinter Review
Here's the patch after applying the suggestions from comment 28. As for comment 30, I agree that the question of how to handle the various types of input elements is a bit vexing (I don't know what happens in IE in that case). Most of the arguments for this behavior have related to input type="hidden"; I don't know whether or not authors commonly rely on one behavior or the other for type="text". If there were a way to restrict this to type="hidden", it would at least be good to know about it. And regarding comment 31, I refer you to bug 256027 (which I've just posted a testcase to and confirmed). Newlines in text inputs seem to be unambiguously bad. I'm not sure where in the process we should be stripping them, but I doubt that anyone out there is using them (intentionally!).
Attachment #207640 - Attachment is obsolete: true
IE6.0 displays <input type="text" value="\n text \n"> (where \n is a newline in the source) as " text " (same as FF 1.5).
IE does trim newlines for type=text :( But we can't trim in the sink sink without inspecting all attributes since it does not trim for type=hidden. One alternative is to trim in GetValue. What do you think?
Hmm.. So keep the newlines in the actual attr (per current patch), but strip them out in the frame, basically? That may work.
Attached patch Only trim newlines for hidden values (obsolete) (deleted) — — Splinter Review
Others clearly know the code better than I do, but I would think that doing this trimming every time the value is retrieved would be pretty inefficient. I don't know how much of a performance hit it would be, of course; maybe that's the way to go. But I can think of two other possible resolutions. First, bug 322270 might address much of this issue: my earlier patch would make sure we do the right thing for everything but value attributes, and a fix to bug 322270 will hopefully strip newlines from non-hidden value attributes as we want. (This bug feels pretty close to being a special case of that one, in fact, though that may depend on how each one ends up getting interpreted and fixed.) It might be reasonable to watch and see if a fix there would take care of the outstanding concerns here. The second possible resolution is the attached patch, which takes a somewhat different approach. Here, as we read the list of attributes, we delay processing the value attribute on input elements until after all the others. We also watch the attribute values as they are read in to see if this is a hidden input. When an input element's value attribute is eventually processed, we decide how to handle whitespace based on whether the "isHiddenInput" flag is set. The main point of concern for me here is the process of recognizing and setting the "isHiddenInput" flag. I don't know where to look in the code for how these are recognized elsewhere, so what I've done here might be more fragile than I think. And I really don't have a good understanding of Mozilla's string classes, so I picked a string comparison approach that seems plausible and crossed my fingers. Corrections and suggestions are very welcome (as are warnings that a patch along these lines wouldn't be what we want at all).
What happens in IE if you have an <input type="text"> with newlines in its value="" and you then switch the type to "hidden" dynamically? Does it lose its newlines? Get them back? How about if you switch back? Or do it in the other order?
Hmm.. I could have sworn that I could change the type from "text" to "hidden" yesterday, but now i can't replicate that. Anyhow, IE basically doesn't allow switching inputs between different types. Additionally, I don't think we need to emulate IE exactly, especially when it comes to dynamic behaviour. This is because we already behave quite differently from IE since our value DOM attribute doesn't change when .value changes. What I do think is important is that we behave the same when no scripting is involved since there are a lot more forms like that out there.
I don't like the approach in attachment 207739 [details] [diff] [review], it's putting way too much logic in the contentsink. We do need to deal with <input type=text> though. Currently, if you set the value attribute to a value containing a newline .value will be truncated at the newline as long as the textcontrol frame is around. So we would efficently be setting the value to emptystring if we just used attachment 207640 [details] [diff] [review]. What I think we should do is to use attachment 207640 [details] [diff] [review], but make nsHTMLInputElement::GetValue strip newlines for type=text. There is very little performance concern with doing that btw. (we don't need to sript newlines when we nsHTMLInputElement::GetValue asks the frame for the value since there should never be any newlines in the frame)
Attached file Expanded testcase (obsolete) (deleted) —
This is an expanded version of the original testcase, including hints on what you're supposed to see and examples of text and button inputs whose values include line breaks before, during, and after the main content. Strangely, applying only my reviewed patch from attachment 207657 [details] [diff] [review] to a 1.8 branch build, the initial newline in the text input has apparently already been stripped: the text input shows the intended content with no leading whitespace (or at least, the beginning of the intended content: the linebreak in the middle still cuts it off). The button element behaves terribly, though; I would think we'd want to fix that at some point. People should probably test this on other platforms. And if someone could find the spot in the code where the text box's value is getting the initial newline stripped, I'd love to see it.
Attachment #105514 - Attachment is obsolete: true
Steuard: Will you be able to write up a patch that does what I describe in comment 39?
[This patch is against the 1.8 branch, but the relevant code looks unchanged on the trunk.] (In reply to comment #41) > Steuard: Will you be able to write up a patch that does what I describe in > comment 39? I've already got one... maybe: I have no idea how to test it. I created the new testcase in comment 40 in order to test this patch, but when the test worked even before I compiled with the change to nsHTMLInputElement::GetValue, I decided to post that observation before adding yet another attachment here. I _think_ that this is what you had in mind, but I'm not certain, and as I said I don't know how to test it. Note that this patch still doesn't fix the problem with newlines beginning a button's value attribute. Actually, though, bug 55285 comment 30 and 31 argued that this was the desired behavior, and that's what made it into the official patch. I very much disagree, given that many text editors or the "tidy" utility happily word wrap attribute text (and given that the <button> tag exists specifically to "offer richer rendering possibilities"), but that may be a separate issue.
Ack. I meant to include some extra context for the new part there; it's pretty opaque as it stands. Here are eight lines on either side: ---------- if (frameOwnsValue) { formControlFrame->GetProperty(nsHTMLAtoms::value, aValue); } else { if (!GET_BOOLBIT(mBitField, BF_VALUE_CHANGED) || !mValue) { GetDefaultValue(aValue); } else { CopyUTF8toUTF16(mValue, aValue); } + + // Bug 114997: trim whitespace for non-hidden inputs + static const char* kWhitespace = "\n\r\t\b"; + aValue = nsContentUtils::TrimCharsInSet(kWhitespace, aValue); } return NS_OK; } // Treat value == defaultValue for other input elements nsresult rv = GetAttr(kNameSpaceID_None, nsHTMLAtoms::value, aValue); ----------
Sorry, looks like you need to patch SetValueInternal too so that we strip before setting the value in the frame (when we do set it in the frame).
As with the GetValue fix, this change to SetValueInternal only happens if the frame does not own the value. I've skimmed through all of the other occurrances of mValue in nsHTMLInputElement.cpp, and I _think_ these are the only ones that need changing, but I won't entirely swear to it. As before, this patch seems to work just fine in my recently attached testcase (apart from issues with type=button), but it looked right even before the changes to the input element code, too. Anyone with clever ideas on how to test it beyond what's attached should speak up.
Attachment #207765 - Attachment is obsolete: true
> As before, this patch seems to work just fine in my recently attached testcase > (apart from issues with type=button), but it looked right even before the > changes to the input element code, too. Ok. After coming back and looking at these results again in Firefox 1.5 and in my patched 1.5.0.1 build, I can summarize as follows: 1. My patch above does fix the issue reported in this bug: the hidden input's value starts out with linebreaks at the beginning and end just as it should. 2. The behavior of newlines in text inputs is unchanged, for better and worse: initial and final line breaks are stripped (good), but internal linebreaks remain and make some text invisible (quite bad). But that's a different bug. 3. Button inputs can now have even worse problems, but that's not my fault: as I described in comment 42, I believe that the chosen fix for bug 55285 was a mistake (I'd much rather go back to something like the fix in bug 55285 comment 29). If we stick with the patch in bug 55285 comment 33, then the weirdness here is pretty close to "desired behavior". In short, there are still some open questions, but as far as I can tell my patch does fix this particular bug. As usual, suggestions for further testing are welcome.
> 3. Button inputs can now have even worse problems, Like what?
In reply to comment 47: > > 3. Button inputs can now have even worse problems, > Like what? Like this. As I see it, the correct display of the <input type="button"> in the testcase is a single-line button reading "first and second". (Anyone who wants fancier formatting can use <button>.) In Firefox 1.5, what we actually get is "first and\\second", where that "\\" denotes a line break. But as this screen capture from a 1.8 branch build with my patch applied shows, what we get now is something line "\\ \\first and\\second" (there's a _lot_ of whitespace at the top, and none at the bottom). It's almost as if button height is being computed for the full value (with newlines as "white-space: pre") and _then_ the displayed text is getting leading and trailing newlines stripped (perhaps in the same as-yet-unknown place that the text input's leading and trailing newlines are being removed). In practice, I doubt that many people at all have HTML with a newline between button text and the surrounding quotes, so this strange change in behavior probably won't have much impact on the real web. But presumably _something_ should be done to fix the way buttons work in this case, quite apart from anything we do in this bug.
Ah. Yeah, I wouldn't consider the behavior there to be a problem... I'm not even sure why the button behavior changes at all with this patch, actually. Do buttons get the value attr instead of using GetValue() or something?
Just for the record, I tried bug 55285 attachment 106069 [details] [diff] [review] (or rather, the same basic idea but with the new file path) in conjunction with my patch for this bug, and buttons do exactly what I consider reasonable (no extra whitespace, no linebreaks at all). Others have clearly disagreed on the desired behavior, but this _is_ one consistent solution. But button issues aside, should I be requesting review for the latest patch here?
> and buttons do exactly what I consider reasonable Not with spaces in the middle of the value. Or on the sides. Pages depend on the existing behavior for those... And yes, if this works you should probably see what sicking thinks.
Comment on attachment 208137 [details] [diff] [review] Above, plus corresponding change in SetValueInternal Requesting review for the latest patch. Meanwhile, if "white-space: pre" really is desired behavior for buttons (including newlines), then "type=button" should probably be added to "hidden" as an exception in bug 322270. (Or should this be a standards mode/quirks mode distinction somehow?)
Attachment #208137 - Flags: superreview?(bzbarsky)
Attachment #208137 - Flags: review?(bugmail)
The desired behavior for buttons is to keep spaces, not newlines.
(In reply to comment #49) > I'm not even sure why the button behavior changes at all with this > patch, actually. Do buttons get the value attr instead of using > GetValue() or something? Looks like the answer is yes, at least if I've understood the code correctly: http://lxr.mozilla.org/mozilla1.8/source/layout/forms/nsHTMLButtonControlFrame.cpp#202 ---------- NS_IMETHODIMP nsHTMLButtonControlFrame::GetValue(nsAString* aResult) { return nsFormControlHelper::GetValueAttr(mContent, aResult); } ---------- This GetValueAttr() function is defined in nsFormControlHelper.cpp; as the name indicates, it finds the value using GetAttr(). And this GetValue() function is inherited and called in turn by nsGfxButtonControlFrame::CreateAnonymousContent(): http://lxr.mozilla.org/mozilla1.8/source/layout/forms/nsGfxButtonControlFrame.cpp#159 ---------- result = GetValue(&initvalue); ---------- I'm reasonably sure that's where the button text is actually being determined. Given that nsGfxButtonControlFrame exists entirely to produce buttons from input elements (or so says the comment before its declaration), couldn't we just replace this with result = mContent.GetValue(&initvalue); In fact, isn't that what we _should_ be doing? Or would it be better to make this change in nsHTMLButtonControlFrame::GetValue() instead? (Is it safe to assume that nsHTMLButtonControlFrame applies only to <input type=button>s as well?) Assuming some change along those lines should be made, would it be best for me to file a separate bug for this and post this little patch there (with appropriate updates to surrounding comments)? Or should I just include this change in my patch for this bug, since it's small and somewhat related?
(In reply to comment #54) > I'm reasonably sure that's where the button text is actually being determined. > Given that nsGfxButtonControlFrame exists entirely to produce buttons from > input elements (or so says the comment before its declaration) That's correct. > couldn't we just replace this with > > result = mContent.GetValue(&initvalue); Well, with some QIing and such, but yeah. I think we could. > In fact, isn't that what we _should_ be doing? I think so. > Or would it be better to make this change in > nsHTMLButtonControlFrame::GetValue() instead? (Is it safe to assume that > nsHTMLButtonControlFrame applies only to <input type=button>s as well?) nsHTMLButtonControlFrame is used for <button> and <input type="button">. I don't think we should be changing it here. > Assuming some change along those lines should be made, would it be best for me > to file a separate bug for this and post this little patch there (with > appropriate updates to surrounding comments)? Yeah, that sounds like the right approach. If nothing else, it will make reviewing easier. Thanks for looking into this!
(In reply to comment #55) > > Assuming some change along those lines should be made, would it be best for me > > to file a separate bug for this and post this little patch there (with > > appropriate updates to surrounding comments)? > Yeah, that sounds like the right approach. I've filed bug 326250 on this issue, copying over most of my conclusions in comment 54. Unfortunately, I know nothing about QI (yet) and I don't have the time to puzzle it out myself right now. Hopefully, someone who does will be able to produce a valid patch without too much trouble. I haven't listed bug 326250 as a blocker for this one because the issue is unlikely to arise much in practice, but it would still be nice to see both bugs fixed at close to the same time.
So the change to nsHTMLInputElement::GetValue only applies to text, password, and file inputs (not buttons, in particular). Hence the effect observed in bug 326250 comment 5.
> So the change to nsHTMLInputElement::GetValue only applies to text, password, > and file inputs (not buttons, in particular). D'oh. Thanks for testing that for me, and for spotting my mistake. This patch now trims whitespace characters (other than spaces) from the start and end of _all_ input element value attributes except type=hidden. Together with your patch to bug 326250, my expanded testcase now behaves properly (at least as far as leading and trailing whitespace are concerned; newlines in the middle of elements are a different bug). What are the odds that this patch would be appropriate for 1.8.1? It's not a big change, and I doubt that the fix for bug 326250 would be necessary very often in practice (though as I said there, I do have a working port of that patch to the 1.8 branch if you want it).
Attachment #207657 - Attachment is obsolete: true
Attachment #207739 - Attachment is obsolete: true
Attachment #208137 - Attachment is obsolete: true
Attachment #212131 - Flags: superreview?(bzbarsky)
Attachment #212131 - Flags: review?(bugmail)
Attachment #208137 - Flags: superreview?(bzbarsky)
Attachment #208137 - Flags: review?(bugmail)
Comment on attachment 212131 [details] [diff] [review] Corrected patch to apply to all non-hidden inputs >Index: content/html/content/src/nsHTMLInputElement.cpp >@@ -724,29 +735,34 @@ nsHTMLInputElement::SetValueInternal(con >+ // Bug 114997: trim \n, etc. for non-hidden inputs >+ aValue = nsContentUtils::TrimCharsInSet(kWhitespace, aValue); aValue is const. Does this actually work? sr=bzbarsky if it does...
Attachment #212131 - Flags: superreview?(bzbarsky) → superreview+
Attached patch Treat aValue as const; otherwise, same logic (obsolete) (deleted) — — Splinter Review
I'm not sure how to test whether the const aValue caused problems for the previous patch, but now that you point out the issue I'm not happy with that code in any case (I could even imagine different compilers handling it differently). So here's a new version that sidesteps the issue and is probably cleaner in general. My only remaining concern is formatting: I'm stuck with either some ugly non-standard indentation or a really long line; I've opted for the latter. If there are style guidelines that suggest otherwise, let me know.
Attachment #212131 - Attachment is obsolete: true
Attachment #213276 - Flags: superreview?(bzbarsky)
Attachment #213276 - Flags: review?(bugmail)
Attachment #212131 - Flags: review?(bugmail)
Doesn't that have the ?: issue with strings? In general I think creative indentation is preferred to overlong lines.
(In reply to comment #61) > Doesn't that have the ?: issue with strings? Yes, yes it does. Blast. Here's a version treating the ?: the same way as in nsHTMLContentSink.cpp. Incidentally, I think it should be _really_ easy to modify this patch to also fix bug 256027. In the two places in nsHTMLInputElement.cpp where I deal with text/password/file inputs, simply follow nsContentUtils::TrimCharsInSet() with the moral equivalent of "s/[kWhitespace]/ /g". I haven't been able to spot a function that will do this (nothing like full regex handling is actually needed, of course), but it's hard to imagine that such a function isn't already defined somewhere. So I'll leave this patch up for review, but if someone points me in the direction of the right function to apply here I will happily extend it to make this additional fix. (Or, alternately, we could get this checked in and then submit a separate patch in bug 256027.)
Attachment #213276 - Attachment is obsolete: true
Attachment #213760 - Flags: superreview?(bzbarsky)
Attachment #213760 - Flags: review?(bugmail)
Attachment #213276 - Flags: superreview?(bzbarsky)
Attachment #213276 - Flags: review?(bugmail)
Comment on attachment 213760 [details] [diff] [review] Same idea once again, but without unreliable string ?: No, the idea in comment 44 was to *only* trim when the value is set in the frame. However I'm starting to have second throughts if that is needed at all since SetValueInternal is only called when someone calls .value = "foo" and for that I don't think we want to strip whitespace, do we?
(In reply to comment #63) > No, the idea in comment 44 was to *only* trim when the value is set in the > frame. My apologies for misunderstanding your point. I'm sufficiently new to the Mozilla code that I still don't have a clear notion of what it means for the frame to own the value (or what the alternative(s) is/are). I'm starting to suspect that perhaps it's not a great use of reviewers' time to make you guys hold my hand all the way through the learning experience (though I've certainly appreciated it!). > However I'm starting to have second throughts if that is needed at all > since SetValueInternal is only called when someone calls .value = "foo" and > for that I don't think we want to strip whitespace, do we? I think you're right: the issue here is whether to strip whitespace from the HTML source, not whether whitespace should be allowed at all. (There might be some question of the latter, actually, given problems like bug 256027 with newlines in text inputs, but I'm pretty sure this isn't the bug to resolve them. For the record, even with this new patch initial and trailing whitespace in the argument to SetValue is being stripped somewhere.) Once again, my thanks to you folks for putting up with my rather patchy understanding of the code while I've figured out what this QI and SetValue business is all about. I've now attached a version of the patch that doesn't change SetValueInternal at all. Other than that, it's identical to the previous version.
Attachment #213760 - Attachment is obsolete: true
Attachment #218718 - Flags: superreview?(bzbarsky)
Attachment #218718 - Flags: review?(bugmail)
Attachment #213760 - Flags: superreview?(bzbarsky)
Attachment #213760 - Flags: review?(bugmail)
Attachment #207760 - Attachment is obsolete: true
Comment on attachment 218718 [details] [diff] [review] Same patch, but without changes to SetValueInternal Looks good!
Attachment #218718 - Flags: review?(bugmail) → review+
The "frame owns the value" when the the text-input is being displayed and there is an editor instantiated that allows the user to change the value of the input. We do this so that we don't have to for every edit update mValue inside the nsHTMLInputElement, instead we flag that the editor is the authoritative source of the value.
Comment on attachment 218718 [details] [diff] [review] Same patch, but without changes to SetValueInternal sr=bzbarsky. Thanks for being patient with this!
Attachment #218718 - Flags: superreview?(bzbarsky) → superreview+
Yes, certainly. And thanks for the patch :) Many versions of the patch is not a problem, we all started there. Hacking mozilla takes some learning and this class is certainly not the easiest place to start. Just keep at it!
Assignee: harishd → steuard+moz
Status: ASSIGNED → NEW
Attached patch Merged to tip (deleted) — — Splinter Review
Attachment #218718 - Attachment is obsolete: true
Patch checked in on trunk. Steuard, thanks again for doing this!
Status: NEW → RESOLVED
Closed: 19 years ago
Priority: P2 → --
Resolution: --- → FIXED
Whiteboard: [DUPEME]
(In reply to comment #71) > Patch checked in on trunk. Great! Should I ask for branch approval for this (perhaps after waiting a few days for regressions to show up on trunk)? It's a relatively small change, and the issue it fixes may be important for compatibility with a variety of web applications. As I've mentioned, my work on this bug has all been done on branch, so I know that it compiles and functions properly there (at least on Mac). If so, any hints about whom to request for review would be appreciated; I know I found a list at one point, but I haven't been able to track it down again (and figuring out the right people to ask always feels like black magic). > Steuard, thanks again for doing this! No problem: it was fun, despite the occasional false turn along the way. And I've been a fan of the Mozilla project since it started; it's nice to be able to contribute to it occasionally.
You could ask jst for approval for the 1.8.1 branch. I don't think we can get it on the 1.8.0 branch
For 1.8 branch, I guess we could do this... I'd give it a week or two on trunk; if nothing comes up, request approval1.8.1 from a content module owner (so sicking, me, jst, peterv, etc). See http://www.mozilla.org/owners.html for the module owner info.
Doh! I just noticed that this patch contained an error.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Tweak (deleted) — — Splinter Review
So we don't want to trim whitespace if the value is transferred to content-node again. We only want to trim when getting the value from the attribute. I'm not sure if modifying GetDefaultValue or modifying all callsites to it is the right thing to do. But it feels logical that .defaultValue == .value unless .value has been changed.
Attachment #219422 - Flags: superreview?(bzbarsky)
Attachment #219422 - Flags: review?(bzbarsky)
Comment on attachment 219422 [details] [diff] [review] Tweak Make that \n\r\t\b thing a static string so it's obviously the same both places, and looks good... r+sr=bzbarsky
Attachment #219422 - Flags: superreview?(bzbarsky)
Attachment #219422 - Flags: superreview+
Attachment #219422 - Flags: review?(bzbarsky)
Attachment #219422 - Flags: review+
Checked in
Status: REOPENED → RESOLVED
Closed: 19 years ago → 19 years ago
Resolution: --- → FIXED
(In reply to comment #76) > Created an attachment (id=219422) [edit] > Tweak ... > We only want to trim when getting the value from the attribute. Thanks for catching this, and for the fix.
This is just a combination of attachment 218723 [details] [diff] [review] and attachment 219422 [details] [diff] [review] (as checked in to trunk), but with the diff against the 1.8 branch rather than trunk. Bon Echo compiles fine with these changes and seems to behave as expected, so it seems like fixing this for Firefox 2.0 would be reasonable (and might be appreciated by web developers). Two other notes. First, as usual I'll need someone else to do the branch checkin if this is approved. And second, if anyone has concerns about obscure button behaviors as described in comment 48, I do have a 1.8 branch backport of the patch for bug 326250.
Attachment #220965 - Flags: approval-branch-1.8.1?(bzbarsky)
Hmm... I'm actually wondering whether we'd want to take bug 326250 (and bug 330469, then) in addition to this patch... Steuard, do you think you can put together a 1.8 branch backport of those three patches so I can get an idea of what it looks like all together?
I mean put together in a single patch.
This patch for the 1.8 branch combines the fixes for this bug, bug 326250, and bug 330469. At least on my Mac, Bon Echo compiles properly and passes the testcases on these bugs (apart from remaining linebreaks in the /middle/ of value attributes, of course). The main change to the patch for bug 326250 is the retention of nsFormControlHelper::GetValueAttr, which is still used on the 1.8 branch. The only change to the patch for bug 330469 was a substitution of nsHTMLAtoms::value for nsGkAtoms::value, as nsGkAtoms doesn't seem to exist on the branch. As a side note, I'm still in the market for a clean way to fix bug 256027 along the lines of what I suggested in comment 62. Any suggestions for the right string processing function to use would be appreciated; that fix would then be quick and easy. And finally, speaking of nsGkAtoms on trunk, there's still an occurrance of nsHTMLAtoms::value at nsGfxButtonControlFrame.cpp line 286 on trunk; should that be changed to use nsGkAtoms::value instead?
Attachment #220965 - Attachment is obsolete: true
Attachment #221318 - Flags: approval-branch-1.8.1?(bzbarsky)
Attachment #220965 - Flags: approval-branch-1.8.1?(bzbarsky)
No need to go through and change all ns*Atoms into nsGkAtoms manually. I'll write up search-n-replace patch soon that does it across the codebase.
Comment on attachment 221318 [details] [diff] [review] 1.8.1 branch patch for this+326250+330469 branch181=bzbarsky
Attachment #221318 - Flags: approval-branch-1.8.1?(bzbarsky) → approval-branch-1.8.1+
Fixed on 1.8 branch.
Keywords: fixed1.8.1
Comment on attachment 221318 [details] [diff] [review] 1.8.1 branch patch for this+326250+330469 +static const char* kWhitespace = "\n\r\t\b"; this should be more efficient per http://people.redhat.com/drepper/dsohowto.pdf: +static const char kWhitespace[] = "\n\r\t\b";
biesi, r+sr=bzbarsky to make that change.
I checked in biesi's suggested change, fwiw.
Thanks; I didn't get a chance to do this myself in the last few days.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: