122119 - cannot use consecutive space characters in applet parameters

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Peter Csizmadia]

Consecutive space characters in applet parameters are converted to only one
space. Example:
<applet NAME="x" VALUE="'  '">   (two spaces between quotes)
The value returned by getParameter("x") is "' '" (only one space between quotes)

Comment 1

[R.K.Aa.]

related: bug 50348

Comment 2

[Asa Dotzler [:asa]]

Over to harish. Harish, can you help me find a good home for this? I'm not sure
who should get this, parser, DOM? This could affect major sites that are doing
tickers and whatnot. 

Comment 3

[harishd]

This is certainly not a parser problem because parser does not compress
attribute values. AFAIK, this is a layout issue

Comment 4

[Peter Lubczynski]

Maybe caused by CompressWhitespace() at:
http://lxr.mozilla.org/mozilla/source/layout/html/base/src/nsObjectFrame.cpp#3007

Should this be changed to something like:
Trim("\b\t\r\n ", PR_TRUE, PR_TRUE, PR_FALSE);

Comment 5

[Chris Petersen]

Changing Priority to P2.

Comment 6

[Peter Csizmadia]

More than 4 months passed. Mozilla 1.0 has been released. Nothing changed. It  seems that you officially promoted this bug to feature. Will Netscape 7.0 have  this new feature too?  

Comment 7

[Johnny Stenback (:jst)]

Peterl, yeah, looks like your suggestion should fix this, how about getting this
checked in soon? If you land this on the trunk then maybe you could get this in
for Netscape 7.0 too?

Comment 8

[Peter Lubczynski]

Attachment: possible patch?

This patch changes |CompressWhitespace| into |Trim|. We now match IE and the
testcase passes. Please review.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 87098 [details] [diff] [review]
possible patch?

sr=jst. This looks very correct to me, the old code definately wasn't doing
what was expected, I think we should get this in for MachV.

Comment 10

[Johnny Stenback (:jst)]

Nominating and setting milestone (please reset if you disagree).

Comment 11

[harishd]

Comment on attachment 87098 [details] [diff] [review]
possible patch?


>-     name.CompressWhitespace();  // XXX right function?
>-     value.CompressWhitespace();
>+     name.Trim("\b\t\r\n ", PR_TRUE, PR_TRUE, PR_FALSE);
>+     value.Trim("\b\t\r\n ", PR_TRUE, PR_TRUE, PR_FALSE);

Can you order this differently? That is replace \b\t\r\n with \n\r\t\b.
Also, what about leading/trailing space characters?

Comment 12

[harishd]

>Also, what about leading/trailing space characters? 
Doh! didn't notice the space. Ignore this comment.

Anyway, the ordering should be " \n\r\t\b".

Comment 13

[Peter Lubczynski]

Attachment: patch flipping order of whitespace chars

Comment 14

[harishd]

Comment on attachment 87114 [details] [diff] [review]
patch flipping order of whitespace chars

r=harishd

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 87114 [details] [diff] [review]
patch flipping order of whitespace chars

sr=jst

Comment 16

[Peter Lubczynski]

patch in trunk (after 1.1 alpha), marking FIXED and nominating for 1.0 branch

Comment 17

[scottputterman]

Do we have examples on any major site where this is a problem?

Comment 18

[Peter Csizmadia]

http://www.jchem.com/example/index.jsp
http://www.chemaxon.com/marvin/doc/example-sketch3.2.html
and there are a lot more pages that may also show the bug on other sites that
use our applets

Comment 19

[Johnny Stenback (:jst)]

And besides, any major site out there can write an applet tag with param
elements in it that end up not working becase of this bug any time in the
future. This patch is as safe as they come, I see no reason not to take this on
the branch.

Comment 20

[rubydoo123]

This is a very important fix, preserving whitespace within attributes is a high
profile issue especially in regards to HTML complinace. The whitespace can have
significant meaning to a web author. This may seem minor to someone reviewing
this, but it is indeed worth the approval.

As Jonny stated, this a a very low-risk fix

Comment 21

[Paul Wyskoczka]

Shrir, can you verify this fix and update the bug. thx.

Comment 22

[shrirang khanzode]

testcase works now. I see two spaces just fine. Repoerter, pls check other tests 
that uhave. Thnx! verif on trnk0621 nt

Comment 23

[Peter Csizmadia]

The latest build works also with other test pages. Thanks!

Comment 24

[rubydoo123]

this should have been marked as nsbeta1+ already

Comment 25

[Jaime Rodriguez, Jr.]

Resolving as verified per Comment #22 From shrirang khanzode.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 87114 [details] [diff] [review]
patch flipping order of whitespace chars

Please land this on the 1.0.1 branch.  Once there, replace the
"mozilla1.0.1+" keyword with the "fixed1.0.1" keyword.

Comment 27

[scottputterman]

adding adt1.0.1+.  Please land this by the end of 6/24 on the branch.

Comment 28

[Peter Lubczynski]

landed on 1.0.1 branch

Comment 29

[Chris Petersen]

Verified in the branch OS X (2002-07-16-05) and Windows ME (2002-07-16-08)