Bug 45931

Summary: trimSpaces incorrectly modifies output
Product: Tomcat 6 Reporter: Meetesh Karia <meetesh.karia>
Component: JasperAssignee: Tomcat Developers Mailing List <dev>
Status: REOPENED ---    
Severity: enhancement CC: slaurent
Priority: P2    
Version: 6.0.16   
Target Milestone: default   
Hardware: All   
OS: All   
Attachments: replace whitespace with single space
Adds a third state for the trimSpaces flag

Description Meetesh Karia 2008-10-01 12:43:22 UTC
Created attachment 22665 [details]
replace whitespace with single space

Enabling trimSpaces collapses all spaces to nothing which can end up removing desired whitespace from the generated html.

A possible solution to this is to reduce whitespace to a single space rather than removing it altogether.  The attached patch has this implementation.
Comment 1 Mark Thomas 2008-10-03 07:20:41 UTC
trimSpaces is equivalent to trimDirectiveWhitespaces

Section JSP.3.3.8 is clear that the whitespace must be removed completely.

trimSpaces can currently be true or false. I would not be against an enhancement that added the value "single" and in that case using the code in your patch.

Let us know if you need some pointers on writing a new patch.
Comment 2 Meetesh Karia 2008-10-03 10:22:50 UTC
Hi Mark,

Thanks for the comment.  The main question I have is whether you'd want to leave the -trimspaces option for JspC backwards-compatible (ie, if there's no "true", "false", or "single" after it, treat it as "true"), or whether you'd be ok with changing the behavior to requiring the value after the flag.

Also, if there's anything else that's important to keep in mind when writing a patch for tomcat, please share.

Thanks!

(In reply to comment #1)
> trimSpaces is equivalent to trimDirectiveWhitespaces
> 
> Section JSP.3.3.8 is clear that the whitespace must be removed completely.
> 
> trimSpaces can currently be true or false. I would not be against an
> enhancement that added the value "single" and in that case using the code in
> your patch.
> 
> Let us know if you need some pointers on writing a new patch.
> 

Comment 3 Mark Thomas 2008-10-03 10:50:13 UTC
Keep the current behaviour (ie true) if nothing is specified for trimspaces since that is backwards compatible.

Things to keep in mind:
 - patches should be in diff -u format
 - avoid changing stuff that isn't relevant (ie don't mix a functional patch with code clean-up)
 - keep the patch as simple as you can

Note I have re-opened this as an enhancement so it doesn't get lost.
Comment 4 Meetesh Karia 2008-11-07 13:30:40 UTC
Created attachment 22843 [details]
Adds a third state for the trimSpaces flag

This patch adds the "single" value for trimSpaces.  I wasn't able to find any automated tests for jasper so I did lots of manual testing.  If there is a test suite and someone can point me to it, I can add in an automated test or two.

Also, when trying to manually test this, I noticed that precompiling tags that worked against 6.0.18 didn't work against the trunk.  So, I was able to test the embeddable servlet option functionality against the trunk but only able to test the jspc functionality against 6.0.18.  Also, none of the places of code affected by the patch appear to have been changed between 6.0.18 and the current trunk.
Comment 5 Meetesh Karia 2008-12-09 11:04:21 UTC
Hi all, I was just wondering if there was a timeline for review/inclusion of this patch.  Thanks!
Comment 6 Trevor Cook 2009-03-20 06:33:31 UTC
(In reply to comment #5)
> Hi all, I was just wondering if there was a timeline for review/inclusion of
> this patch.  Thanks!

I'm wondering this as well.  Any ETA for a release?
Comment 7 Christopher Schultz 2009-10-19 08:08:58 UTC
I think it's worth mentioning that adding this feature potentially makes Tomcat incompatible with other app servers: if people use this "single" feature, and then move to another app server that doesn't support this kind of configuration, they'll have to change all their JSPs to suit.

If this feature is added, it should be very clear in the documentation that its use should be restricted to dire circumstances, and that JSP authors should be using the more portable workaround to forcing spaces to appear between directives, etc.:

${thingOne}${' '}${thingTwo}
Comment 8 Konstantin Kolinko 2010-08-25 17:06:38 UTC
(In reply to comment #7)
> I think it's worth mentioning that adding this feature potentially makes Tomcat
> incompatible with other app servers: if people use this "single" feature, and
> then move to another app server that doesn't support this kind of
> configuration, they'll have to change all their JSPs to suit.
>

In other web servers the space will not be removed at all. They will render several spaces instead of that single one. Nothing is lost, but just wasting some bandwidth.