As title states, JS-added "Read More" link on a status breaks HTML in the post if it's at the character cutoff. Example and patch attached.

Comments

icecreamyou’s picture

Project: Facebook-style Statuses (Microblog) » Statuses (Social Microblog)
Version: 6.x-3.x-dev » 7.x-1.x-dev
Component: Submodules » Code (API)

Thanks for the patch -- unfortunately the only reason why it works is because browsers are pretty good at inferring the expected behavior of broken HTML. I've committed it to FBSS 6.x and Statuses 7.x, but a more comprehensive solution is needed. Specifically, we need to make sure that we don't end up with overlapping HTML tags, like this:

<a href="http://example.com/">some text... Read more<span class="facebook-status-readmore">rest of the message (the long part that is hidden)</a></span>

This is a difficult problem and not one for which I am able to write a solution without learning something new.

rabisg’s picture

While trying to patch this, came across a additional problem.
Instead of truncating the text to the size mentioned in Configuration/Statuses/Advanced it truncates the HTML text to that size.
Attaching patch for that too.

Specifically, we need to make sure that we don't end up with overlapping HTML tags

This should fix it too.

icecreamyou’s picture

Status: Active » Needs work
+++ b/resources/statuses.js
@@ -70,7 +67,7 @@ attach: function (context) {
-    ctxt.find('.statuses-content').each(fbss_truncate);
+    ctxt.find('.statuses-content p').each(fbss_truncate);

Not sure why you made this change but it will break if there is no input filter being used or there are multiple paragraphs in a status or there is other markup like tables. The truncating should run on the entire status, not per-paragraph.

+++ b/resources/statuses.js
@@ -216,4 +213,40 @@ function fbss_refresh() {
+//Function that returns substring ignoring HTML tags
+//Source: http://jsfiddle.net/danmana/5mNNU/

Couple of things:

  • Only GPLv2+ code can be committed to d.o, so in order to commit this function we would have to get explicit permission from the author to have it licensed under GPLv2. Additionally third-party code is normally not permitted, although in this case it would be maintained as a fork so it wouldn't matter.
  • This function fails in a number of cases; a search for truncate HTML comes up with some better algorithms.
  • Drupal already has a function that truncates HTML correctly -- text_summary() -- so we should just use that. This will require changing the HTML that Statuses outputs so that we have the truncated version in a hidden div. This should be done in _statuses_run_filter() in statuses.module.

Looking back on it now I am not sure why the "readmore" stuff is even in there, it doesn't seem to be used for anything. Probably cruft. So it's fine to remove that.

rabisg’s picture

The truncating should run on the entire status, not per-paragraph.


Sorry my bad, forgot to remove that before creating the patch.

Also I can write the desired function(truncate HTML) on my own. An easy way would be to just pull out a substring, append readmore link and expect Jquery to close the unclosed tags(which it does gracefully). But since I read it somewhere that reusing code is a good idea, I decided to use it.
Thanks for the tip on licenses. I'll take care of it next time. Do you want me to submit a modified patch?

icecreamyou’s picture

Yes, what I'd like to see is a patch that uses Drupal's built-in text_summary() to do this instead of writing a new function or using someone else's, as suggested in #3 above. You're right that generally reusing code is a good idea, but better to reuse code that we already have. :-)

rabisg’s picture

Modified _statuses_run_filter() and statuses.js

icecreamyou’s picture

+++ b/resources/statuses.js
@@ -52,26 +52,16 @@ attach: function (context) {
     var oldMsgText = th.html();

this line can be removed too I think

+++ b/resources/statuses.js
@@ -52,26 +52,16 @@ attach: function (context) {
+    });    ¶
+  }
+  ¶
+  ctxt.find('.statuses-content').each(fbss_truncate);
+  ¶

trailing spaces

+++ b/statuses.module
@@ -1029,10 +1029,21 @@ function _statuses_element_sort($a, $b) {
-  if (variable_get('statuses_filter', 'none') != 'none') {
-    return check_markup($status, variable_get('statuses_filter', 'none'));
+  $filter = variable_get('statuses_filter', NULL); ¶

trailing space; also the default value of this variable is 'none' not NULL so it needs the 'if' statement check

+++ b/statuses.module
@@ -1029,10 +1029,21 @@ function _statuses_element_sort($a, $b) {
+    $text = '<span class="statuses-read-more-toggle">' .
+            $truncated_text .
+            '<span class="statuses-read-more-link">&hellip;&nbsp;<a class="statuses-readmore-toggle active">' .
+            t('Read more') .
+            '</a></span></span><span class="statuses-full-text">' .
+            $text . '</span>';

Indented too far... just indent 2 spaces from $text. Also since we're doing this in PHP instead of JS now, let's use l() to create the link instead of manually typing out <a href="#" class="....

These are very minor issues, overall the patch looks good. Just fix these syntax things and it's RTBC.

mathankumarc’s picture

Status: Needs work » Needs review

Can we go ahead and commit this patch and close the issue?

icecreamyou’s picture

Status: Needs review » Needs work

If the issues from #7 are fixed this is RTBC from me. It will also need a backport although I believe the changes should be identical.

icecreamyou’s picture

Status: Needs work » Needs review
StatusFileSize
new2.57 KB

Updated but untested patch attached. Should be ready, but will also need a backport.

icecreamyou’s picture

Status: Needs review » Fixed

Committed #10 to dev with a slight change, after testing.

icecreamyou’s picture

Status: Fixed » Patch (to be ported)

Whoops, forgot this needs a backport.

mathankumarc’s picture

Can we move this issue back to FBSS issue queue?

icecreamyou’s picture

Project: Statuses (Social Microblog) » Facebook-style Statuses (Microblog)
Version: 7.x-1.x-dev » 6.x-3.x-dev
Component: Code (API) » User interface