Item13964: CommentPlugin removal of the location feature creates massive compatibility problem

-- KennethLavrsen - 17 Feb 2016

Example topic that fails

First I added a comment (custom template that just adds a row)

Then an action. Action is supposed to go above the location text. It ends above the CRstatus COMMENT

Then another comment using the CRStatus.

See how the ACTIONS gets convoluted into the comments. Not pretty

---++ Action and Comment

---++ Status Updates

%EDITTABLE{format="|date,11,,%d %b %Y|text,30, %WIKINAME% |textarea,5x60|" quietsave="off" editbutton="Edit Status"}%
%TABLE{columnwidths="140,150,600"}%
| *Status Date* | *Name* | *Status* |
| 17 Feb 2016 | KennethLavrsen | comment |
%ACTION{ created="2016-02-17" creator="Main.KennethLavrsen" due="2016-12-24" state="open" uid="004408" who="Main.KennethLavrsen" }% Hello<br />- Created by Main.KennethLavrsen, 17 Feb 2016 - 18:03 %ENDACTION%
| 17 Feb 2016 | KennethLavrsen | status added after action |


%COMMENT{type="CRStatus"}%

---++ Actions

<!--Action-items-inserted-above-do-not-delete-this-->

%COMMENT{type="action" location="<\!--Action-items-inserted-above-do-not-delete-this--\>"}%

---

Example with "off the shelf Comment type"

---++ Action and Comment

---++ Status Updates



1 comment

-- Main.KennethLavrsen - 17 Feb 2016
%ACTION{ created="2016-02-17" creator="Main.KennethLavrsen" due="2016-12-24" state="open" uid="004409" who="Main.KennethLavrsen" }% 2 Action<br />- Created by Main.KennethLavrsen, 17 Feb 2016 - 18:08 %ENDACTION%


3 Comment

-- Main.KennethLavrsen - 17 Feb 2016
%COMMENT{type="above"}%

---++ Actions

<!--Action-items-inserted-above-do-not-delete-this-->

%COMMENT{type="action" location="<\!--Action-items-inserted-above-do-not-delete-this--\>"}%

---

Fix for the anchor issue: Plugin is not encoding value strings in the form, which breaks it when html characters are used.

diff --git a/CommentPlugin/lib/Foswiki/Plugins/CommentPlugin/Comment.pm b/CommentPlugin/lib/Foswiki/Plugins/CommentPlugin/Comment.pm
index f487c86..6b4a397 100644
--- a/CommentPlugin/lib/Foswiki/Plugins/CommentPlugin/Comment.pm
+++ b/CommentPlugin/lib/Foswiki/Plugins/CommentPlugin/Comment.pm
@@ -17,6 +17,7 @@ package Foswiki::Plugins::CommentPlugin::Comment;
 
 sub _hidden {
     my ( $name, $value ) = @_;
+    $value = Foswiki::Func::expandCommonVariables("\%ENCODE{\"$value\"}\%");
     return "<input type=\"hidden\" name=\"$name\" value=\"$value\" />";
 }
 

Except: This fix breaks a bunch of the unit tests. Not sure if it's bad tests or excessive encoding. On further inspection, ... Using type=entity for the ENCODE breaks fewer unit tests, and seems to fix the issue. the default URL encode is probably too much.

-- GeorgeClark - 17 Feb 2016

Can't use comment macro - the examples above break the page.

---

George, I think you are comparing apples with oranges. The problem is in the generated HTML page, where I see this:

<input type="hidden" name="comment_location" value="<!--Action-items-inserted-above-do-not-delete-this-->" /&gt;

However when I use a detailed debug to examine what the plugin is generating, I see this:

<input type="hidden" name="comment_location" value="<!--Action-items-inserted-above-do-not-delete-this-->" />

There's nothing wrong with what the plugin is generating, it's perfectly valid HTML. It looks to me like there's a problem in the core renderer, I suspect in the comment hoisting, which is resulting in that >

-- Main.CrawfordCurrie - 18 Feb 2016 - 08:49

For the record - even if the fix is in core - there is still an action to put the documentation back. There is no consensus or vote to remove this feature and we are many that find it very smart and have used it to create user interfaces in quite advanced applications. It is a documented feature and why would people not use it then?

-- Main.KennethLavrsen - 18 Feb 2016 - 10:28

I removed the feature documentation because there is no way I can make it work with AJAX comment submission. This is because the location can't be located in the rendered output. I left the code in situ in case someone actually was using it, and for the last couple of years no-one has raised a flag.

It's easy enough to put the documentation back, I guess, though it would have to be annotated to say it won't work with the AJAX interface.

I have pushed this down to Normal priority and opened Item13966 for the real (core) bug.

-- Main.CrawfordCurrie - 18 Feb 2016 - 12:18

OK, so plan of record.

There is a core bug to be fixed - fine.

Meanwhile - if we can ALSO fix the problem as suggested by George then I strongly suggest we do that so people can upgrade the plugin and fix a major problem.

With respect to documentation of location. I can write a small warning note that Ajax based COMMENT tags will not work with location and that you need to disable ajax in general - or for the COMMENT type you want to use. That is reasonable.

-- Main.KennethLavrsen - 18 Feb 2016 - 14:12