Bug 45931 - trimSpaces incorrectly modifies output
Summary: trimSpaces incorrectly modifies output
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 6
Classification: Unclassified
Component: Jasper (show other bugs)
Version: 6.0.16
Hardware: All All
: P2 enhancement (vote)
Target Milestone: default
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-10-01 12:43 UTC by Meetesh Karia
Modified: 2017-06-06 12:10 UTC (History)
1 user (show)



Attachments
replace whitespace with single space (308 bytes, patch)
2008-10-01 12:43 UTC, Meetesh Karia
Details | Diff
Adds a third state for the trimSpaces flag (13.64 KB, patch)
2008-11-07 13:30 UTC, Meetesh Karia
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.
Comment 9 Mark Thomas 2017-04-07 14:18:53 UTC
First of all, let me apologise for letting this one slip through the cracks.

I'm currently reviewing all open Tomcat enhancement requests as part of the end-of-life process.

I am struggling to see a use case for this option. trimSpaces applies to each block of template text in turn. I'm struggling to see a (non-contrived) use case where the option for a single space would be useful. Therefore, I am resolving this as WONTFIX.

I'd be more than happy to revisit this but I would ask that a sample JSP be provided (ideally one that works when dropped into a clean Tomcat install) that demonstrates a use case for this new option.
Comment 10 David Kavanagh 2017-05-29 11:26:55 UTC
Hello,

I am reopening this enhancement request, at the request of Mark Thomas.
I have come across an issue with trimSpace removing white space from
parts of the HTML code.
The use case is found in the following mailing list thread.

http://mail-archives.apache.org/mod_mbox/tomcat-users/201705.mbox/%3CCAFOY9=DN=Y-_7iZm38Kf4waGSOpzw4NyuLYOLA_PLxfm7VO4Cg@mail.gmail.com%3E

Thanks
Comment 11 Mark Thomas 2017-06-06 12:10:45 UTC
A variation of this patch has been applied to 9.0.x for 9.0.0.M22 onwards.