[drupal-devel] [bug] Strange markup in book render functions

Fri May 6 08:53:12 UTC 2005

Issue status update for http://drupal.org/node/1898

 Project:      Drupal
 Version:      cvs
 Component:    book.module
 Category:     bug reports
 Priority:     critical
 Assigned to:  puregin
 Reported by:  kika
 Updated by:   Dries
 Status:       patch

Let's not abbreviate 'section' to 'sect'.  Also, we use '-' to separate
words in CSS names.  

- If the section ID uses the node ID, maybe we should use 'node-'
instead of 'sect-'?

- If the class uses depth, maybe we should use 'depth-' instead of
'sect'?

The same is true for your other patch where you suggest to use 'n'. 
Better to make that 'node-'.  (Feel free to merge both patches.)

Looks like the book.module generates a _lot_ of CSS IDs/classes. If the
introduction of these new IDs/classes allows us to remove some of the
existing IDs/classes, please do.

Dries

Previous comments:
------------------------------------------------------------------------

June 10, 2003 - 00:06 : kika

Snippet from book_render():

$output .= "<dt>". l($node->title, "node/view/$node->nid")
."</dt><dd>". book_body($node) ."<br /><br /></dd>";

Is using definition list here is really correct? This should be
replaced with appropiate div's or even better, funnel through theme()

Same goes for the function book_print():

 $output .= "$node->title";

  if ($node->body) {
    $output .= "<ul>". book_body($node) ."</ul>";
  }

Why wrap body inside <ul>?

------------------------------------------------------------------------

June 11, 2003 - 02:31 : al

Yeah, it's all nasty. I patched this ages ago, but no one seemed
interested. The patch now won't apply due to all the changes since
then. 

I'm on this - expect a new patch to hit my sandbox shortly.

------------------------------------------------------------------------

June 11, 2003 - 08:03 : ax

please remove the superfluous <ul>s and <li> [1], too.

to solve the problem of invalid nested <hx> tags, maybe we should
change &lt;h$depth&gt; to &lt;h1 class="book-h$depth"&gt;? this way, we
could use <h2> - <h6> in book pages. alternatively, we should only use
<h5> and <h6> in book pages - we shouldn't have nestings deeper than 4
in printed books, then.

additionally, i think it would be a good idea to add id's to all book
headings so they can be jumped to via fragment identifiers
(url#fragment-identifier). i often wish this would be possible when
referencing the last paragraph of a long page of documentation ...
these id's should be of the form <hx id="foo" name="foo"> [2], with foo
probably being the node-id of the single book page.

adding id's to headings would also be a good idea for headings /
sections in single book nodes. for example, it would be nice if i could
link to list point 10 in the drupal installation instructions [3] via
something like url#10. these id's had to be added manually. the
question is: how can these id's be named to be unique within a
/printed/ book? probably <node_id>_<section_id> ...

[1] [ [drupal-devel] Drupal handbook formatting and heading tags |
http://lists.drupal.org/pipermail/drupal-devel/2003-June/025860.html ]
[2] [ XHTML Spec, HTML Compatibility Guidelines, Fragment Identifiers |
http://www.w3.org/TR/xhtml1/#C_8 ]
[3] http://drupal.org/node/view/260

------------------------------------------------------------------------

June 15, 2003 - 16:23 : al

Fixed in latest CVS.
Uses all of Ax's suggestions:
 - <h1 class="depthX">
 - Fixed to use CSS rather than tables.
 - Changes link at top to standard "breadcrumb" style, rather than
hierarchy in tree view.
 - Removes odd and superfluous <ul> tags and the like.
 - Makes printed docs have proper <html> and <body> wrapping, etc.

------------------------------------------------------------------------

May 5, 2005 - 09:20 : puregin

I'm re-opening this, because the problem persists in CVS.

1) The current CVS version (1.288.2.4) of  book module still wraps
$node->body in an <ul> element in book_print_recurse().  This produces
non-conformant HTML.

I'm not sure why this was done in the first place - I'm guessing
someone wanted to insulate from having bad markup in a book page throw
off the enclosing page.

Getting rid of the offending UL tags is a one-line solution, but what
is the right thing to do here?

I'd like to see something very structural - wrapping the /entire/
output (including the generated sectional header) of the
book_print_recurse() wrapped in a <div> element.  This will properly
nest true structural sections, making the structure accessible to
applications via DOM.  It will also make it easier to use XSLT, for
example,  to transform into various XML based  export formats, for
example.  Any reason not to do so?

Also...  when we get the 'printer-friendly' version of a page, the
subtree rooted at that page is returned, and the root page is marked up
with a class="book-h1".   Is there any reason why we don't compute the
depth of the root page, and use that to generate the "in-place" header?
  In other words, if a section is "book-h3" relative to the top-level
book, shouldn't it be "book-h3" whenever it is viewed?

In this case, we could apply the same principal to generating the <div>
tags around each section.

------------------------------------------------------------------------

May 6, 2005 - 00:58 : puregin

Attachment: http://drupal.org/files/issues/book_printer_friendly_div.patch (1.57 KB)

I am attaching a patch which fixes improper printer-friendly output, as
discussed.

The patch fixes book_print and book_print_recurse() to insert a <div>
start tag before emitting the header, and to close this  after
subsections have been recursively generated.

The <div> tag has an id attribute of the form id="sect-123", where the
node's nid is 123, and a class attribute of form class="sect/n/" where
the depth of the section is /n/.

I'm marking this issue as critical, since the present HTML output plays
havoc with any attempts at sane PDF generation.

I will address the issue of where printer-friendly subsections should
be 'rooted' in a follow-up.

Djun