Closed
Bug 157074
Opened 22 years ago
Closed 22 years ago
verify-new-product doubles comment linefeeds on Win32
Categories
(Bugzilla :: Creating/Changing Bugs, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.16
People
(Reporter: jouni, Assigned: bbaetz)
Details
(Keywords: regression)
Attachments
(1 file, 2 obsolete files)
3.71 KB,
patch
|
bbaetz
:
review+
bbaetz
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•22 years ago
|
||
The same fix as in 144728.
Reporter | ||
Updated•22 years ago
|
Assignee | ||
Comment 2•22 years ago
|
||
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?
Reporter | ||
Comment 3•22 years ago
|
||
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.
Assignee | ||
Comment 4•22 years ago
|
||
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.
Assignee | ||
Comment 5•22 years ago
|
||
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 6•22 years ago
|
||
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.
Attachment #91273 -
Flags: review+
Reporter | ||
Comment 7•22 years ago
|
||
Comment on attachment 91273 [details] [diff] [review] v2 Works on Win32, looks good -> r=jouni, but I agree with Myk's nits.
Attachment #91273 -
Flags: review+
Assignee | ||
Comment 8•22 years ago
|
||
Taking. I've renamed teh filter to html_linebreak, and I 'll check it in later this afternoon
Assignee: jouni → bbaetz
Status: ASSIGNED → NEW
Comment 9•22 years ago
|
||
make sure the final version of the patch gets attached to the bug
Assignee | ||
Comment 10•22 years ago
|
||
I forgot about this; I'll check it in tomorrow
Attachment #91273 -
Attachment is obsolete: true
Assignee | ||
Comment 11•22 years ago
|
||
Comment on attachment 91774 [details] [diff] [review] final version Carrying forward r1 and r2
Attachment #91774 -
Flags: review+
Assignee | ||
Comment 12•22 years ago
|
||
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
Closed: 22 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 13•22 years ago
|
||
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: 1.2.2.2; previous revision: 1.2.2.1 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
Updated•11 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•