Re: html structure revisited

[Gavin Smith]

On Sat, Feb 13, 2021 at 10:34:06AM -0800, Per Bothner wrote:
> On 2/13/21 8:59 AM, Patrice Dumas wrote:
> > Regarding your patch, my feeling is that the best is to have a function
> > to register opened sectioning commands, and another to ask for the
> > closing elements down to a given level.  Does it looks ok?
> 
> How about the attached new patch?
> 
> I also added a call to $command->{'extra'}->{'associated_node'}.
> This is to catch the case of a header command (such as @subheading)
> with no associated node.
> 
> (The need for that is because my change "switches" the
> node and section commands: The tree has the node element before the
> sectioning command.  However, my change is to have the emitted
> sectioning element wrap the node elements *and* sub-sections,
> so the "node" element is processed to emit a <div class="section">
> while the "section" element generates a <div class="node">.)

It's good that you are managing to understand the code and have
made the changes that you wanted to.

I have been looking at your patch and reading earlier mails on
this topic.

I have two main questions:
* Why output extra <div> elements for split output, when there is
only one section (not including subsections) per output file anyway?
It's harmless, anyway.

* What is the point of <div class="node">?  If it is really necessary
to wrap a section's contents, not including subsections, in a <div>,
then I think this should be called something else.  It's already
confusing exactly what a "node" is.  (Is it a part of a manual that
is displayed to the user at once, and has sources in a clearly
separate part of the output file/files (as in split HTML output,
and for Info output), or is it the target of a cross-reference,
or is it an entry in the table of contents - all related concepts,
indeed, but there is the potential for confusion).  Does a node in
HTML include the navigation bar in a node, does it include a table
of contents in a node?  I don't see any need to add to the confusion.

It's clear that <div class='chapter'> should wrap a whole chapter
(clear as well to somebody who's never heard of Texinfo before),
but <div class='node'> could mean anything.(*)

I propose not to output the <div> for "node" unless there is a good
reason for it.

(*) "When I use a word, it means just what I choose it to mean - neither
more nor less."  --- Humpty Dumpty

> -- 
>       --Per Bothner
> per@bothner.com   http://per.bothner.com/

> diff --git a/tp/Texinfo/Convert/HTML.pm b/tp/Texinfo/Convert/HTML.pm
> index 443b6b4702..e1ebeedde4 100644
> --- a/tp/Texinfo/Convert/HTML.pm
> +++ b/tp/Texinfo/Convert/HTML.pm
> @@ -2402,6 +2402,29 @@ sub _default_element_header($$$$)
>    return $result;
>  }
>  
> +sub _open_section($$$)
> +{
> +  my $self = shift;
> +  my $level = shift;
> +  my $close = shift;
> +  while (@{$self->{'pending_closes'}} < $level) {
> +      push(@{$self->{'pending_closes'}}, "");
> +  }
> +  push(@{$self->{'pending_closes'}}, $close);
> +}
> +
> +sub _close_sections($$)
> +{
> +  my $self = shift;
> +  my $level = shift;
> +  my $result = '';
> +  while (@{$self->{'pending_closes'}} > $level) {
> +      my $close = pop @{$self->{'pending_closes'}};
> +      $result .= $close if ($close);
> +  }
> +  return $result;
> +}
> +
>  sub _convert_heading_command($$$$$)
>  {
>    my $self = shift;
> @@ -2419,9 +2442,26 @@ sub _convert_heading_command($$$$$)
>      return $result;
>    }
>  
> +  my $section = $command->{'extra'}->{'associated_section'};
> +  my $node;
> +  if ($section) {
> +      my $level = $section->{'level'};
> +      $result .= $self->_close_sections($level);
> +      $self->_open_section($level, "</div>\n");
> +  } else {
> +      $node = $command->{'extra'}->{'associated_node'};
> +  }
> +  $result .= '<div';
> +  if ($section) {
> +      $result .= ' class="'.$section->{'cmdname'}.'"';
> +  } elsif ($node) {
> +      $result .= ' class="node"';
> +  } else {
> +      $result .= " class=\"$cmdname\"";
> +  }
>    my $element_id = $self->command_id($command);
> -  $result .= "<span id=\"$element_id\"></span>"
> -    if (defined($element_id) and $element_id ne '');
> +  $result .= " id=\"$element_id\""
> +      if (defined($element_id) and $element_id ne '');
>  
>    print STDERR "Process $command "
>          .Texinfo::Structuring::_print_root_command_texi($command)."\n"
> @@ -2433,6 +2473,7 @@ sub _convert_heading_command($$$$$)
>        and $command->{'parent'}->{'type'} eq 'element') {
>      $element = $command->{'parent'};
>    }
> +  $result .= ">\n";
>    if ($element) {
>      $result .= &{$self->{'format_element_header'}}($self, $cmdname, 
>                                              $command, $element);
> @@ -2514,7 +2555,7 @@ sub _convert_heading_command($$$$$)
>                         eq 'inline')))) {
>      $result .= _mini_toc($self, $command);
>    }
> -
> +  $result .= '</div>' if (! $section);
>    return $result;
>  }
>  
> @@ -4663,9 +4704,6 @@ sub _convert_element_type($$$$)
>    if ($element->{'extra'}->{'special_element'}) {
>      $special_element = $element->{'extra'}->{'special_element'};
>      my $id = $self->command_id($element);
> -    if ($id ne '') {
> -      $result .= "<span id=\"$id\"></span>\n";
> -    }
>      if ($self->get_conf('HEADERS') 
>          # first in page
>          or $self->{'counter_in_file'}->{$element->{'filename'}} == 1) {
> @@ -4691,13 +4729,13 @@ sub _convert_element_type($$$$)
>      if ($special_element_body eq '') {
>        return '';
>      }
> -    $result .= $special_element_body;
> +    $result .= $special_element_body . '</div>';
>    } elsif (!$element->{'element_prev'}) {
>      $result .= $self->_print_title();
>      if (!$element->{'element_next'}) {
>        # only one element
>        my $foot_text = &{$self->{'format_footnotes_text'}}($self);
> -      return 
> $result.$content.$foot_text.$self->get_conf('DEFAULT_RULE')."\n";
> +      return 
> $result.$content.$foot_text.$self->get_conf('DEFAULT_RULE')."</div>\n";
>      }
>    }
>    $result .= $content unless ($special_element);
> @@ -5127,6 +5165,7 @@ sub converter_initialize($)
>  
>    $self->{'document_context'} = [];
>    $self->{'multiple_pass'} = [];
> +  $self->{'pending_closes'} = [];
>    $self->_new_document_context('_toplevel_context');
>  
>    if ($self->get_conf('SPLIT') and $self->get_conf('SPLIT') ne 'chapter'
> @@ -6494,10 +6533,10 @@ sub _default_program_string($)
>  sub _default_end_file($)
>  {
>    my $self = shift;
> -  my $program_text = '';
> +  my $program_text = $self->_close_sections(0);
>    if ($self->get_conf('PROGRAM_NAME_IN_FOOTER')) {
>      my $program_string = &{$self->{'format_program_string'}}($self);
> -    $program_text = "<p><font size=\"-1\">
> +    $program_text .= "<p><font size=\"-1\">
>    $program_string
>  </font></p>";
>    }