From 41cd67517ea394acbcf97b796083cb99bfd57a76 Mon Sep 17 00:00:00 2001
From: Jonathan Hefner <jonathan@hefner.pro>
Date: Sat, 19 Aug 2023 12:40:12 -0500
Subject: [PATCH] Generate uniform <h1> headings for all modules

In 60e05b54004ebc34244888b840ae344815238700, we began generating default
`<h1>` headings for modules.  However, these headings were only
generated for modules that had a description.  If a module had no
comment to begin with, it would not get a generated heading.

Furthermore, since c17deffa99a415ca43f23a3df61b567aacd237dc, each
module's full name has been prominently displayed at the top of its
page.  These names are visually redundant with generated headings, which
also use the full name.

To unify the design and ensure that all modules have an `<h1>`, this
commit converts each module's full name into an `<h1>` heading inside an
`<hgroup>`, and uses postprocessing to pull any comment-based `<h1>`
into the `<hgroup>`.

This commit also renames the "Included Modules" section to "Inherits
From", and moves any listed base class from the top of the page to the
top of that section.  This change focuses the `<h1>`, both visually and
from an SEO perspective.
---
 .../generator/template/rails/_context.rhtml   | 37 ++++++-------
 lib/rdoc/generator/template/rails/class.rhtml | 11 ++--
 lib/rdoc/generator/template/rails/file.rhtml  |  7 ++-
 .../template/rails/resources/css/main.css     | 32 +++++++----
 lib/sdoc/generator.rb                         | 10 ----
 lib/sdoc/helpers.rb                           | 11 ++++
 lib/sdoc/postprocessor.rb                     | 10 ++++
 spec/helpers_spec.rb                          | 38 +++++++++++++
 spec/postprocessor_spec.rb                    | 53 +++++++++++++++++++
 spec/rdoc_generator_markup_spec.rb            | 36 -------------
 10 files changed, 157 insertions(+), 88 deletions(-)
 delete mode 100644 spec/rdoc_generator_markup_spec.rb

diff --git a/lib/rdoc/generator/template/rails/_context.rhtml b/lib/rdoc/generator/template/rails/_context.rhtml
index ffbccde6..4e5bc6d6 100644
--- a/lib/rdoc/generator/template/rails/_context.rhtml
+++ b/lib/rdoc/generator/template/rails/_context.rhtml
@@ -1,16 +1,13 @@
 <div id="context">
   <% unless (description = context.description).empty? %>
-    <% unless context.comment_title %>
-      <h1><%= context.title %></h1>
-    <% end %>
     <div class="description">
       <%= description %>
     </div>
   <% end %>
 
 
+  <%# File only: requires %>
   <% unless context.requires.empty? %>
-    <!-- File only: requires -->
     <div class="content__divider">Required Files</div>
     <ul>
       <% context.requires.each do |req| %>
@@ -20,6 +17,20 @@
   <% end %>
 
 
+  <%# Module only: ancestors %>
+  <% unless context.is_a?(RDoc::TopLevel) || (ancestors = module_ancestors(context)).empty? %>
+    <div class="content__divider">Inherits From</div>
+    <ul>
+      <% ancestors.each do |kind, ancestor| %>
+        <li>
+          <span class="kind"><%= kind %></span>
+          <%= ancestor.is_a?(String) ? full_name(ancestor) : link_to(ancestor) %>
+        </li>
+      <% end %>
+    </ul>
+  <% end %>
+
+
   <% sections = context.sections.select { |s| s.title }.sort_by{ |s| s.title.to_s } %>
   <% unless sections.empty? then %>
     <!-- Sections -->
@@ -51,22 +62,6 @@
     </dl>
   <% end %>
 
-  <% unless context.includes.empty? %>
-    <!-- Includes -->
-    <div class="content__divider">Included Modules</div>
-    <ul>
-      <% context.includes.each do |inc| %>
-        <li>
-          <% if inc.module.is_a?(String) %>
-            <%= full_name inc.name %>
-          <% else %>
-            <%= link_to inc.module %>
-          <% end %>
-        </li>
-      <% end %>
-    </ul>
-  <% end %>
-
 
 
   <% context.each_section do |section, constants, attributes| %>
@@ -185,7 +180,7 @@
     <ul>
       <% (context.modules.sort + context.classes.sort).each do |mod| %>
         <li>
-          <span class="type"><%= mod.type.upcase %></span>
+          <span class="kind"><%= mod.type %></span>
           <%= link_to mod %>
         </li>
       <% end %>
diff --git a/lib/rdoc/generator/template/rails/class.rhtml b/lib/rdoc/generator/template/rails/class.rhtml
index 7d0d3d4f..cb998f29 100644
--- a/lib/rdoc/generator/template/rails/class.rhtml
+++ b/lib/rdoc/generator/template/rails/class.rhtml
@@ -19,14 +19,9 @@
     <%= include_template '_panel.rhtml' %>
 
     <main id="content">
-        <div class="content__full-name">
-          <span class="qualifier"><%= klass.type %></span>
-          <%= module_breadcrumbs klass %>
-          <% if !klass.module? && superclass = klass.superclass %>
-            <span class="qualifier">&lt;</span>
-            <%= superclass.is_a?(String) ? full_name(superclass) : link_to(superclass) %>
-          <% end %>
-        </div>
+        <hgroup class="content__title">
+          <h1><span class="kind"><%= klass.type %></span> <%= module_breadcrumbs klass %></h1>
+        </hgroup>
 
         <%= include_template '_context.rhtml', {:context => klass} %>
 
diff --git a/lib/rdoc/generator/template/rails/file.rhtml b/lib/rdoc/generator/template/rails/file.rhtml
index cad54657..432c2c63 100644
--- a/lib/rdoc/generator/template/rails/file.rhtml
+++ b/lib/rdoc/generator/template/rails/file.rhtml
@@ -16,10 +16,9 @@
     <%= include_template '_panel.rhtml' %>
 
     <main id="content">
-        <h1 class="content__full-name">
-          <span class="qualifier">File</span>
-          <%= full_name file %>
-        </h1>
+        <hgroup class="content__title">
+          <h1><span class="kind">File</span> <%= full_name file %></h1>
+        </hgroup>
 
         <% if source_url = github_url(file.relative_name) %>
           <p><%= link_to_external "View on GitHub", source_url %></p>
diff --git a/lib/rdoc/generator/template/rails/resources/css/main.css b/lib/rdoc/generator/template/rails/resources/css/main.css
index 5cc18415..f3ac6525 100644
--- a/lib/rdoc/generator/template/rails/resources/css/main.css
+++ b/lib/rdoc/generator/template/rails/resources/css/main.css
@@ -55,9 +55,6 @@ a {
 a:has(> code:only-child) {
   text-decoration: none;
 }
-code a {
-  text-decoration: none;
-}
 
 /* TODO: Remove this hack when Firefox supports `:has()` */
 a code {
@@ -74,6 +71,15 @@ a code {
   display: none;
 }
 
+.kind {
+  font-family: monospace;
+  font-weight: bold;
+}
+
+.kind::after {
+  content: " "; /* Ensure trailing space has width of 1 monospace char */
+}
+
 table {
   border-collapse: collapse;
 }
@@ -380,18 +386,22 @@ html {
   font-style: normal;
 }
 
-.content__full-name {
-  font-family: monospace;
-  font-size: 1.4em;
+.content__title :is(h1, p) {
+  font-size: 1.6em;
   line-height: 1.25;
-  padding: 0.125em 0;
+}
+
+.content__title h1 {
+  font-weight: normal;
 
   margin-left: 1em;
   text-indent: -1em;
 }
 
-.content__full-name .qualifier {
-  font-weight: bold;
+.content__title p {
+  font-style: italic;
+
+  margin-top: 0;
 }
 
 .content__section-title {
@@ -513,6 +523,10 @@ html {
  * Description of method or module
  */
 
+#context > .description {
+  margin-top: var(--space-lg);
+}
+
 .description :is(h1, h2, h3, h4, h5, h6) {
   line-height: 1.25;
   padding: 0.125em 0;
diff --git a/lib/sdoc/generator.rb b/lib/sdoc/generator.rb
index 46bd14eb..d3713a35 100644
--- a/lib/sdoc/generator.rb
+++ b/lib/sdoc/generator.rb
@@ -27,16 +27,6 @@ class RDoc::Options
   attr_accessor :search_index
 end
 
-module RDoc::Generator::Markup
-  def comment_title
-    @comment_title ||= @comment.to_s.match(/\A[=#] (.*)$/) {|match| match[1] }
-  end
-
-  def title
-    comment_title || full_name
-  end
-end
-
 class RDoc::Generator::SDoc
   RDoc::RDoc.add_generator self
 
diff --git a/lib/sdoc/helpers.rb b/lib/sdoc/helpers.rb
index c1bd7144..f3cdaa28 100644
--- a/lib/sdoc/helpers.rb
+++ b/lib/sdoc/helpers.rb
@@ -127,6 +127,17 @@ def module_breadcrumbs(rdoc_module)
     "<code>#{crumbs.join("::<wbr>")}</code>"
   end
 
+  def module_ancestors(rdoc_module)
+    ancestors = rdoc_module.includes.map { |inc| ["module", inc.module] }
+
+    if !rdoc_module.module? && superclass = rdoc_module.superclass
+      superclass_name = superclass.is_a?(String) ? superclass : superclass.full_name
+      ancestors.unshift(["class", superclass]) unless superclass_name == "Object"
+    end
+
+    ancestors
+  end
+
   def method_signature(rdoc_method)
     if rdoc_method.call_seq
       rdoc_method.call_seq.split(/\n+/).map do |line|
diff --git a/lib/sdoc/postprocessor.rb b/lib/sdoc/postprocessor.rb
index 0e941493..2cdae5eb 100644
--- a/lib/sdoc/postprocessor.rb
+++ b/lib/sdoc/postprocessor.rb
@@ -13,6 +13,7 @@ def process(rendered)
     version_rails_guides_urls!(document)
     unlink_unintentional_ref_links!(document)
     style_ref_links!(document)
+    unify_h1_headings!(document)
     highlight_code_blocks!(document)
 
     document.to_s
@@ -88,6 +89,15 @@ def style_ref_links!(document)
     end
   end
 
+  def unify_h1_headings!(document)
+    if h1 = document.at_css("#context > .description h1:first-child")
+      if hgroup = document.at_css("#content > hgroup")
+        h1.remove
+        hgroup.add_child(%(<p>#{h1.inner_html}</p>))
+      end
+    end
+  end
+
   def highlight_code_blocks!(document)
     document.css(".description pre > code, .method__source pre > code").each do |element|
       code = element.inner_text
diff --git a/spec/helpers_spec.rb b/spec/helpers_spec.rb
index 5875f2d2..2e43e230 100644
--- a/spec/helpers_spec.rb
+++ b/spec/helpers_spec.rb
@@ -523,6 +523,44 @@ module Foo; module Bar; module Qux; end; end; end
     end
   end
 
+  describe "#module_ancestors" do
+    it "returns a list with the base class (if applicable) and included modules" do
+      # RDoc chokes on ";" when parsing includes, so replace with "\n".
+      top_level = rdoc_top_level_for <<~RUBY.gsub(";", "\n")
+        module M1; end
+        module M2; end
+        class C1; end
+
+        module Foo; include M1; include M2; end
+        class Bar < C1; include M2; include M1; end
+        class Qux < Cx; include Foo; include Mx; end
+      RUBY
+
+      m1, m2, c1, foo, bar, qux = %w[M1 M2 C1 Foo Bar Qux].map { |name| top_level.find_module_named(name) }
+
+      _(@helpers.module_ancestors(foo)).must_equal [["module", m1], ["module", m2]]
+      _(@helpers.module_ancestors(bar)).must_equal [["class", c1], ["module", m2], ["module", m1]]
+      _(@helpers.module_ancestors(qux)).must_equal [["class", "Cx"], ["module", foo], ["module", "Mx"]]
+    end
+
+    it "excludes the default base class (Object) from the result" do
+      # RDoc chokes on ";" when parsing includes, so replace with "\n".
+      top_level = rdoc_top_level_for <<~RUBY.gsub(";", "\n")
+        class Object; end
+        class Foo; include M1; end
+      RUBY
+
+      _(@helpers.module_ancestors(top_level.find_module_named("Object"))).must_equal [["class", "BasicObject"]]
+      _(@helpers.module_ancestors(top_level.find_module_named("Foo"))).must_equal [["module", "M1"]]
+
+      top_level = rdoc_top_level_for <<~RUBY.gsub(";", "\n")
+        class Foo; include M1; end
+      RUBY
+
+      _(@helpers.module_ancestors(top_level.find_module_named("Foo"))).must_equal [["module", "M1"]]
+    end
+  end
+
   describe "#method_signature" do
     it "returns the method signature wrapped in <code>" do
       method = rdoc_top_level_for(<<~RUBY).find_module_named("Foo").find_method("bar", false)
diff --git a/spec/postprocessor_spec.rb b/spec/postprocessor_spec.rb
index 82288639..16f295d4 100644
--- a/spec/postprocessor_spec.rb
+++ b/spec/postprocessor_spec.rb
@@ -124,6 +124,59 @@
       _(SDoc::Postprocessor.process(rendered)).must_include expected
     end
 
+    it "unifies <h1> headings for a context" do
+      rendered = <<~HTML
+        <div id="content">
+          <hgroup><h1>module Foo</h1></hgroup>
+
+          <div id="context">
+            <div class="description"><h1>The Foo</h1><p>Lorem ipsum.</p></div>
+          </div>
+        </div>
+      HTML
+
+      expected = <<~HTML
+        <div id="content">
+          <hgroup><h1>module Foo</h1><p>The Foo</p></hgroup>
+
+          <div id="context">
+            <div class="description"><p>Lorem ipsum.</p></div>
+          </div>
+        </div>
+      HTML
+
+      _(SDoc::Postprocessor.process(rendered)).must_include expected
+    end
+
+    it "does not relocate non-leading <h1> headings" do
+      rendered = <<~HTML
+        <div id="content">
+          <hgroup><h1>module Foo</h1></hgroup>
+
+          <div id="context">
+            <div class="description"><p>Lorem ipsum.</p><h1>Red Herring</h1></div>
+            <div class="method">
+              <div class="description"><h1>Red Herring</h1></div>
+            </div>
+          </div>
+        </div>
+      HTML
+
+      _(SDoc::Postprocessor.process(rendered)).must_include rendered
+    end
+
+    it "does not relocate <h1> headings when <hgroup> is not present" do
+      rendered = <<~HTML
+        <div id="content">
+          <div id="context">
+            <div class="description"><h1>Main Page</h1></div>
+          </div>
+        </div>
+      HTML
+
+      _(SDoc::Postprocessor.process(rendered)).must_include rendered
+    end
+
     it "highlights code blocks" do
       rendered = <<~HTML
         <div class="description">
diff --git a/spec/rdoc_generator_markup_spec.rb b/spec/rdoc_generator_markup_spec.rb
deleted file mode 100644
index 5d44bb8e..00000000
--- a/spec/rdoc_generator_markup_spec.rb
+++ /dev/null
@@ -1,36 +0,0 @@
-require File.join(File.dirname(__FILE__), '/spec_helper')
-
-describe RDoc::Generator::Markup do
-  before :each do
-    @module = RDoc::NormalModule.new 'Example::SomeClass'
-  end
-
-  describe "#comment_title" do
-    it "should parse the h1 title from the comment if present" do
-      @module.comment = RDoc::Comment.new '= Some Title'
-      _(@module.comment_title).must_equal 'Some Title'
-    end
-
-    it "should parse the markdown h1 title from the comment if present" do
-      @module.comment = RDoc::Comment.new '# Markdown Title'
-      _(@module.comment_title).must_equal 'Markdown Title'
-    end
-
-    it "should ignore lower level titles" do
-      @module.comment = RDoc::Comment.new '== Some Title'
-      _(@module.comment_title).must_be_nil
-    end
-  end
-
-  describe "#title" do
-    it "should parse the h1 title from the comment if present" do
-      @module.comment = RDoc::Comment.new '= Some Title'
-      _(@module.title).must_equal 'Some Title'
-    end
-
-    it "should fallback to the full_name" do
-      @module.comment = RDoc::Comment.new 'Some comment without title'
-      _(@module.title).must_equal "Example::SomeClass"
-    end
-  end
-end