On Win32, going through the verify-new-product template doubles linefeeds on all comments. This is exactly the same as bug 144728; please see that for further information.
Status: NEW → ASSIGNED
Keywords: patch, regression, review
Target Milestone: --- → Bugzilla 2.16
Comment on attachment 91088 [details] [diff] [review] v1 This code used to use value_quote. value_quote changes this to &013;, not \n. (It also converts \n\r, but I think that was just to cover all the bases) Looking at bug 32000 + friends, it appears that that was deliberate, because of html whitespace collapsing rules. See bug 4928 (I think) This (and bug 144728) should be made to escape like value_quote used to, shouldn't it? And we should move this out into a common routine in CGI.pl while we're at it. Actually, 2.14 value_quoted all the hidden text fields. Should this rather be a TT filter?
Yes, I agree with you. Do you want all that stuff for 2.16? If so, consider taking this; I won't have time for that until at some point next week. If not, r= this and let's file another for 2.18. I'd go for patching 2.16 with this and doing a better fix later on.
We need this for 2.16, else several browsers will break. Not sure which ones - there are several mentioned in the various bugs, but the whole issue is confused a bit.
Created attachment 91273 [details] [diff] [review] v2 I can't notice a difference on unix (mozilla doesn't actually do this whitespace collapsing, due to a bug) jouni, does this work for you on windows?
Attachment #91088 - Attachment is obsolete: true
Comment on attachment 91273 [details] [diff] [review] v2 >Index: globals.pl >@@ -1561,6 +1561,20 @@ >+ # HTML collapses newlines in element attributes to a single space, >+ # so form elements which may have whitespace (ie comments) need >+ # to be encoded using
Nit: when I read a function description, I expect the first sentence of the description to explain what the function does, so when I looked at this comment the first time I read it as "HTML-collapses newlines..." and got confused. Say something like this instead: Encodes line breaks in strings used as HTML attribute values. Useful when passing data from one script to another via hidden form fields, since HTML collapses line breaks... >+ # See bug 4928, bug 22983 and bug 32000 for more details Nit: "See bugs 4928, 22983, and 32000..." >+ html_newline => sub Nit: "line break" is the generic term for the \n (new line) and \r (carriage return) characters. >Index: template/en/default/global/hidden-fields.html.tmpl >@@ -28,5 +28,5 @@ >+ <input type="hidden" name="[% field.key %]" value="[% field.value FILTER html FILTER html_newline %]"> Nit: it's a long line, so using the FILTER short-hand (|) would help it out: [% field.value FILTER html | html_newline %] All review issues nits, and it works on Linux, so r=myk, but someone should test on Windows (at least) and give second review.
Comment on attachment 91273 [details] [diff] [review] v2 Works on Win32, looks good -> r=jouni, but I agree with Myk's nits.
Taking. I've renamed teh filter to html_linebreak, and I 'll check it in later this afternoon
Assignee: jouni → bbaetz
Status: ASSIGNED → NEW
make sure the final version of the patch gets attached to the bug
Created attachment 91774 [details] [diff] [review] final version I forgot about this; I'll check it in tomorrow
Attachment #91273 - Attachment is obsolete: true
Comment on attachment 91774 [details] [diff] [review] final version Carrying forward r1 and r2
Attachment #91774 - Flags: review+
Checked into trunk and branch. Attachment 91774 [details] [diff] applied to the branch without conflicts, only some offsets. myk, you may want to update bmo.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
This fix busted the tree, because the revised post-nit patch didn't rename the filter call in hidden-templates, causing all calls to hidden-templates to fail. I checked in the following to fix the problem. Both patches just change hidden-fields to call html_linebreak instead of html_newline. Checking in template/en/default/global/hidden-fields.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/hidden-fields.html.tmpl,v <-- hidden-fields.html.tmpl new revision: 220.127.116.11; previous revision: 18.104.22.168 done Checking in template/en/default/global/hidden-fields.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/hidden-fields.html.tmpl,v <-- hidden-fields.html.tmpl new revision: 1.4; previous revision: 1.3 done
You need to log in before you can comment on or make changes to this bug.