Merge pull request #2524 from johanrd/review/2427

Post-merge review of #2427 (`template-no-duplicate-landmark-elements`)

Author: NullVoxPopuli

lib/rules/template-no-duplicate-landmark-elements.js

@@ -131,10 +131,25 @@ module.exports = {
       }
     }
 
+    // Track elements that create new landmark scopes (dialog, popover)
+    const newScopeElements = [];
+
     return {
       GlimmerElementNode(node) {
         elementStack.push(node.tag);
 
+        // <dialog> and elements with popover attribute create isolated landmark scopes
+        if (isNewScopeElement(node)) {
+          landmarksStack.push(new Map());
+          newScopeElements.push(node);
+        }
+
+        // Dynamic role values — skip (can't determine role statically)
+        const roleAttr = node.attributes?.find((attr) => attr.name === 'role');
+        if (roleAttr && roleAttr.value?.type !== 'GlimmerTextNode') {
+          return;
+        }
+
         const landmarkRole = getLandmarkRole(
           node,
           LANDMARK_ROLES,
@@ -150,41 +165,22 @@ module.exports = {
         }
       },
 
-      'GlimmerElementNode:exit'() {
+      'GlimmerElementNode:exit'(node) {
         elementStack.pop();
-      },
-
-      GlimmerBlock(node) {
-        const parent = node.parent;
-        if (parent && isIfUnless(parent)) {
-          // Entering a branch of an if/unless block.
-          // Push a copy of the scope from BEFORE the conditional
-          // (which is the second-to-last on the stack, since the
-          // conditional's own scope was pushed on enter).
-          const parentScope = landmarksStack.at(-2) || landmarksStack.at(-1);
-          landmarksStack.push(cloneLandmarks(parentScope));
-        }
-      },
-
-      'GlimmerBlock:exit'(node) {
-        const parent = node.parent;
-        if (parent && isIfUnless(parent)) {
+        if (newScopeElements.at(-1) === node) {
+          newScopeElements.pop();
           landmarksStack.pop();
         }
       },
 
-      GlimmerBlockStatement(node) {
-        if (isIfUnless(node)) {
-          // Push a scope for the conditional block itself.
-          // Each branch (GlimmerBlock) will push its own scope on top.
-          landmarksStack.push(cloneLandmarks(currentLandmarks()));
-        }
+      // Every Block gets its own scope (matching the original's Block visitor).
+      // This handles each, let, with, etc. — not just if/unless.
+      GlimmerBlock() {
+        landmarksStack.push(cloneLandmarks(currentLandmarks()));
       },
 
-      'GlimmerBlockStatement:exit'(node) {
-        if (isIfUnless(node)) {
-          landmarksStack.pop();
-        }
+      'GlimmerBlock:exit'() {
+        landmarksStack.pop();
       },
 
       'Program:exit'() {
@@ -194,13 +190,6 @@ module.exports = {
   },
 };
 
-function isIfUnless(node) {
-  if (node.type === 'GlimmerBlockStatement' && node.path?.type === 'GlimmerPathExpression') {
-    return ['if', 'unless'].includes(node.path.original);
-  }
-  return false;
-}
-
 function getLabel(node) {
   // Check aria-label
   const ariaLabel = node.attributes?.find((attr) => attr.name === 'aria-label');
@@ -232,6 +221,10 @@ function getRoleValue(node) {
   return null;
 }
 
+function isNewScopeElement(node) {
+  return node.tag === 'dialog' || node.attributes?.some((attr) => attr.name === 'popover');
+}
+
 function getLandmarkRole(node, LANDMARK_ROLES, ELEMENT_TO_ROLE, insideSectioning) {
   const role = getRoleValue(node);
   if (role && LANDMARK_ROLES.has(role)) {

tests/lib/rules/template-no-duplicate-landmark-elements.js

@@ -18,6 +18,10 @@ ruleTester.run('template-no-duplicate-landmark-elements', rule, {
     '<template>{{#if this.isCreateProjectFromSavedSearchEnabled}}<form></form>{{else}}<form></form>{{/if}}</template>',
     // header inside sectioning element loses landmark role
     "<template><main><header><h1>Main Page Header</h1></header></main><dialog id='my-dialog'><header><h1>Dialog Header</h1></header></dialog></template>",
+    // Landmarks inside dialog are in a separate scope
+    '<template><nav></nav><dialog><nav></nav></dialog></template>',
+    // Dynamic role values — can't determine role statically
+    '<template><div role={{this.role}}></div><div role={{this.role}}></div></template>',
   ],
 
   invalid: [
@@ -81,6 +85,14 @@ hbsRuleTester.run('template-no-duplicate-landmark-elements (hbs)', rule, {
     "<main><header><h1>Main Page Header</h1></header><button commandfor='my-dialog'>Open Dialog</button></main><div popover id='my-dialog'><header><h1>Dialog Header</h1></header></div>",
     // Conditional branches: elements in if/else are mutually exclusive
     '{{#if this.isCreateProjectFromSavedSearchEnabled}}<form></form>{{else}}<form></form>{{/if}}',
+    // Landmarks inside dialog are in a separate scope
+    '<nav></nav><dialog><nav></nav></dialog>',
+    // Landmarks inside popover element are in a separate scope
+    '<nav></nav><div popover><nav></nav></div>',
+    // Dynamic role values — can't determine role statically
+    '<div role={{this.role}}></div><div role={{this.role}}></div>',
+    // Each blocks: landmarks in each body are scoped
+    '{{#each this.items as |item|}}<nav aria-label={{item.label}}></nav>{{/each}}',
   ],
   invalid: [
     {