Formatter: Improve element and blank line spacing

javascript/packages/core/src/ast-utils.ts

@@ -19,11 +19,11 @@ import {
 } from "./nodes.js"
 
 import {
-  isNode,
   isAnyOf,
   isLiteralNode,
   isERBNode,
   isERBContentNode,
+  isHTMLCommentNode,
   areAllOfType,
   filterLiteralNodes
 } from "./node-type-guards.js"
@@ -37,6 +37,12 @@ export type ERBOutputNode = ERBNode & {
   }
 }
 
+export type ERBCommentNode = ERBNode & {
+  tag_opening: {
+    value: "<%#"
+  }
+}
+
 /**
  * Checks if a node is an ERB output node (generates content: <%= %> or <%== %>)
  */
@@ -47,6 +53,17 @@ export function isERBOutputNode(node: Node): node is ERBOutputNode {
   return ["<%=", "<%=="].includes(node.tag_opening?.value)
 }
 
+/**
+ * Checks if a node is a ERB comment node (control flow: <%# %>)
+ */
+export function isERBCommentNode(node: Node): node is ERBCommentNode {
+  if (!isERBNode(node)) return false
+  if (!node.tag_opening?.value) return false
+
+  return node.tag_opening?.value === "<%#" || (node.tag_opening?.value !== "<%#" && (node.content?.value || "").trimStart().startsWith("#"))
+}
+
+
 /**
  * Checks if a node is a non-output ERB node (control flow: <% %>)
  */
@@ -200,8 +217,8 @@ export function getTagName(node: HTMLElementNode | HTMLOpenTagNode | HTMLCloseTa
 /**
  * Check if a node is a comment (HTML comment or ERB comment)
  */
-export function isCommentNode(node: Node): boolean {
-  return isNode(node, HTMLCommentNode) || (isERBNode(node) && !isERBControlFlowNode(node))
+export function isCommentNode(node: Node): node is HTMLCommentNode | ERBCommentNode {
+  return isHTMLCommentNode(node) || isERBCommentNode(node)
 }
 
 /**

javascript/packages/formatter/src/format-helpers.ts

@@ -56,17 +56,6 @@ export const SPACEABLE_CONTAINERS = new Set([
   'figure', 'details', 'summary', 'dialog', 'fieldset'
 ])
 
-export const TIGHT_GROUP_PARENTS = new Set([
-  'ul', 'ol', 'nav', 'select', 'datalist', 'optgroup', 'tr', 'thead',
-  'tbody', 'tfoot'
-])
-
-export const TIGHT_GROUP_CHILDREN = new Set([
-  'li', 'option', 'td', 'th', 'dt', 'dd'
-])
-
-export const SPACING_THRESHOLD = 3
-
 /**
  * Token list attributes that contain space-separated values and benefit from
  * spacing around ERB content for readability

javascript/packages/formatter/src/format-printer.ts

@@ -13,6 +13,8 @@ import {
   isERBNode,
   isCommentNode,
   isERBControlFlowNode,
+  isERBCommentNode,
+  isERBOutputNode,
   filterNodes,
 } from "@herb-tools/core"
 
@@ -47,9 +49,6 @@ import {
   FORMATTABLE_ATTRIBUTES,
   INLINE_ELEMENTS,
   SPACEABLE_CONTAINERS,
-  SPACING_THRESHOLD,
-  TIGHT_GROUP_CHILDREN,
-  TIGHT_GROUP_PARENTS,
   TOKEN_LIST_ATTRIBUTES,
 } from "./format-helpers.js"
 
@@ -288,6 +287,86 @@ export class FormatPrinter extends Printer {
     return nodes.filter(child => isNoneOf(child, HTMLAttributeNode, WhitespaceNode))
   }
 
+
+  /**
+   * Determine if an HTML element or ERB block would format to multiple lines
+   * Used for spacing multiline elements
+   *
+   * We render the node and check if the output contains newlines.
+   * This is the most accurate and reliable way to determine multiline status.
+   */
+  private isMultilineElement(node: Node): boolean {
+    try {
+      return this.capture(() => this.visit(node)).join("\n").trim().includes("\n")
+    } catch (e) {
+      return false
+    }
+  }
+
+  /**
+   * Get a grouping key for a node (tag name for HTML, ERB type for ERB)
+   */
+  private getGroupingKey(node: Node): string | null {
+    if (isNode(node, HTMLElementNode)) {
+      return getTagName(node)
+    }
+
+    if (isERBOutputNode(node)) return "erb-output"
+    if (isERBCommentNode(node)) return "erb-comment"
+    if (isERBNode(node)) return "erb-code"
+
+    return null
+  }
+
+  /**
+   * Detect groups of consecutive same-tag/same-type single-line elements
+   * Returns a map of index -> group info for efficient lookup
+   */
+  private detectTagGroups(siblings: Node[]): Map<number, { tagName: string; groupStart: number; groupEnd: number }> {
+    const groupMap = new Map<number, { tagName: string; groupStart: number; groupEnd: number }>()
+    const meaningfulNodes: Array<{ index: number; groupKey: string }> = []
+
+    for (let i = 0; i < siblings.length; i++) {
+      const node = siblings[i]
+
+      if (!this.isMultilineElement(node)) {
+        const groupKey = this.getGroupingKey(node)
+
+        if (groupKey) {
+          meaningfulNodes.push({ index: i, groupKey })
+        }
+      }
+    }
+
+    let groupStart = 0
+
+    while (groupStart < meaningfulNodes.length) {
+      const startGroupKey = meaningfulNodes[groupStart].groupKey
+      let groupEnd = groupStart
+
+      while (groupEnd + 1 < meaningfulNodes.length && meaningfulNodes[groupEnd + 1].groupKey === startGroupKey) {
+        groupEnd++
+      }
+
+      if (groupEnd > groupStart) {
+        const groupStartIndex = meaningfulNodes[groupStart].index
+        const groupEndIndex = meaningfulNodes[groupEnd].index
+
+        for (let i = groupStart; i <= groupEnd; i++) {
+          groupMap.set(meaningfulNodes[i].index, {
+            tagName: startGroupKey,
+            groupStart: groupStartIndex,
+            groupEnd: groupEndIndex
+          })
+        }
+      }
+
+      groupStart = groupEnd + 1
+    }
+
+    return groupMap
+  }
+
   /**
    * Determine if spacing should be added between sibling elements
    *
@@ -303,69 +382,68 @@ export class FormatPrinter extends Printer {
    * @param hasExistingSpacing - Whether user-added spacing already exists
    * @returns true if spacing should be added before the current element
    */
-  private shouldAddSpacingBetweenSiblings(
-    parentElement: HTMLElementNode | null,
-    siblings: Node[],
-    currentIndex: number,
-    hasExistingSpacing: boolean
-  ): boolean {
-    if (hasExistingSpacing) {
+  private shouldAddSpacingBetweenSiblings(parentElement: HTMLElementNode | null, siblings: Node[], currentIndex: number, hasExistingSpacing: boolean): boolean {
+    if (hasExistingSpacing) return true
+
+    const currentNode = siblings[currentIndex]
+    const previousMeaningfulIndex = findPreviousMeaningfulSibling(siblings, currentIndex)
+    const previousNode = previousMeaningfulIndex !== -1 ? siblings[previousMeaningfulIndex] : null
+
+    if (previousNode && (isNode(previousNode, XMLDeclarationNode) || isNode(previousNode, HTMLDoctypeNode))) {
       return true
     }
 
     const hasMixedContent = siblings.some(child => isNode(child, HTMLTextNode) && child.content.trim() !== "")
 
-    if (hasMixedContent) {
-      return false
-    }
+    if (hasMixedContent) return false
 
-    const meaningfulSiblings = siblings.filter(child => isNonWhitespaceNode(child))
+    const isCurrentComment = isCommentNode(currentNode)
+    const isPreviousComment = previousNode ? isCommentNode(previousNode) : false
+    const isCurrentMultiline = this.isMultilineElement(currentNode)
+    const isPreviousMultiline = previousNode ? this.isMultilineElement(previousNode) : false
 
-    if (meaningfulSiblings.length < SPACING_THRESHOLD) {
-      return false
+    if (isPreviousComment && !isCurrentComment && (isNode(currentNode, HTMLElementNode) || isERBNode(currentNode))) {
+      return isPreviousMultiline && isCurrentMultiline
     }
 
-    const parentTagName = parentElement ? getTagName(parentElement) : null
-
-    if (parentTagName && TIGHT_GROUP_PARENTS.has(parentTagName)) {
+    if (isPreviousComment && isCurrentComment) {
       return false
     }
 
+    if (isCurrentMultiline || isPreviousMultiline) {
+      return true
+    }
+
+    const meaningfulSiblings = siblings.filter(child => isNonWhitespaceNode(child))
+    const parentTagName = parentElement ? getTagName(parentElement) : null
     const isSpaceableContainer = !parentTagName || (parentTagName && SPACEABLE_CONTAINERS.has(parentTagName))
 
     if (!isSpaceableContainer && meaningfulSiblings.length < 5) {
       return false
     }
 
-    const currentNode = siblings[currentIndex]
-    const previousMeaningfulIndex = findPreviousMeaningfulSibling(siblings, currentIndex)
-    const isCurrentComment = isCommentNode(currentNode)
+    const tagGroups = this.detectTagGroups(siblings)
+    const currentGroup = tagGroups.get(currentIndex)
+    const previousGroup = previousNode ? tagGroups.get(previousMeaningfulIndex) : undefined
 
-    if (previousMeaningfulIndex !== -1) {
-      const previousNode = siblings[previousMeaningfulIndex]
-      const isPreviousComment = isCommentNode(previousNode)
+    if (currentGroup && previousGroup && currentGroup.groupStart === previousGroup.groupStart && currentGroup.groupEnd === previousGroup.groupEnd) {
+      return false
+    }
 
-      if (isPreviousComment && !isCurrentComment && (isNode(currentNode, HTMLElementNode) || isERBNode(currentNode))) {
-        return false
-      }
+    if (previousGroup && previousGroup.groupEnd === previousMeaningfulIndex) {
+      return true
+    }
 
-      if (isPreviousComment && isCurrentComment) {
-        return false
-      }
+    const allSingleLineHTMLElements = meaningfulSiblings.every(node => isNode(node, HTMLElementNode) && !this.isMultilineElement(node))
+
+    if (allSingleLineHTMLElements && tagGroups.size === 0) {
+      return false
     }
 
     if (isNode(currentNode, HTMLElementNode)) {
       const currentTagName = getTagName(currentNode)
 
-      if (INLINE_ELEMENTS.has(currentTagName)) {
-        return false
-      }
-
-      if (TIGHT_GROUP_CHILDREN.has(currentTagName)) {
-        return false
-      }
-
-      if (currentTagName === 'a' && parentTagName === 'nav') {
+      if (currentTagName && INLINE_ELEMENTS.has(currentTagName)) {
         return false
       }
     }
@@ -676,7 +754,21 @@ export class FormatPrinter extends Printer {
       }
 
       if (isNonWhitespaceNode(child) && lastWasMeaningful && !hasHandledSpacing) {
-        this.push("")
+        const previousNode = i > 0 ? children[i - 1] : null
+        const hasExistingSpacing = !!(previousNode && isNode(previousNode, HTMLTextNode) &&
+          previousNode.content.trim() === "" &&
+          (previousNode.content.includes('\n\n') || previousNode.content.split('\n').length > 2))
+
+        const shouldAddSpacing = this.shouldAddSpacingBetweenSiblings(
+          null,
+          children,
+          i,
+          hasExistingSpacing
+        )
+
+        if (shouldAddSpacing) {
+          this.push("")
+        }
       }
 
       this.visit(child)
@@ -1189,8 +1281,7 @@ export class FormatPrinter extends Printer {
   }
 
   visitERBContentNode(node: ERBContentNode) {
-    // TODO: this feels hacky
-    if (node.tag_opening?.value === "<%#") {
+    if (isERBCommentNode(node)) {
       this.visitERBCommentNode(node)
     } else {
       this.printERBNode(node)

javascript/packages/formatter/test/cli.test.ts

@@ -94,6 +94,7 @@ describe("CLI Binary", () => {
       expect(formattedContent).toBe(dedent`
         <div class="container">
           <%= "Hello" %>
+
           <p>World</p>
         </div>
       ` + '\n')

javascript/packages/formatter/test/document-formatting.test.ts

@@ -147,7 +147,6 @@ describe("Document-level formatting", () => {
     const result = formatter.format(source)
     expect(result).toEqual(dedent`
       <% page_title = "User Profile" %>
-
       <% user_data = { name: "John", age: 30 } %>
 
       <!DOCTYPE html>
@@ -236,9 +235,7 @@ describe("Document-level formatting", () => {
     const result = formatter.format(source)
     expect(result).toEqual(dedent`
       <h1>Title</h1>
-
       <p>Content</p>
-
       <footer>Footer</footer>
     `)
   })

javascript/packages/formatter/test/erb-formatter/erb-formatter-additional.test.ts

@@ -85,7 +85,6 @@ describe("ERB Formatter Additional Tests", () => {
 
       expect(result).toBe(dedent`
         <%- vite_client_tag %>
-
         <%= vite_typescript_tag "application", "data-turbo-track": "reload", defer: true %>
 
         <%= stylesheet_link_tag "tailwind",
@@ -159,6 +158,7 @@ describe("ERB Formatter Additional Tests", () => {
           <div>
             <div>
               <span>Short text</span>
+
               <p>
                 In the event we decide to issue a refund, we will reimburse you no later
                 than fourteen (14) days from the date on which we make that

javascript/packages/formatter/test/erb-formatter/erb-formatter-compat.test.ts

@@ -120,7 +120,6 @@ describe("ERB Formatter Compatibility Tests", () => {
 
       expect(result).toEqual(dedent`
         <% if eeee then "b" else c end %>
-
         <% if eeee then a else c end %>
       `)
     })
@@ -147,7 +146,6 @@ describe("ERB Formatter Compatibility Tests", () => {
 
       expect(result).toEqual(dedent`
         <div <% if eeee then "b" else c end %>></div>
-
         <div <% if eeee then a else c end %>></div>
       `)
     })
@@ -328,7 +326,6 @@ describe("ERB Formatter Compatibility Tests", () => {
 
       expect(result).toEqual(dedent`
         <%# This is a comment %>
-
         <div>Content</div>
 
         <% # Another comment style %>
@@ -371,6 +368,7 @@ describe("ERB Formatter Compatibility Tests", () => {
         <% categories.each do |category| %>
           <div class="category">
             <h3><%= category.name %></h3>
+
             <% category.items.each do |item| %>
               <div class="item"><%= item.name %></div>
             <% end %>