Diff module breaks layout for fixed width themes. If a line is too long, it will expand the comparison area. Wikipedia has a solution to this problem, which I've ported to Diff in the attached patch.

CommentFileSizeAuthor
#3 diff_layout_fix.patch3.28 KBmoonray
diff_layout_fix.patch3.75 KBmoonray

Comments

moonray’s picture

moonray commented
Project: Revision Moderation » Diff

(That's supposed to be under Diff, not Revision Moderation)

dww’s picture

dww
we/he/they
commented
Status: Needs review » Needs work

Without looking closely, this seems to introduce an XSS bug:

-      $output .= '<tr><td colspan="4" class="diff-section-title">'. t('Changes to %name', array('%name' => $node_diff['name'])) .'</td></tr>';
+      $output .= '<tr><td colspan="4" class="diff-section-title">'. t('Changes to !name', array('!name' => $node_diff['name'])) .'</td></tr>';

Why is this change in the patch? Are we sure node_diff['name'] is getting properly escaped somewhere else?

Thanks.

moonray’s picture

moonray commented
Status: Needs work » Needs review
StatusFileSize
new diff_layout_fix.patch3.28 KB

Wow, that sort of snuck in there while I was playing around with the code. New patch has those changes removed.

dww’s picture

dww
we/he/they
commented

Yes, that looks much better, thanks. ;) I'll try it out in the next few days if possible, and commit.

dww’s picture

dww
we/he/they
commented
Status: Needs review » Fixed

I re-reviewed this, tested it to reproduce the bug and confirmed the patch solves it, and committed a slightly modified version to HEAD, DRUPAL-5 and DRUPAL-4-7. Basically, I really didn't like how some of the diff.css changes looked when rendered on the scree, so I removed this:

table.diff td.diff-deletedline, table.diff td.diff-addedline, table.diff td.diff-context {
  font-family: courier, serif;
  padding: 0;
}

And the font-size: 79%; for table.diff td. The primary functionality still works, but the diff is no longer displayed in ugly, small courier. ;)

Thanks!
-Derek

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.