Bug #24301 for Text-Diff-HTML: XHTML doesn't allow div element inside pre element

On Wed Jan 10 11:52:18 2007, pete@void.printf.net wrote: Show quoted text
Good catch, thanks. I've changed that documentation to instead read: You will also likely want to wrap the output of your diff in its own element (a C<< <div> >> will do) styled with "white-space: pre". Show quoted text
I think that keeping things as they are and making a more intelligent CSS suggestion, as I've done above, is the way to go. Show quoted text
Can you send me a patch for that, and maybe an example of what it looks like? Thanks, David

On Mon, May 05, 2008 at 04:29:15PM -0400, David Wheeler via RT wrote: Show quoted text
I agree that this is a better solution, and have changed my instance to use this method. Show quoted text
Actually, on reflection, I can't really provide a clean patch. What I did was to subclass Text::Diff::HTML (patching would have led to portability issues), and I had to override the _header method from Text::Diff to add in the SUFFIX options (i.e. take FILENAME_SUFFIX_A and FILENAME_SUFFIX_B as per the PREFIX options and include them in the output before the newline if defined). This is a bit of a shame, because it's copying rather than reusing most of the code from that method, and it might be worth looking for a better solution --- I can't think of one other than patching Text::Diff itself, but maybe you can. Once I'd done that, it was just a question of overriding file_header with a version which did not encode the PREFIX and SUFFIX options, so that I could supply values like '<del>---', '</del>', '<ins>+++', and '</ins>', so that I marked up the file header lines the same way as I mark up the corresponding hunk lines. Sorry that this isn't as clean as I initially suggested... Regards, Pete.

On Thu, May 08, 2008 at 12:49:59PM -0400, David Wheeler via RT wrote: Show quoted text
Sorry, let me try to explain a bit better. * What I want to do: The normal behaviour of Text::Diff::HTML is to render the difference between the list of fruits in foofile and barfile as follows. <div class="file"><span class="fileheader">--- foofile [date] +++ barfile [date] </span><div class="hunk"><span class="hunkheader">@@ -1,7 +1,7 @@ </span><span class="ctx"> apple banana clementine </span><del>- date </del><ins>+ damson </ins><span class="ctx"> elderberry fig guava </span><span class="hunkfooter></span></div><span class="filefooter"></span></div> I want to be able to produce the following instead. <div class="file"><span class="fileheader"><del>--- foofile [date] </del><ins>+++ barfile [date] </ins></span><div class="hunk"><span class="hunkheader">@@ -1,7 +1,7 @@ </span><span class="ctx"> apple banana clementine </span><del>- date </del><ins>+ damson </ins><span class="ctx"> eggfruit fig guava </span><span class="hunkfooter></span></div><span class="filefooter"></span></div> Note the del and ins elements which appear around the filenames in the fileheader now. I wanted to do this because I use colour coding on these elements, and I wanted the filenames at the top of the diff to appear in the same colours (or rather different shades of the same hues) as the lines which appear only in that file. In the above case 'foofile' is coloured like 'date' and 'barfile' is coloured like 'damson'. I think (and my users agreed) that this improves readability of the diff. (It doesn't have to be del and ins, but there needs to be some markup to distinguish the two lines, which there isn't at present.) * How I currently do this: The '---' and '+++' prefixes were added by Text::Diff::_header from the FILENAME_PREFIX_A and _B options. To make my change, I effectively replace those values with '<del>---' and '<ins>+++', but (a) I also need to be able to supply a FILENAME_SUFFIX_A of '</del>' and a FILENAME_SUFFIX_B of '</ins>', so I had to override _header with a new implemenation which took those options and used them (if defined) in the correct places in its return value; and (b) I need to stop Text::Diff::HTML::file_header doing entity encoding on those values, because I don't want to escape the markup I'm providing, so I had to override that method too. * How I would do this in an ideal world: I think that Text::Diff could be patched to honour the SUFFIX options without breaking anything. That would mean I didn't need to do (a), which is good as (a) is poor code reuse. Avoiding (b) a little more complicated, I guess, because the current behaviour (that it should use '---' and '+++' prefixes by default, that these can be changed by user-supplied option, and that they should be escaped) didn't ought to change for backwards compatibility reasons. Either (i) making Text::Diff::HTML add the del and ins tags to the SUFFIX and PREFIX values, or (ii) making it not escape the user-supplied values, might break users' existing code. I guess it might be possible to provide a new option to turn on one or other of these behaviours, but that might be over-engineering... Regards, Pete. P.S. I appreciate that this is the wrong place to be requesting patches to Text::Diff. I only threw this in as an aside since I was mentioning the documentation error you've already fixed --- it isn't a big deal to me, and I don't want to take up too much of your time with it.

On May 8, 2008, at 10:58, Pete Gillin via RT wrote: Show quoted text
Okay, that's kind of what I thought you were saying, though I wasn't sure. Thanks for the detailed example. Show quoted text
Well, it'd be simpler to wrap them in spans instead of del and ins tags. That way the closing tag is always the same. Show quoted text
Okay, I'll have to look more closely at the code (it has been a while) and see what I can figure out. This is a reasonable feature request, though, as it adds semantic meaning to the output, which is always good (and easy to style). Show quoted text
It's okay with me for feature requests to be here. It makes it easier to keep track. ;-) Best, David