Uploaded image for project: 'Struts 2'
  1. Struts 2
  2. WW-4634

Centre alignment doesn't seem to work in Velocity tags

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5.1
    • Component/s: Other
    • Labels:
      None

      Description

      When using a tag in a Velocity template, for instance #stextfield("label=Foo" "align=center") with the default xhtml theme, inputs are prepended with just <td> instead of <td align="center"> (as it happens in 2.3.x) in the generated HTML. A form where you want to centre some of the inputs simply doesn't render as intended.

      I'm using Velocity 1.7 and Velocity Tools 2.0, if that helps.

        Issue Links

          Activity

          Hide
          victorsosa victorsosa added a comment - - edited

          HI,

          I am not sure when this happen; but I think is because align is Not supported in HTML5.

          http://www.w3schools.com/tags/att_td_align.asp

          We shouldn't use the align attribute and instead use CSS.

          Show
          victorsosa victorsosa added a comment - - edited HI, I am not sure when this happen; but I think is because align is Not supported in HTML5 . http://www.w3schools.com/tags/att_td_align.asp We shouldn't use the align attribute and instead use CSS.
          Hide
          victorsosa victorsosa added a comment - - edited

          Can you please post the complete generated HTML code?

          Show
          victorsosa victorsosa added a comment - - edited Can you please post the complete generated HTML code?
          Hide
          cn42 Christoph Nenning added a comment -

          I would prefer to not generate inline css as that prevents usage of CSP.

          https://en.wikipedia.org/wiki/Content_Security_Policy

          What do you think about generating class names based on tag attributes? So applications can style on their own and yet decide on tag level which styles to apply.

          Show
          cn42 Christoph Nenning added a comment - I would prefer to not generate inline css as that prevents usage of CSP. https://en.wikipedia.org/wiki/Content_Security_Policy What do you think about generating class names based on tag attributes? So applications can style on their own and yet decide on tag level which styles to apply.
          Hide
          jontxu Jon Juaristi added a comment - - edited

          An excerpt:

          Velocity code:

          #stextfield ("align=center" "name=filter" "label=Filter")

          Generated HTML:

          Struts 2.3

          <tr>
              <td class="tdLabel"><label for="do_filter" class="label">Filter:</label></td>
              <td align="center"><input type="text" name="filter" value="" id="do_filter" align="center"></td>
          </tr>

          Struts 2.5

          <tr>
              <td class="tdLabel"><label for="do_filter" class="label">Filter:</label></td>
              <td><input type="text" name="filter" value="" id="do_filter" align="center"></td>
          </tr>
          Show
          jontxu Jon Juaristi added a comment - - edited An excerpt: Velocity code: #stextfield ( "align=center" "name=filter" "label=Filter" ) Generated HTML: Struts 2.3 <tr> <td class= "tdLabel" ><label for = "do_filter" class= "label" >Filter:</label></td> <td align= "center" ><input type= "text" name= "filter" value= "" id=" do_filter " align=" center"></td> </tr> Struts 2.5 <tr> <td class= "tdLabel" ><label for = "do_filter" class= "label" >Filter:</label></td> <td><input type= "text" name= "filter" value= "" id=" do_filter " align=" center"></td> </tr>
          Hide
          victorsosa victorsosa added a comment -

          OK, I can workout a CSS fix to apply the align="center" in the second TD.

          Show
          victorsosa victorsosa added a comment - OK, I can workout a CSS fix to apply the align="center" in the second TD.
          Hide
          victorsosa victorsosa added a comment - - edited

          I agree on not generate inline css.

          And we should assign a default class to this component; So If any applications want style on their own by just overwritten the classes.

          Show
          victorsosa victorsosa added a comment - - edited I agree on not generate inline css. And we should assign a default class to this component; So If any applications want style on their own by just overwritten the classes.
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          This is a regression, it must be fixed without introducing any additional dependencies. Such change with additional CSS class can be introduced in 2.5

          Show
          lukaszlenart Lukasz Lenart added a comment - This is a regression, it must be fixed without introducing any additional dependencies. Such change with additional CSS class can be introduced in 2.5
          Hide
          victorsosa victorsosa added a comment -

          So, as Lukasz Lenart said; to keep the old style but at the same time using the CSS classes.
          You just keep using

          #stextfield("label=Foo" "align=center")
              <td 
                  <#if parameters.align??>
                      class="tdAlign${parameters.align?html}"
                  </#if>
          
               .tdAligncenter{text-align:center;}
          

          This code will produce:
          <td class="tdAligncenter">

          Show
          victorsosa victorsosa added a comment - So, as Lukasz Lenart said; to keep the old style but at the same time using the CSS classes. You just keep using #stextfield( "label=Foo" "align=center" ) <td <# if parameters.align??> class= "tdAlign${parameters.align?html}" </# if > .tdAligncenter{text-align:center;} This code will produce: <td class="tdAligncenter">
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user victorsosa opened a pull request:

          https://github.com/apache/struts/pull/95

          WW-4634 Centre alignment doesn't seem to work in Velocity tags

          Fix for WW-4634

          Centre alignment doesn't seem to work in Velocity tags

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/victorsosa/struts WW-4634

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/struts/pull/95.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #95


          commit da8b13a87ef0d6af54a8cabbc62d54fbca67f683
          Author: victor sosa <victorsosa@users.noreply.github.com>
          Date: 2016-03-03T18:16:11Z

          Merge pull request #8 from apache/master

          update pull

          commit a5d716393999110b3701563479a96e83136a8b97
          Author: victor sosa <victorsosa@users.noreply.github.com>
          Date: 2016-03-03T18:41:11Z

          Merge pull request #9 from victorsosa/easymock_update

          Easymock update

          commit 6bb1ec67ad539d4bf6850f8314ef521e06b8295e
          Author: victor sosa <victorsosa@users.noreply.github.com>
          Date: 2016-03-04T22:07:16Z

          Merge pull request #10 from apache/master

          pull update

          commit 379b75292e6d084dc881e71f0a826c45934d9df8
          Author: victor sosa <victorsosa@users.noreply.github.com>
          Date: 2016-03-27T18:43:40Z

          Merge pull request #11 from apache/master

          update pull

          commit 6b360108e57b8986c6409b26cba9b3cd4f16a603
          Author: victorsosa <victor.sosa@peopleware.do>
          Date: 2016-05-17T12:37:13Z

          WW-4634


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user victorsosa opened a pull request: https://github.com/apache/struts/pull/95 WW-4634 Centre alignment doesn't seem to work in Velocity tags Fix for WW-4634 Centre alignment doesn't seem to work in Velocity tags You can merge this pull request into a Git repository by running: $ git pull https://github.com/victorsosa/struts WW-4634 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/struts/pull/95.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #95 commit da8b13a87ef0d6af54a8cabbc62d54fbca67f683 Author: victor sosa <victorsosa@users.noreply.github.com> Date: 2016-03-03T18:16:11Z Merge pull request #8 from apache/master update pull commit a5d716393999110b3701563479a96e83136a8b97 Author: victor sosa <victorsosa@users.noreply.github.com> Date: 2016-03-03T18:41:11Z Merge pull request #9 from victorsosa/easymock_update Easymock update commit 6bb1ec67ad539d4bf6850f8314ef521e06b8295e Author: victor sosa <victorsosa@users.noreply.github.com> Date: 2016-03-04T22:07:16Z Merge pull request #10 from apache/master pull update commit 379b75292e6d084dc881e71f0a826c45934d9df8 Author: victor sosa <victorsosa@users.noreply.github.com> Date: 2016-03-27T18:43:40Z Merge pull request #11 from apache/master update pull commit 6b360108e57b8986c6409b26cba9b3cd4f16a603 Author: victorsosa <victor.sosa@peopleware.do> Date: 2016-05-17T12:37:13Z WW-4634
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lukaszlenart commented on a diff in the pull request:

          https://github.com/apache/struts/pull/95#discussion_r63514815

          — Diff: core/src/main/resources/template/xhtml/controlheader.ftl —
          @@ -21,5 +21,8 @@
          */
          -->
          <#include "/$

          {parameters.templateDir}

          /$

          {parameters.expandTheme}

          /controlheader-core.ftl" />

          • <td
            + <td
            + <#if parameters.align??>
            + class="tdAlign$ {parameters.align?html}

            "

              • End diff –

          Can you use `-` to separate prefix and suffix?

          Show
          githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on a diff in the pull request: https://github.com/apache/struts/pull/95#discussion_r63514815 — Diff: core/src/main/resources/template/xhtml/controlheader.ftl — @@ -21,5 +21,8 @@ */ --> <#include "/$ {parameters.templateDir} /$ {parameters.expandTheme} /controlheader-core.ftl" /> <td + <td + <#if parameters.align??> + class="tdAlign$ {parameters.align?html} " End diff – Can you use `-` to separate prefix and suffix?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lukaszlenart commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-219706843

          This doesn't fix the main issue, right?

          Show
          githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-219706843 This doesn't fix the main issue, right?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user victorsosa commented on a diff in the pull request:

          https://github.com/apache/struts/pull/95#discussion_r63515782

          — Diff: core/src/main/resources/template/xhtml/controlheader.ftl —
          @@ -21,5 +21,8 @@
          */
          -->
          <#include "/$

          {parameters.templateDir}

          /$

          {parameters.expandTheme}

          /controlheader-core.ftl" />

          • <td
            + <td
            + <#if parameters.align??>
            + class="tdAlign$ {parameters.align?html}"
            — End diff –

            You mean like this class="tdAlign-${parameters.align?html}

            " ?

          Show
          githubbot ASF GitHub Bot added a comment - Github user victorsosa commented on a diff in the pull request: https://github.com/apache/struts/pull/95#discussion_r63515782 — Diff: core/src/main/resources/template/xhtml/controlheader.ftl — @@ -21,5 +21,8 @@ */ --> <#include "/$ {parameters.templateDir} /$ {parameters.expandTheme} /controlheader-core.ftl" /> <td + <td + <#if parameters.align??> + class="tdAlign$ {parameters.align?html}" — End diff – You mean like this class="tdAlign-${parameters.align?html} " ?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lukaszlenart commented on a diff in the pull request:

          https://github.com/apache/struts/pull/95#discussion_r63516015

          — Diff: core/src/main/resources/template/xhtml/controlheader.ftl —
          @@ -21,5 +21,8 @@
          */
          -->
          <#include "/$

          {parameters.templateDir}

          /$

          {parameters.expandTheme}

          /controlheader-core.ftl" />

          • <td
            + <td
            + <#if parameters.align??>
            + class="tdAlign$ {parameters.align?html}

            "

              • End diff –

          yes

          Show
          githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on a diff in the pull request: https://github.com/apache/struts/pull/95#discussion_r63516015 — Diff: core/src/main/resources/template/xhtml/controlheader.ftl — @@ -21,5 +21,8 @@ */ --> <#include "/$ {parameters.templateDir} /$ {parameters.expandTheme} /controlheader-core.ftl" /> <td + <td + <#if parameters.align??> + class="tdAlign$ {parameters.align?html} " End diff – yes
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user victorsosa commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-219709110

          Yes, it fix the main issue and keep the back compatibility.

          Show
          githubbot ASF GitHub Bot added a comment - Github user victorsosa commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-219709110 Yes, it fix the main issue and keep the back compatibility.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user victorsosa commented on a diff in the pull request:

          https://github.com/apache/struts/pull/95#discussion_r63517163

          — Diff: core/src/main/resources/template/xhtml/controlheader.ftl —
          @@ -21,5 +21,8 @@
          */
          -->
          <#include "/$

          {parameters.templateDir}

          /$

          {parameters.expandTheme}

          /controlheader-core.ftl" />

          • <td
            + <td
            + <#if parameters.align??>
            + class="tdAlign$ {parameters.align?html}

            "

              • End diff –

          Done

          Show
          githubbot ASF GitHub Bot added a comment - Github user victorsosa commented on a diff in the pull request: https://github.com/apache/struts/pull/95#discussion_r63517163 — Diff: core/src/main/resources/template/xhtml/controlheader.ftl — @@ -21,5 +21,8 @@ */ --> <#include "/$ {parameters.templateDir} /$ {parameters.expandTheme} /controlheader-core.ftl" /> <td + <td + <#if parameters.align??> + class="tdAlign$ {parameters.align?html} " End diff – Done
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user victorsosa commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-219713305

          Another thing I will change the name to just

          > align-center

          because this css class can be used in other places that are using the align attribute; which is s Not supported in HTML5.

          Show
          githubbot ASF GitHub Bot added a comment - Github user victorsosa commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-219713305 Another thing I will change the name to just > align-center because this css class can be used in other places that are using the align attribute; which is s Not supported in HTML5.
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          Jon Juaristi exactly what 2.3 version you have used?

          Show
          lukaszlenart Lukasz Lenart added a comment - Jon Juaristi exactly what 2.3 version you have used?
          Hide
          jontxu Jon Juaristi added a comment -

          2.3.28.1

          Show
          jontxu Jon Juaristi added a comment - 2.3.28.1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user aleksandr-m commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-219809968

          Why not just add some CSS class to this table cell? Then end-users can style it as they want and align parameter will not be needed.

          Show
          githubbot ASF GitHub Bot added a comment - Github user aleksandr-m commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-219809968 Why not just add some CSS class to this table cell? Then end-users can style it as they want and align parameter will not be needed.
          Hide
          aleksandr-m Aleksandr Mashchenko added a comment -

          Velocity tags? Are we still supporting them?

          Show
          aleksandr-m Aleksandr Mashchenko added a comment - Velocity tags? Are we still supporting them?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user victorsosa commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-219813310

          CSS class to this table cell is what we have now and to have back compatibility as @lukaszlenart said.

          Show
          githubbot ASF GitHub Bot added a comment - Github user victorsosa commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-219813310 CSS class to this table cell is what we have now and to have back compatibility as @lukaszlenart said.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lukaszlenart commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-219817155

          I'm just wondering what was the main reason, I have prepared a unit test and it didn't pass when using 2.3.29-SNAPSHOT which means 2.3.28.1 is broken the same way.

          Show
          githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-219817155 I'm just wondering what was the main reason, I have prepared a unit test and it didn't pass when using 2.3.29-SNAPSHOT which means 2.3.28.1 is broken the same way.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user victorsosa commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-219820007

          @lukaszlenart the parameter align was remove on this commit
          https://github.com/victorsosa/struts/commit/a0b34806b0f700a1fd240b698c4cec474711bbd0

          Show
          githubbot ASF GitHub Bot added a comment - Github user victorsosa commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-219820007 @lukaszlenart the parameter align was remove on this commit https://github.com/victorsosa/struts/commit/a0b34806b0f700a1fd240b698c4cec474711bbd0
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user aleksandr-m commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-219820070

          Issue is about Velocity tags, not FreeMarker. Are we still supporting Velocity tags?

          Show
          githubbot ASF GitHub Bot added a comment - Github user aleksandr-m commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-219820070 Issue is about Velocity tags, not FreeMarker. Are we still supporting Velocity tags?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lukaszlenart commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-219820654

          So it's not a regression bug Yes, we do support Velocity.

          Show
          githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-219820654 So it's not a regression bug Yes, we do support Velocity.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user aleksandr-m commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-219823160

          How the Velocity templating is working? The only Velocity templates I see are in the [archive](https://github.com/apache/struts/tree/master/core/src/main/resources/template/archive).

          Show
          githubbot ASF GitHub Bot added a comment - Github user aleksandr-m commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-219823160 How the Velocity templating is working? The only Velocity templates I see are in the [archive] ( https://github.com/apache/struts/tree/master/core/src/main/resources/template/archive ).
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user aleksandr-m commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-219824169

          Yep, the aforementioned commit misses css class for that cell in the `controlheader.ftl`. It should be the same way as for the buttons.

          Show
          githubbot ASF GitHub Bot added a comment - Github user aleksandr-m commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-219824169 Yep, the aforementioned commit misses css class for that cell in the `controlheader.ftl`. It should be the same way as for the buttons.
          Hide
          githubbot ASF GitHub Bot added a comment -
          Show
          githubbot ASF GitHub Bot added a comment - Github user victorsosa commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-219826190 OH ok, got you https://github.com/apache/struts/tree/master/core/src/main/resources/template/archive I miss that.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user victorsosa commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-219826548

          the css class for that cell is in the controlheader.ftl already.

          Show
          githubbot ASF GitHub Bot added a comment - Github user victorsosa commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-219826548 the css class for that cell is in the controlheader.ftl already.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lukaszlenart commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-219829308

          All the tags are implemented in FreeMarker, those archive templates are deprecated and should be thrown away.

          Show
          githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-219829308 All the tags are implemented in FreeMarker, those archive templates are deprecated and should be thrown away.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user aleksandr-m commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-219829978

          @lukaszlenart Ahh ok. Thanks for clarifying that.

          Show
          githubbot ASF GitHub Bot added a comment - Github user aleksandr-m commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-219829978 @lukaszlenart Ahh ok. Thanks for clarifying that.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user aleksandr-m commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-220522532

          @victorsosa I mean something like `<td class="tdInput">` in controlheader.ftl and `.tdInput

          {text-align:left;}

          ` in styles.css. <-- This is for 2.5.

          And if alignment doesn't work in 2.3 and your solution fixes it, then this PR should target `support-2-3` branch.

          Show
          githubbot ASF GitHub Bot added a comment - Github user aleksandr-m commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-220522532 @victorsosa I mean something like `<td class="tdInput">` in controlheader.ftl and `.tdInput {text-align:left;} ` in styles.css. <-- This is for 2.5. And if alignment doesn't work in 2.3 and your solution fixes it, then this PR should target `support-2-3` branch.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user victorsosa commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-220740769

          It will break back compatibility as @lukaszlenart said.

          Show
          githubbot ASF GitHub Bot added a comment - Github user victorsosa commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-220740769 It will break back compatibility as @lukaszlenart said.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user aleksandr-m commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-220795005

          @victorsosa You see, this commit already broke backward compatibility, but it was incomplete in the way it didn't introduce css class for the input cell.
          It is 2.5 so some breaking changes are expected.

          Show
          githubbot ASF GitHub Bot added a comment - Github user aleksandr-m commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-220795005 @victorsosa You see, this commit already broke backward compatibility, but it was incomplete in the way it didn't introduce css class for the input cell. It is 2.5 so some breaking changes are expected.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lukaszlenart commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-220817379

          @victorsosa this commit a0b34806b0f700a1fd240b698c4cec474711bbd0 dropped `align` attribute for buttons, it shouldn't affect `textfield` tag. Anyway, I think we should merge this PR and if needed prepare another one to target 2.3

          Show
          githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-220817379 @victorsosa this commit a0b34806b0f700a1fd240b698c4cec474711bbd0 dropped `align` attribute for buttons, it shouldn't affect `textfield` tag. Anyway, I think we should merge this PR and if needed prepare another one to target 2.3
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user victorsosa commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-220818804

          @lukaszlenart ok

          Show
          githubbot ASF GitHub Bot added a comment - Github user victorsosa commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-220818804 @lukaszlenart ok
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user victorsosa opened a pull request:

          https://github.com/apache/struts/pull/96

          WW-4634

          Cherry-pick PR for support-2-3

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/victorsosa/struts support-2-3

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/struts/pull/96.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #96


          commit 73ccb56cd8b1adf6625fe25e611fd0d365a69c1b
          Author: victorsosa <victor.sosa@peopleware.do>
          Date: 2016-05-17T13:00:33Z

          WW-4634


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user victorsosa opened a pull request: https://github.com/apache/struts/pull/96 WW-4634 Cherry-pick PR for support-2-3 You can merge this pull request into a Git repository by running: $ git pull https://github.com/victorsosa/struts support-2-3 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/struts/pull/96.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #96 commit 73ccb56cd8b1adf6625fe25e611fd0d365a69c1b Author: victorsosa <victor.sosa@peopleware.do> Date: 2016-05-17T13:00:33Z WW-4634
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lukaszlenart commented on the pull request:

          https://github.com/apache/struts/pull/96#issuecomment-220900444

          As far I understand the issue was in 2.5 only, 2.3 works as expected.

          Show
          githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on the pull request: https://github.com/apache/struts/pull/96#issuecomment-220900444 As far I understand the issue was in 2.5 only, 2.3 works as expected.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user victorsosa commented on the pull request:

          https://github.com/apache/struts/pull/96#issuecomment-220947364

          But it is using the parameter align instead of a css class. the align parameter is deprecated in html 5.

          Show
          githubbot ASF GitHub Bot added a comment - Github user victorsosa commented on the pull request: https://github.com/apache/struts/pull/96#issuecomment-220947364 But it is using the parameter align instead of a css class. the align parameter is deprecated in html 5.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lukaszlenart commented on the pull request:

          https://github.com/apache/struts/pull/96#issuecomment-220948751

          That's why people should migrate to 2.5 I think we can ask reported if he wants the same in 2.3

          Show
          githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on the pull request: https://github.com/apache/struts/pull/96#issuecomment-220948751 That's why people should migrate to 2.5 I think we can ask reported if he wants the same in 2.3
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user victorsosa commented on the pull request:

          https://github.com/apache/struts/pull/96#issuecomment-221015218

          OK

          Show
          githubbot ASF GitHub Bot added a comment - Github user victorsosa commented on the pull request: https://github.com/apache/struts/pull/96#issuecomment-221015218 OK
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user aleksandr-m commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-221056549

          I would prefer to follow html5 and drop align attribute completely from S2 tags, BUT @victorsosa can you at least add some css class to input cell along with your solution.
          Something like:
          ```
          if align then class="tdInput align-$

          {parameters.align?html}

          " else class="tdInput"
          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user aleksandr-m commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-221056549 I would prefer to follow html5 and drop align attribute completely from S2 tags, BUT @victorsosa can you at least add some css class to input cell along with your solution. Something like: ``` if align then class="tdInput align-$ {parameters.align?html} " else class="tdInput" ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user victorsosa commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-221314653

          @aleksandr-m You mean something like <td class="tdInput"> in controlheader.ftl and .tdInput

          {text-align:left;}

          in styles.css. <-- This is for 2.5.

          THat means *drop align attribute completely* from S2 tags; So from now on the only way to set the align to center for example is to overwrite the css class tdInput in 2.5, right?

          I will change to that code.

          Show
          githubbot ASF GitHub Bot added a comment - Github user victorsosa commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-221314653 @aleksandr-m You mean something like <td class="tdInput"> in controlheader.ftl and .tdInput {text-align:left;} in styles.css. <-- This is for 2.5. THat means * drop align attribute completely * from S2 tags; So from now on the only way to set the align to center for example is to overwrite the css class tdInput in 2.5, right? I will change to that code.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user aleksandr-m commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-221326208

          @victorsosa IMO it would be great. BUT if we really really want it to be backward compatible then we can do both. Add a class for future *and* use align attribute with `class="align-$

          {parameters.align?html}

          "` like you propose in this PR.

          Show
          githubbot ASF GitHub Bot added a comment - Github user aleksandr-m commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-221326208 @victorsosa IMO it would be great. BUT if we really really want it to be backward compatible then we can do both. Add a class for future * and * use align attribute with `class="align-$ {parameters.align?html} "` like you propose in this PR.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user cnenning commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-221490265

          :+1:

          Show
          githubbot ASF GitHub Bot added a comment - Github user cnenning commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-221490265 :+1:
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lukaszlenart commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-221556014

          Tests are failing
          ```
          Error Message

          expected:<...bel:</label></td><td[]><labelid="myname"ti...> but was:<...bel:</label></td><td[class="tdInput"]><labelid="myname"ti...>
          Stacktrace

          junit.framework.ComparisonFailure: expected:<...bel:</label></td><td[]><labelid="myname"ti...> but was:<...bel:</label></td><td[class="tdInput"]><labelid="myname"ti...>
          at junit.framework.Assert.assertEquals(Assert.java:100)
          at junit.framework.Assert.assertEquals(Assert.java:107)
          at junit.framework.TestCase.assertEquals(TestCase.java:269)
          at org.apache.struts2.views.jsp.AbstractUITagTest.verify(AbstractUITagTest.java:246)
          at org.apache.struts2.views.jsp.ui.LabelTest.testSimple(LabelTest.java:49)
          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-221556014 Tests are failing ``` Error Message expected:<...bel:</label></td><td[]><labelid="myname"ti...> but was:<...bel:</label></td><td [class="tdInput"] ><labelid="myname"ti...> Stacktrace junit.framework.ComparisonFailure: expected:<...bel:</label></td><td[]><labelid="myname"ti...> but was:<...bel:</label></td><td [class="tdInput"] ><labelid="myname"ti...> at junit.framework.Assert.assertEquals(Assert.java:100) at junit.framework.Assert.assertEquals(Assert.java:107) at junit.framework.TestCase.assertEquals(TestCase.java:269) at org.apache.struts2.views.jsp.AbstractUITagTest.verify(AbstractUITagTest.java:246) at org.apache.struts2.views.jsp.ui.LabelTest.testSimple(LabelTest.java:49) ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lukaszlenart commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-221556310

          103 tests need to be updated, I think it isn't worth

          Show
          githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-221556310 103 tests need to be updated, I think it isn't worth
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user victorsosa commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-221567053

          I will fix the 103 test cases, give me some time

          Show
          githubbot ASF GitHub Bot added a comment - Github user victorsosa commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-221567053 I will fix the 103 test cases, give me some time
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user victorsosa commented on the pull request:

          https://github.com/apache/struts/pull/96#issuecomment-221600945

          PR remove, it wasn't requested

          Show
          githubbot ASF GitHub Bot added a comment - Github user victorsosa commented on the pull request: https://github.com/apache/struts/pull/96#issuecomment-221600945 PR remove, it wasn't requested
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user victorsosa closed the pull request at:

          https://github.com/apache/struts/pull/96

          Show
          githubbot ASF GitHub Bot added a comment - Github user victorsosa closed the pull request at: https://github.com/apache/struts/pull/96
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lukaszlenart commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-221621914

          @victorsosa osm!

          Show
          githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-221621914 @victorsosa osm!
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lukaszlenart commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-221785224

          I assume we're ok with this and this PR can be merged?

          Show
          githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-221785224 I assume we're ok with this and this PR can be merged?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user victorsosa commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-221804220

          Yes, I am ok

          Show
          githubbot ASF GitHub Bot added a comment - Github user victorsosa commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-221804220 Yes, I am ok
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user aleksandr-m commented on the pull request:

          https://github.com/apache/struts/pull/95#issuecomment-221976394

          Looks fine. Good job.

          Show
          githubbot ASF GitHub Bot added a comment - Github user aleksandr-m commented on the pull request: https://github.com/apache/struts/pull/95#issuecomment-221976394 Looks fine. Good job.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit d06c71d68e5c10990386e8904ca60927ed209250 in struts's branch refs/heads/master from victorsosa
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=d06c71d ]

          WW-4634

          Show
          jira-bot ASF subversion and git services added a comment - Commit d06c71d68e5c10990386e8904ca60927ed209250 in struts's branch refs/heads/master from victorsosa [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=d06c71d ] WW-4634
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 9e11b2c2b1962d3bcaa1c2744ecda56f42e21b77 in struts's branch refs/heads/master from victorsosa
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=9e11b2c ]

          WW-4634

          Show
          jira-bot ASF subversion and git services added a comment - Commit 9e11b2c2b1962d3bcaa1c2744ecda56f42e21b77 in struts's branch refs/heads/master from victorsosa [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=9e11b2c ] WW-4634
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 30b74a3f01379f095f83431a26d6d232a7b7f061 in struts's branch refs/heads/master from victorsosa
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=30b74a3 ]

          Merge branch 'WW-4634' of github.com:victorsosa/struts into WW-4634

          Show
          jira-bot ASF subversion and git services added a comment - Commit 30b74a3f01379f095f83431a26d6d232a7b7f061 in struts's branch refs/heads/master from victorsosa [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=30b74a3 ] Merge branch ' WW-4634 ' of github.com:victorsosa/struts into WW-4634
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 30b74a3f01379f095f83431a26d6d232a7b7f061 in struts's branch refs/heads/master from victorsosa
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=30b74a3 ]

          Merge branch 'WW-4634' of github.com:victorsosa/struts into WW-4634

          Show
          jira-bot ASF subversion and git services added a comment - Commit 30b74a3f01379f095f83431a26d6d232a7b7f061 in struts's branch refs/heads/master from victorsosa [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=30b74a3 ] Merge branch ' WW-4634 ' of github.com:victorsosa/struts into WW-4634
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 08360cb91697a0dd434c43a345822563c9f5d88f in struts's branch refs/heads/master from Lukasz Lenart
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=08360cb ]

          WW-4634 Fixes centre alignment in Velocity tags

          Show
          jira-bot ASF subversion and git services added a comment - Commit 08360cb91697a0dd434c43a345822563c9f5d88f in struts's branch refs/heads/master from Lukasz Lenart [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=08360cb ] WW-4634 Fixes centre alignment in Velocity tags
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/struts/pull/95

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/struts/pull/95
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          PR merged, thanks!

          Show
          lukaszlenart Lukasz Lenart added a comment - PR merged, thanks!
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Struts-JDK7-master #480 (See https://builds.apache.org/job/Struts-JDK7-master/480/)
          WW-4634 (victorsosa: rev 6b360108e57b8986c6409b26cba9b3cd4f16a603)

          • core/src/main/resources/template/xhtml/controlheader.ftl
          • core/src/main/resources/template/xhtml/styles.css
            WW-4634 (victorsosa: rev ffdacf1b60b8bf1df54af004a5fd84752eb522b4)
          • core/src/main/resources/template/xhtml/controlheader.ftl
          • core/src/main/resources/template/xhtml/styles.css
            WW-4634 (victorsosa: rev d06c71d68e5c10990386e8904ca60927ed209250)
          • core/src/main/resources/template/xhtml/styles.css
          • core/src/main/resources/template/xhtml/controlheader.ftl
            WW-4634 (victorsosa: rev 9e11b2c2b1962d3bcaa1c2744ecda56f42e21b77)
          • core/src/main/resources/template/xhtml/controlheader.ftl
          • core/src/main/resources/template/xhtml/styles.css
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Struts-JDK7-master #480 (See https://builds.apache.org/job/Struts-JDK7-master/480/ ) WW-4634 (victorsosa: rev 6b360108e57b8986c6409b26cba9b3cd4f16a603) core/src/main/resources/template/xhtml/controlheader.ftl core/src/main/resources/template/xhtml/styles.css WW-4634 (victorsosa: rev ffdacf1b60b8bf1df54af004a5fd84752eb522b4) core/src/main/resources/template/xhtml/controlheader.ftl core/src/main/resources/template/xhtml/styles.css WW-4634 (victorsosa: rev d06c71d68e5c10990386e8904ca60927ed209250) core/src/main/resources/template/xhtml/styles.css core/src/main/resources/template/xhtml/controlheader.ftl WW-4634 (victorsosa: rev 9e11b2c2b1962d3bcaa1c2744ecda56f42e21b77) core/src/main/resources/template/xhtml/controlheader.ftl core/src/main/resources/template/xhtml/styles.css

            People

            • Assignee:
              lukaszlenart Lukasz Lenart
              Reporter:
              jontxu Jon Juaristi
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development