Only remove processed references to DOM fragments

danilsomsikov · Devtools-frontend LUCI CQ · commit 282ff278ad4a · 2025-03-28T04:15:41.000-07:00
Bug: 400353541 Change-Id: I9b93a605c7ae90705539a9293c4c9b1a9c998582 Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6405493 Reviewed-by: Philip Pfaffe <pfaffe@chromium.org> Commit-Queue: Danil Somsikov <dsv@chromium.org>
diff --git a/scripts/eslint_rules/lib/no-imperative-dom-api.js b/scripts/eslint_rules/lib/no-imperative-dom-api.js
@@ -66,8 +66,9 @@ module.exports = {
     }
 
     /**
-     *  @param {EsLintNode} reference
-     *  @param {DomFragment} domFragment
+     * @param {EsLintNode} reference
+     * @param {DomFragment} domFragment
+     * @return {boolean}
      */
     function processReference(reference, domFragment) {
       const parent = reference.parent;
@@ -93,23 +94,32 @@ module.exports = {
       const subpropertyValue = isSubpropertyAssignment ? grandGrandParent.right : null;
       for (const rule of subrules) {
         if (isPropertyAssignment) {
-          rule.propertyAssignment?.(property, propertyValue, domFragment);
+          if (rule.propertyAssignment?.(property, propertyValue, domFragment)) {
+            return true;
+          }
         } else if (isMethodCall) {
           if (isIdentifier(property, 'addEventListener')) {
             const event = getEvent(firstArg);
             const value = secondArg;
             if (event && value.type !== 'SpreadElement') {
               domFragment.eventListeners.push({key: event, value});
             }
-            return;
+            return true;
+          }
+          if (rule.methodCall?.(property, firstArg, secondArg, domFragment, grandParent)) {
+            return true;
           }
-          rule.methodCall?.(property, firstArg, secondArg, domFragment, grandParent);
         } else if (isPropertyMethodCall) {
-          rule.propertyMethodCall?.(property, grandParent.property, propertyMethodArgument, domFragment);
+          if (rule.propertyMethodCall?.(property, grandParent.property, propertyMethodArgument, domFragment)) {
+            return true;
+          }
         } else if (isSubpropertyAssignment) {
-          rule.subpropertyAssignment?.(property, subproperty, subpropertyValue, domFragment);
+          if (rule.subpropertyAssignment?.(property, subproperty, subpropertyValue, domFragment)) {
+            return true;
+          }
         }
       }
+      return false;
     }
 
     /**
@@ -119,23 +129,32 @@ module.exports = {
       /** @type {[number, number][]} */
       const ranges = [];
       for (const reference of domFragment.references) {
-        const expression = getEnclosingExpression(reference);
+        if (!reference.processed) {
+          continue;
+        }
+        const expression = getEnclosingExpression(reference.node);
         if (!expression) {
           continue;
         }
         const range = expression.range;
-        while ([' ', '\n'].includes(sourceCode.text[range[0] - 1])) {
-          range[0]--;
-        }
         ranges.push(range);
         for (const child of domFragment.children) {
           ranges.push(...getRangesToRemove(child));
         }
       }
+
+      if (domFragment.initializer && domFragment.references.every(r => r.processed)) {
+        ranges.push(getEnclosingExpression(domFragment.initializer).range);
+      }
+      for (const range of ranges) {
+        while ([' ', '\n'].includes(sourceCode.text[range[0] - 1])) {
+          range[0]--;
+        }
+      }
       ranges.sort((a, b) => a[0] - b[0]);
       for (let i = 1; i < ranges.length; i++) {
         if (ranges[i][0] < ranges[i - 1][1]) {
-          ranges[i] = [ranges[i - 1][1], ranges[i][1]];
+          ranges[i] = [ranges[i - 1][1], Math.max(ranges[i][1], ranges[i - 1][1])];
         }
       }
 
@@ -146,7 +165,8 @@ module.exports = {
      * @param {DomFragment} domFragment
      */
     function maybeReportDomFragment(domFragment) {
-      if (!domFragment.replacementLocation || domFragment.parent || !domFragment.tagName) {
+      if (!domFragment.replacementLocation || domFragment.parent || !domFragment.tagName ||
+          domFragment.references.every(r => !r.processed)) {
         return;
       }
       context.report({
@@ -194,7 +214,9 @@ export const DEFAULT_VIEW = (input, _output, target) => {
       'Program:exit'() {
         for (const domFragment of DomFragment.values()) {
           for (const reference of domFragment.references) {
-            processReference(reference, domFragment);
+            if (processReference(reference.node, domFragment)) {
+              reference.processed = true;
+            }
           }
         }
 
diff --git a/scripts/eslint_rules/lib/no-imperative-dom-api/adorner.js b/scripts/eslint_rules/lib/no-imperative-dom-api/adorner.js
@@ -38,11 +38,10 @@ module.exports = {
                   {key: 'jslog', value: '${VisualLogging.adorner(' + sourceCode.getText(property.value) + ')}'});
             }
             if (isIdentifier(property.key, 'content')) {
-              const childFragment = DomFragment.getOrCreate(property.value, sourceCode);
-              childFragment.parent = domFragment;
-              domFragment.children.push(childFragment);
+              domFragment.appendChild(property.value, sourceCode);
             }
           }
+          return true;
         }
       },
       NewExpression(node) {
diff --git a/scripts/eslint_rules/lib/no-imperative-dom-api/class-member.js b/scripts/eslint_rules/lib/no-imperative-dom-api/class-member.js
@@ -21,6 +21,9 @@ class ClassMember {
   /** @type {Node} */
   classDeclaration;
 
+  /** @type {Node|undefined} */
+  initializer;
+
   /**
    * @param {Node} node
    * @param {SourceCode} sourceCode
@@ -44,6 +47,9 @@ class ClassMember {
       classMembers.set(memberName, classMember);
     }
     classMember.references.add(node);
+    if (node.type === 'AssignmentExpression') {
+      classMember.initializer = node;
+    }
     return classMember;
   }
 }
diff --git a/scripts/eslint_rules/lib/no-imperative-dom-api/dom-api-devtools-extensions.js b/scripts/eslint_rules/lib/no-imperative-dom-api/dom-api-devtools-extensions.js
@@ -7,9 +7,9 @@
 'use strict';
 
 const {isIdentifier} = require('./ast.js');
-const {DomFragment} = require('./dom-fragment.js');
 
 /** @typedef {import('eslint').Rule.Node} Node */
+/** @typedef {import('./dom-fragment.js').DomFragment} DomFragment */
 
 module.exports = {
   create : function(context) {
@@ -26,13 +26,12 @@ module.exports = {
       methodCall(property, firstArg, secondArg, domFragment, call) {
         if (isIdentifier(property, 'createChild')) {
           if (firstArg?.type === 'Literal') {
-            const childFragment = DomFragment.getOrCreate(call, sourceCode);
+            const childFragment = domFragment.appendChild(call, sourceCode);
             childFragment.tagName = String(firstArg.value);
-            childFragment.parent = domFragment;
-            domFragment.children.push(childFragment);
             if (secondArg) {
               childFragment.classList.push(secondArg);
             }
+            return true;
           }
         }
       }
diff --git a/scripts/eslint_rules/lib/no-imperative-dom-api/dom-api.js b/scripts/eslint_rules/lib/no-imperative-dom-api/dom-api.js
@@ -23,9 +23,13 @@ module.exports = {
       propertyAssignment(property, propertyValue, domFragment) {
         if (isIdentifier(property, 'className')) {
           domFragment.classList.push(propertyValue);
-        } else if (isIdentifier(property, ['textContent', 'innerHTML'])) {
+          return true;
+        }
+        if (isIdentifier(property, ['textContent', 'innerHTML'])) {
           domFragment.textContent = propertyValue;
+          return true;
         }
+        return false;
       },
       /**
        * @param {Node} property
@@ -36,7 +40,9 @@ module.exports = {
       propertyMethodCall(property, method, firstArg, domFragment) {
         if (isIdentifier(property, 'classList') && isIdentifier(method, 'add')) {
           domFragment.classList.push(firstArg);
+          return true;
         }
+        return false;
       },
       /**
        * @param {Node} property
@@ -52,8 +58,10 @@ module.exports = {
               key: property,
               value: subpropertyValue,
             });
+            return true;
           }
         }
+        return false;
       },
       /**
        * @param {Node} property
@@ -68,12 +76,14 @@ module.exports = {
           const value = secondArg;
           if (attribute.type === 'Literal' && value.type !== 'SpreadElement') {
             domFragment.attributes.push({key: attribute.value.toString(), value});
+            return true;
           }
-        } else if (isIdentifier(property, 'appendChild')) {
-          const childFragment = DomFragment.getOrCreate(firstArg, sourceCode);
-          childFragment.parent = domFragment;
-          domFragment.children.push(childFragment);
         }
+        if (isIdentifier(property, 'appendChild')) {
+          domFragment.appendChild(firstArg, sourceCode);
+          return true;
+        }
+        return false;
       },
       MemberExpression(node) {
         if (isIdentifier(node.object, 'document') && isIdentifier(node.property, 'createElement')
diff --git a/scripts/eslint_rules/lib/no-imperative-dom-api/dom-fragment.js b/scripts/eslint_rules/lib/no-imperative-dom-api/dom-fragment.js
@@ -29,7 +29,8 @@ class DomFragment {
   /** @type {DomFragment|undefined} */ parent;
   /** @type {string|undefined} */ expression;
   /** @type {EsLintNode|undefined} */ replacementLocation;
-  /** @type {EsLintNode[]} */ references = [];
+  /** @type {EsLintNode|undefined} */ initializer;
+  /** @type {{node: EsLintNode, processed?: boolean}[]} */ references = [];
 
   /**
    * @param {Node} estreeNode
@@ -46,17 +47,19 @@ class DomFragment {
       domFragments.set(key, result);
       if ('parent' in key) {
         result.expression = sourceCode.getText(node);
-        result.references.push(node);
+        result.references.push({node});
       } else if (key instanceof ClassMember) {
         result.replacementLocation = /** @type {EsLintNode} */ (key.classDeclaration);
         result.expression = sourceCode.getText(node);
       } else {
-        result.references = key.references.map(r => (/** @type {EsLintNode} */ (r.identifier)));
-        result.references.push(/** @type {EsLintNode} */ (key.identifiers[0]));
+        result.references = key.references.filter(r => !key.identifiers.includes(r.identifier))
+                                .map(r => ({node: /** @type {EsLintNode} */ (r.identifier)}));
+        result.initializer = /** @type {EsLintNode} */ (key.identifiers[0]);
       }
     }
     if (key instanceof ClassMember) {
-      result.references = /** @type {EsLintNode[]} */ ([...key.references]);
+      result.references = [...key.references].map(r => ({node: /** @type {EsLintNode} */ (r)}));
+      result.initializer = /** @type {EsLintNode} */ (key.initializer);
     }
     return result;
   }
@@ -145,6 +148,22 @@ class DomFragment {
     components.push('</', this.tagName, '>');
     return components;
   }
+
+  /**
+   *  @param {Node} node
+   *  @param {SourceCode} sourceCode
+   */
+  appendChild(node, sourceCode) {
+    const child = DomFragment.getOrCreate(node, sourceCode);
+    this.children.push(child);
+    child.parent = this;
+    for (const reference of child.references) {
+      if (reference.node === node) {
+        reference.processed = true;
+      }
+    }
+    return child;
+  }
 }
 
 /**
diff --git a/scripts/eslint_rules/lib/no-imperative-dom-api/toolbar.js b/scripts/eslint_rules/lib/no-imperative-dom-api/toolbar.js
@@ -39,10 +39,10 @@ module.exports = {
        */
       methodCall(property, firstArg, secondArg, domFragment, call) {
         if (isIdentifier(property, 'appendToolbarItem')) {
-          const childFragment = DomFragment.getOrCreate(firstArg, sourceCode);
-          childFragment.parent = domFragment;
-          domFragment.children.push(childFragment);
+          domFragment.appendChild(firstArg, sourceCode);
+          return true;
         }
+        return false;
       },
       NewExpression(node) {
         if (isMemberExpression(
@@ -99,15 +99,13 @@ module.exports = {
               key: 'list',
               value: 'completions',
             });
-            const dataList = DomFragment.getOrCreate(completions, sourceCode);
+            const dataList = domFragment.appendChild(completions, sourceCode);
             dataList.tagName = 'datalist';
             dataList.attributes.push({
               key: 'id',
               value: 'completions',
             });
             dataList.textContent = completions;
-            domFragment.children.push(dataList);
-            dataList.parent = domFragment;
           }
           args.shift();  // dynamicCompletions is not supported
           const jslogContext = args.shift();
diff --git a/scripts/eslint_rules/tests/no-imperative-dom-api.test.js b/scripts/eslint_rules/tests/no-imperative-dom-api.test.js
@@ -363,5 +363,42 @@ class Widget2 extends UI.Widget.Widget {
 }`,
       errors: [{messageId: 'preferTemplateLiterals'}, {messageId: 'preferTemplateLiterals'}],
     },
+    {
+      filename: 'front_end/ui/components/component/file.ts',
+      code: `
+class SomeWidget extends UI.Widget.Widget {
+  constructor() {
+    super();
+    const toolbar = this.contentElement.createChild('devtools-toolbar');
+    const filterInput = new UI.Toolbar.ToolbarButton(i18nString(UIStrings.editName), 'edit', undefined, 'edit-name');
+    toolbar.appendToolbarItem(filterInput);
+    const anotherElement = document.createElement('div');
+    this.process(filterInput, anotherElement);
+  }
+}`,
+      output: `
+
+export const DEFAULT_VIEW = (input, _output, target) => {
+  render(html\`
+    <div>
+      <devtools-toolbar>
+        <devtools-button title=\${i18nString(UIStrings.editName)}
+            .variant=\${Buttons.Button.Variant.TOOLBAR} .iconName=\${'edit'}
+            .jslogContext=\${'edit-name'}></devtools-button>
+      </devtools-toolbar>
+    </div>\`,
+    target, {host: input});
+};
+
+class SomeWidget extends UI.Widget.Widget {
+  constructor() {
+    super();
+    const filterInput = new UI.Toolbar.ToolbarButton(i18nString(UIStrings.editName), 'edit', undefined, 'edit-name');
+    const anotherElement = document.createElement('div');
+    this.process(filterInput, anotherElement);
+  }
+}`,
+      errors: [{messageId: 'preferTemplateLiterals'}],
+    },
   ],
 });

Original file line number	Diff line number	Diff line change
`@@ -38,11 +38,10 @@ module.exports = {`
`38`	`38`	`{key: 'jslog', value: '${VisualLogging.adorner(' + sourceCode.getText(property.value) + ')}'});`
`39`	`39`	`}`
`40`	`40`	`if (isIdentifier(property.key, 'content')) {`
`41`		`- const childFragment = DomFragment.getOrCreate(property.value, sourceCode);`
`42`		`- childFragment.parent = domFragment;`
`43`		`- domFragment.children.push(childFragment);`
	`41`	`+ domFragment.appendChild(property.value, sourceCode);`
`44`	`42`	`}`
`45`	`43`	`}`
	`44`	`+ return true;`
`46`	`45`	`}`
`47`	`46`	`},`
`48`	`47`	`NewExpression(node) {`
Original file line number	Diff line number	Diff line change
`@@ -21,6 +21,9 @@ class ClassMember {`
`21`	`21`	`/** @type {Node} */`
`22`	`22`	`classDeclaration;`
`23`	`23`
	`24`	`+ /** @type {Node\|undefined} */`
	`25`	`+ initializer;`
	`26`	`+`
`24`	`27`	`/**`
`25`	`28`	`* @param {Node} node`
`26`	`29`	`* @param {SourceCode} sourceCode`
`@@ -44,6 +47,9 @@ class ClassMember {`
`44`	`47`	`classMembers.set(memberName, classMember);`
`45`	`48`	`}`
`46`	`49`	`classMember.references.add(node);`
	`50`	`+ if (node.type === 'AssignmentExpression') {`
	`51`	`+ classMember.initializer = node;`
	`52`	`+ }`
`47`	`53`	`return classMember;`
`48`	`54`	`}`
`49`	`55`	`}`
Original file line number	Diff line number	Diff line change
`@@ -7,9 +7,9 @@`
`7`	`7`	`'use strict';`
`8`	`8`
`9`	`9`	`const {isIdentifier} = require('./ast.js');`
`10`		`-const {DomFragment} = require('./dom-fragment.js');`
`11`	`10`
`12`	`11`	`/** @typedef {import('eslint').Rule.Node} Node */`
	`12`	`+/** @typedef {import('./dom-fragment.js').DomFragment} DomFragment */`
`13`	`13`
`14`	`14`	`module.exports = {`
`15`	`15`	`create : function(context) {`
`@@ -26,13 +26,12 @@ module.exports = {`
`26`	`26`	`methodCall(property, firstArg, secondArg, domFragment, call) {`
`27`	`27`	`if (isIdentifier(property, 'createChild')) {`
`28`	`28`	`if (firstArg?.type === 'Literal') {`
`29`		`- const childFragment = DomFragment.getOrCreate(call, sourceCode);`
	`29`	`+ const childFragment = domFragment.appendChild(call, sourceCode);`
`30`	`30`	`childFragment.tagName = String(firstArg.value);`
`31`		`- childFragment.parent = domFragment;`
`32`		`- domFragment.children.push(childFragment);`
`33`	`31`	`if (secondArg) {`
`34`	`32`	`childFragment.classList.push(secondArg);`
`35`	`33`	`}`
	`34`	`+ return true;`
`36`	`35`	`}`
`37`	`36`	`}`
`38`	`37`	`}`