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)

x86
Windows NT
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 2.16

People

(Reporter: jouni, Assigned: bbaetz)

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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.
Attached patch v1 (obsolete) — Splinter Review
The same fix as in 144728.
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.
Attached patch v2 (obsolete) — Splinter Review
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.
Attachment #91273 - Flags: review+
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+
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
Attached patch final versionSplinter Review
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
Closed: 22 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: 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
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: