Bug 579069

Summary: callout in programlisting/screen not working completely in html output
Product: [Community] Publican Reporter: Raphaël Hertzog <raphael>
Component: publicanAssignee: Jeff Fearn <jfearn>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: 1.6CC: jfearn, mmcallis, publican-list, r.landmann
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: publican-1.6.3-0.fc12 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-05-17 14:56:26 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Attachments:
Description Flags
Sample book to reproduce the problem
none
Screenshot of the weird rendering
none
Possible patch none

Description Raphaël Hertzog 2010-04-02 08:59:31 EDT
Created attachment 404177 [details]
Sample book to reproduce the problem

Description of problem:
It fails to build the "html" output when you use <co id="something"> inside <programlisting> and inside <screen>.

Version-Release number of selected component (if applicable):
publican 1.6.2

How reproducible:
Try "publican build --langs=en-US --formats=html --publish" on the attached document.

You will get error messages during the build like this:
runtime error: file /usr/share/publican/xsl/xhtml-common.xsl line 979 element attribute
xsl:attribute: Cannot add attributes to an element if children have been already added to the element.

And the build will not complete successfully (you don't get to see "Finished html" but the line indicating that perl died "at /usr/share/perl5/Publican/Builder.pm line 746").
Comment 1 Raphaël Hertzog 2010-04-02 09:31:32 EDT
Created attachment 404180 [details]
Screenshot of the weird rendering

It might be related to the same problem, but I get a weird rendering of the callout in HTML (see attached picture): the reference callout image in the program listing is way bigger than the same image in the calloutlist afterwards (but the latter is inside a link while the former is not). It's also on a line of its own instead of being at the end of the line where it was in the source.

BTW, I'm using docbook-xsl 1.75.2 (latest version from Debian Unstable).
Comment 2 Raphaël Hertzog 2010-04-02 11:56:18 EDT
Created attachment 404203 [details]
Possible patch

The "anchor" template is not always called from the start of a new tag in the generic docbook XSL stylesheets (I gave an example where this legitimately happened). Thus it should output a tag on its own and not expect to be able to add an attribute to its parent tag.

I'm not sure why it was overridden from the default docbook-xsl stylesheet but in any case the override is not correct.
Comment 3 Jeff Fearn 2010-04-05 19:59:06 EDT
(In reply to comment #2)
> Created an attachment (id=404203) [details]
> Possible patch
> 
> The "anchor" template is not always called from the start of a new tag in the
> generic docbook XSL stylesheets (I gave an example where this legitimately
> happened). Thus it should output a tag on its own and not expect to be able to
> add an attribute to its parent tag.
> 
> I'm not sure why it was overridden from the default docbook-xsl stylesheet but
> in any case the override is not correct.    

The generic XSL outputs invalid XHTML. It is never valid to have an anchor tag with no content in XHTML, so we don't do that.

I'll look in to where this tag is being called from and see if I can get it to generate valid XHTML.

FWIW http://www.docbook.org/tdg/en/html/programlistingco.html has been tested and works, and since this is what you seem to be trying to achieve you could try that out until I can get to this.

Cheers, Jeff.
Comment 4 Raphaël Hertzog 2010-04-06 03:32:14 EDT
You might want to report that the docbook-xsl folks then I guess instead of fixing it only for publican.
Comment 5 Raphaël Hertzog 2010-04-06 13:19:47 EDT
I sent a mail upstream without checking your assertion that <a/> is invalid in XHTML and upstream says that the DTD allows <a> to be an empty tag: http://www.mail-archive.com/docbook-apps@lists.oasis-open.org/msg14001.html

So what was the concrete problem that you tried to avoid?
Comment 6 Jeff Fearn 2010-04-06 19:23:36 EDT
(In reply to comment #5)
> I sent a mail upstream without checking your assertion that <a/> is invalid in
> XHTML and upstream says that the DTD allows <a> to be an empty tag:
> http://www.mail-archive.com/docbook-apps@lists.oasis-open.org/msg14001.html
> 
> So what was the concrete problem that you tried to avoid?    

hmm this change was made several years ago, probably 3 or 4 years ago, way before we made publican open source, so it's a bit dim in my memory. On further thinking I believe my original response is incorrect.

If I recall correctly, and this was a long time ago, it was actually because the anchors are emitted in places where they are invalid. Anchors only have 8 or so valid parents in XHTML 1.0 Strict mode, assuming you categorize the various H tags as counting as one, and the anchors are emitted all over the place.

I think I looked in to everywhere it was called, and it was a big job to change the incorrect ones. Since it's never used anywhere there isn't another tag to link to, it was just easier to put the ID in the real tag instead of injecting a tag just to put an ID in.

The way we tested this was to build the Deployment Guide in html-single then run that through http://validator.w3.org, it generated massive numbers of errors in XHTML 1.0 Strict mode and we went about tidying them up.

I might actually give this another go since it's been a while since we've done that and invalid markup may have crept in.

FYI I've checked a fix for this in to the 1.6 branch and it will be available in 1.6.3 which will probably come out in the next week or so.
Comment 7 Raphaël Hertzog 2010-04-07 02:40:35 EDT
Thanks, I informed the docbook-xsl authors and gave them the URL of the deployment guide so that they can try to reproduce the original problem.

Thanks for the fix too, but it's only a partial fix for me. I have other cases where the anchor template is called without being at the start of a new tag, for example when processing the following docbook:

<sect1>
 <title>Some title</title>
 <para>Some text</para>
 <screen id="sample">
 </screen>
</sect1>

The problem comes when converting <screen>, it has an id tag to be able to refer to it and the docbook-xsl does:
<xsl:template match="programlisting|screen|synopsis">
  <xsl:param name="suppress-numbers" select="'0'"/>
  <xsl:variable name="id">
    <xsl:call-template name="object.id"/>
  </xsl:variable>

  <xsl:call-template name="anchor"/>
[...]

There's no new parent tag yet at that point.
Comment 8 Jeff Fearn 2010-05-04 17:53:07 EDT
(In reply to comment #7)
> Thanks, I informed the docbook-xsl authors and gave them the URL of the
> deployment guide so that they can try to reproduce the original problem.
> 
> Thanks for the fix too, but it's only a partial fix for me. I have other cases
> where the anchor template is called without being at the start of a new tag,
> for example when processing the following docbook:
> 
> <sect1>
>  <title>Some title</title>
>  <para>Some text</para>
>  <screen id="sample">
>  </screen>
> </sect1>
> 
> The problem comes when converting <screen>, it has an id tag to be able to
> refer to it and the docbook-xsl does:
> <xsl:template match="programlisting|screen|synopsis">
>   <xsl:param name="suppress-numbers" select="'0'"/>
>   <xsl:variable name="id">
>     <xsl:call-template name="object.id"/>
>   </xsl:variable>
> 
>   <xsl:call-template name="anchor"/>
> [...]
> 
> There's no new parent tag yet at that point.    

Hi Raphaël, we discourage linking to objects that don't have titles, because it is makes translation difficult. I'll sit down with Rudi and Manuel and add some text to the Users Guide on linking to informal objects and how that impacts translation.

It's very similar to the discussions in file:///usr/share/doc/publican-doc-1.6.2/en-US/index.html#appe-Users_Guide-Disallowed_elements_and_attributes

If you want to link to something you should be using a formal object, like an example, since this fits in to translation work flow.

I will look in to fixing this issue for any tags you bring to my attention though.
Comment 9 Raphaël Hertzog 2010-05-05 01:48:48 EDT
Right, in my case the references pointing to the object were <xref ... xrefstyle="page"> so it only had to contain a page number and no translatable material. Obviously this only works with the PDF output (my content comes from a published/printed book so it was ok in that context).
Comment 10 Jeff Fearn 2010-05-06 00:59:17 EDT
Hi, I took a different approach. First I emptied out the anchor  template so it does nothing. Then I overrode common.html.attributes to set the ID if it's present.

Need to QA the output, but it seems like an overall better approach as it shouldn't require overriding a bunch of templates.
Comment 11 Ruediger Landmann 2010-05-06 14:27:46 EDT
Confirmed working in 1.6.3.t150 -- callout numbers appear suitably small and located beside the actual line of code to which they apply.
Comment 12 Fedora Update System 2010-05-13 18:20:11 EDT
publican-1.6.3-0.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/publican-1.6.3-0.fc13
Comment 13 Fedora Update System 2010-05-13 18:21:32 EDT
publican-1.6.3-0.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/publican-1.6.3-0.fc12
Comment 14 Fedora Update System 2010-05-15 16:35:47 EDT
publican-1.6.3-0.fc12 has been pushed to the Fedora 12 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update publican'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/publican-1.6.3-0.fc12
Comment 15 Fedora Update System 2010-05-15 16:45:09 EDT
publican-1.6.3-0.fc13 has been pushed to the Fedora 13 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update publican'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/publican-1.6.3-0.fc13
Comment 16 Fedora Update System 2010-05-17 14:55:03 EDT
publican-1.6.3-0.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 17 Fedora Update System 2010-05-17 14:58:58 EDT
publican-1.6.3-0.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.