Skip Menu |
 

Preferred bug tracker

Please visit the preferred bug tracker to report your issue.

This queue is for tickets about the Text-Diff-HTML CPAN distribution.

Report information
The Basics
Id: 24301
Status: resolved
Priority: 0/
Queue: Text-Diff-HTML

People
Owner: Nobody in particular
Requestors: pete [...] void.printf.net
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: 0.06



Subject: XHTML doesn't allow div element inside pre element
Date: Wed, 10 Jan 2007 16:51:48 +0000
To: bug-text-diff-html [...] rt.cpan.org
From: Pete Gillin <pete [...] void.printf.net>
Download (untitled) / with headers
text/plain 1.4k
The POD for Text::Diff::HTML suggests that: 'You will also likely want to wrap the output of your diff in a "<pre>" element.' This is great advice... except that the output includes div elements and the XHTML spec (XHTML 1.0 Strict, at least) does not allow div elements within a pre element. So either it is a bug that the documentation suggests that you wrap in a pre element and can obtain valid XHTML by doing so, or it is a bug that the output produced includes divs. As a workaround (because you're right, the pre element is very useful), I have subclassed Text::Diff::HTML are reimplemented the (file|hunk)_(header|footer) methods to do the same thing but with spans instead of divs. I humbly suggest that this change should (possibly optionally, or possibly with an option to revert to div to maintain backwards compatibility where essential) be adopted in Text::Diff::HTML. Best wishes, Pete. P.S. While I was subclassing, I also reimplemented the _header method in my subclass to honour options FILENAME_SUFFIX_[AB] which, if defined, are joined onto the end of the lines of the file header. This made it easy for me to mark up and apply style to each line separately, specifically to have the +++ and --- lines in colours related to the + and - lines in the hunk, which I feel improves readability. If you wanted to consider this for a subsequent version, that would be great too. P.P.S. I can supply patches for either of the above if that would help.
Download (untitled) / with headers
text/plain 1.8k
On Wed Jan 10 11:52:18 2007, pete@void.printf.net wrote: Show quoted text
> The POD for Text::Diff::HTML suggests that: 'You will also likely want > to wrap the output of your diff in a "<pre>" element.' This is great > advice... except that the output includes div elements and the XHTML > spec (XHTML 1.0 Strict, at least) does not allow div elements within a > pre element. So either it is a bug that the documentation suggests > that you wrap in a pre element and can obtain valid XHTML by doing so, > or it is a bug that the output produced includes divs.
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
> As a workaround (because you're right, the pre element is very > useful), I have subclassed Text::Diff::HTML are reimplemented the > (file|hunk)_(header|footer) methods to do the same thing but with > spans instead of divs. I humbly suggest that this change should > (possibly optionally, or possibly with an option to revert to div to > maintain backwards compatibility where essential) be adopted in > Text::Diff::HTML.
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
> P.S. While I was subclassing, I also reimplemented the _header method > in my subclass to honour options FILENAME_SUFFIX_[AB] which, if > defined, are joined onto the end of the lines of the file header. This > made it easy for me to mark up and apply style to each line > separately, specifically to have the +++ and --- lines in colours > related to the + and - lines in the hunk, which I feel improves > readability. If you wanted to consider this for a subsequent version, > that would be great too.
Can you send me a patch for that, and maybe an example of what it looks like? Thanks, David
Subject: Re: [rt.cpan.org #24301] XHTML doesn't allow div element inside pre element
Date: Thu, 8 May 2008 14:12:03 +0100
To: David Wheeler via RT <bug-Text-Diff-HTML [...] rt.cpan.org>
From: Pete Gillin <pete [...] void.printf.net>
Download (untitled) / with headers
text/plain 1.6k
On Mon, May 05, 2008 at 04:29:15PM -0400, David Wheeler via RT 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".
I agree that this is a better solution, and have changed my instance to use this method. Show quoted text
> > P.S. While I was subclassing, I also reimplemented the _header method > > in my subclass to honour options FILENAME_SUFFIX_[AB] which, if > > defined, are joined onto the end of the lines of the file > > header...
> Can you send me a patch for that, and maybe an example of what it > looks like?
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.
Subject: Re: [rt.cpan.org #24301] XHTML doesn't allow div element inside pre element
Date: Thu, 8 May 2008 09:49:41 -0700
To: bug-Text-Diff-HTML [...] rt.cpan.org
From: "David E. Wheeler" <dwheeler [...] cpan.org>
Download (untitled) / with headers
text/plain 1.4k
On May 8, 2008, at 06:12, Pete Gillin via RT wrote: Show quoted text
> I agree that this is a better solution, and have changed my instance > to use this method.
Cool. I have this checked in to svn, too, if you want to grab it there. https://svn.kineticode.com/Text-Diff-HTML/trunk/ 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...
I'm still not really following what it is you're doing here. I mean, what is the outcome? How does the output change? Thanks, David
Subject: Re: [rt.cpan.org #24301] XHTML doesn't allow div element inside pre element
Date: Thu, 8 May 2008 18:57:33 +0100
To: David Wheeler via RT <bug-Text-Diff-HTML [...] rt.cpan.org>
From: Pete Gillin <pete [...] void.printf.net>
Download (untitled) / with headers
text/plain 3.5k
On Thu, May 08, 2008 at 12:49:59PM -0400, David Wheeler via RT wrote: Show quoted text
> I'm still not really following what it is you're doing here. I mean, > what is the outcome? How does the output change?
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.
Subject: Re: [rt.cpan.org #24301] XHTML doesn't allow div element inside pre element
Date: Thu, 8 May 2008 19:07:29 +0100
To: David Wheeler via RT <bug-Text-Diff-HTML [...] rt.cpan.org>
From: Pete Gillin <pete [...] void.printf.net>
Download (untitled) / with headers
text/plain 519b
Actually, I've just thought that there's another option to fix by (b) above: Text::Diff::HTML could arrange for the each filename to appear in a span element with an appropriate class. Then I'd be able to attach the CSS I wanted to those classes to colour code them like the del and ins elements, and it wouldn't break any existing code (except in the unlikely event that someone has already defined CSS for the new classes you're adding). This is still not possible without the patch to Text::Diff to fix (a). Pete.
Subject: Re: [rt.cpan.org #24301] XHTML doesn't allow div element inside pre element
Date: Thu, 8 May 2008 11:26:20 -0700
To: bug-Text-Diff-HTML [...] rt.cpan.org
From: "David E. Wheeler" <dwheeler [...] cpan.org>
On May 8, 2008, at 10:58, Pete Gillin via RT wrote: Show quoted text
> 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.)
Okay, that's kind of what I thought you were saying, though I wasn't sure. Thanks for the detailed example. Show quoted text
> * 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.
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
> 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...
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
> 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.
It's okay with me for feature requests to be here. It makes it easier to keep track. ;-) Best, David
Subject: Re: [rt.cpan.org #24301] XHTML doesn't allow div element inside pre element
Date: Thu, 8 May 2008 11:27:03 -0700
To: bug-Text-Diff-HTML [...] rt.cpan.org
From: "David E. Wheeler" <dwheeler [...] cpan.org>
Download (untitled) / with headers
text/plain 658b
On May 8, 2008, at 11:08, Pete Gillin via RT wrote: Show quoted text
> Actually, I've just thought that there's another option to fix by (b) > above: Text::Diff::HTML could arrange for the each filename to appear > in a span element with an appropriate class. Then I'd be able to > attach the CSS I wanted to those classes to colour code them like the > del and ins elements, and it wouldn't break any existing code > (except in the unlikely event that someone has already defined CSS for > the new classes you're adding).
GMTA. ;-) Show quoted text
> This is still not possible without the patch to Text::Diff to fix (a).
I'll take a look at it sometime in the next week. Best, David


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.