157074 - verify-new-product doubles comment linefeeds on Win32

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Jouni Heikniemi]

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.

Comment 1

[Jouni Heikniemi]

Attachment: v1

The same fix as in 144728.

Comment 2

[Bradley Baetz (:bbaetz)]

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?

Comment 3

[Jouni Heikniemi]

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. 

Comment 4

[Bradley Baetz (:bbaetz)]

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.

Comment 5

[Bradley Baetz (:bbaetz)]

Attachment: 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?

Comment 6

[Myk Melez [:myk] [@mykmelez]]

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 7

[Jouni Heikniemi]

Comment on attachment 91273 [details] [diff] [review]
v2

Works on Win32, looks good -> r=jouni, but I agree with Myk's nits.

Comment 8

[Bradley Baetz (:bbaetz)]

Taking. I've renamed teh filter to html_linebreak, and I 'll check it in later
this afternoon

Comment 9

[Dave Miller [:justdave]]

make sure the final version of the patch gets attached to the bug

Comment 10

[Bradley Baetz (:bbaetz)]

Attachment: final version

I forgot about this; I'll check it in tomorrow

Comment 11

[Bradley Baetz (:bbaetz)]

Comment on attachment 91774 [details] [diff] [review]
final version

Carrying forward r1 and r2

Comment 12

[Bradley Baetz (:bbaetz)]

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.

Comment 13

[Jouni Heikniemi]

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