Dom Adorner

This CL adds support to show multiple issues through squiggly lines in
the DOM tree of Elements Panel.

Screenshot:https://imgur.com/a/hKpaXd3 Explainer:
https://docs.google.com/document/d/1NIoIQdiSN72HVb4TosfE7tMHNTjEEjsCye_ymqoOXHw/edit?tab=t.0#heading=h.mymnss778pd7

Bug: 378738916
Change-Id: I0b37553c056b429cbac445bdbc35779af81ffdc9
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6249604
Reviewed-by: Philip Pfaffe <[email protected]>
Commit-Queue: Changhao Han <[email protected]>
Reviewed-by: Changhao Han <[email protected]>

Author: yanlingwang23

front_end/models/issues_manager/SelectElementAccessibilityIssue.ts

@@ -31,9 +31,7 @@ export class SelectElementAccessibilityIssue extends Issue {
   }
 
   getDescription(): MarkdownIssueDescription|null {
-    if (this.issueDetails.hasDisallowedAttributes &&
-        (this.issueDetails.selectElementAccessibilityIssueReason !==
-         Protocol.Audits.SelectElementAccessibilityIssueReason.InteractiveContentOptionChild)) {
+    if (this.isInteractiveContentAttributesSelectDescendantIssue()) {
       return {
         file: 'selectElementAccessibilityInteractiveContentAttributesSelectDescendant.md',
         links: [],
@@ -58,6 +56,12 @@ export class SelectElementAccessibilityIssue extends Issue {
     return this.issueDetails;
   }
 
+  isInteractiveContentAttributesSelectDescendantIssue(): boolean {
+    return this.issueDetails.hasDisallowedAttributes &&
+        (this.issueDetails.selectElementAccessibilityIssueReason !==
+         Protocol.Audits.SelectElementAccessibilityIssueReason.InteractiveContentOptionChild);
+  }
+
   static fromInspectorIssue(issuesModel: SDK.IssuesModel.IssuesModel, inspectorIssue: Protocol.Audits.InspectorIssue):
       SelectElementAccessibilityIssue[] {
     const selectElementAccessibilityIssueDetails = inspectorIssue.details.selectElementAccessibilityIssueDetails;

front_end/panels/elements/ElementIssueUtils.ts

@@ -67,6 +67,10 @@ const UIStrings = {
    * @description Tooltip text shown in the Elements panel when an element has an error.
    */
   interactiveContentLegendChild: 'Interactive element inside of a <legend> element',
+  /**
+   * @description Tooltip text shown in the Elements panel when an element has an error.
+   */
+  interactiveContentAttributesSelectDescendant: 'Element with invalid attributes within a <select> element',
 } as const;
 
 const str_ = i18n.i18n.registerUIStrings('panels/elements/ElementIssueUtils.ts', UIStrings);
@@ -89,6 +93,12 @@ export function getElementIssueDetails(issue: IssuesManager.Issue.Issue): Elemen
   }
   if (issue instanceof IssuesManager.SelectElementAccessibilityIssue.SelectElementAccessibilityIssue) {
     const issueDetails = issue.details();
+    if (issue.isInteractiveContentAttributesSelectDescendantIssue()) {
+      return {
+        tooltip: i18nString(UIStrings.interactiveContentAttributesSelectDescendant),
+        nodeId: issueDetails.nodeId,
+      };
+    }
     return {
       tooltip: getTooltipFromSelectElementAccessibilityIssue(issueDetails.selectElementAccessibilityIssueReason),
       nodeId: issueDetails.nodeId,

front_end/panels/elements/ElementsTreeElement.ts

@@ -264,7 +264,7 @@ export class ElementsTreeElement extends UI.TreeOutline.TreeElement {
   private aiButtonContainer?: HTMLElement;
   private contentElement: HTMLElement;
   #elementIssues = new Map<string, IssuesManager.Issue.Issue>();
-  #nodeElementToIssue = new Map<Element, IssuesManager.Issue.Issue>();
+  #nodeElementToIssue = new Map<Element, IssuesManager.Issue.Issue[]>();
   #highlights: Range[] = [];
 
   readonly tagTypeContext: TagTypeContext;
@@ -457,7 +457,7 @@ export class ElementsTreeElement extends UI.TreeOutline.TreeElement {
     }
   }
 
-  get issuesByNodeElement(): Map<Element, IssuesManager.Issue.Issue> {
+  get issuesByNodeElement(): Map<Element, IssuesManager.Issue.Issue[]> {
     return this.#nodeElementToIssue;
   }
 
@@ -468,15 +468,23 @@ export class ElementsTreeElement extends UI.TreeOutline.TreeElement {
       if (attribute.getElementsByClassName('webkit-html-attribute-name')[0].textContent === name) {
         const attributeElement = attribute.getElementsByClassName('webkit-html-attribute-name')[0];
         attributeElement.classList.add('violating-element');
-        this.#nodeElementToIssue.set(attributeElement, issue);
+        this.#updateNodeElementToIssue(attributeElement, issue);
       }
     }
   }
 
   #highlightTagAsViolating(issue: IssuesManager.Issue.Issue): void {
     const tagElement = this.listItemElement.getElementsByClassName('webkit-html-tag-name')[0];
     tagElement.classList.add('violating-element');
-    this.#nodeElementToIssue.set(tagElement, issue);
+    this.#updateNodeElementToIssue(tagElement, issue);
+  }
+
+  #updateNodeElementToIssue(nodeElement: Element, issue: IssuesManager.Issue.Issue): void {
+    if (!this.#nodeElementToIssue.has(nodeElement)) {
+      this.#nodeElementToIssue.set(nodeElement, [issue]);
+      return;
+    }
+    this.#nodeElementToIssue.get(nodeElement)?.push(issue);
   }
 
   expandedChildrenLimit(): number {

front_end/panels/elements/ElementsTreeOutline.test.ts

@@ -173,6 +173,30 @@ describeWithMockConnection('ElementsTreeOutline', () => {
       tagElement.classList.remove('violating-element');
     }
 
+    // Test that multiple issues being added to the tree element.
+    {
+      const inspectorIssue = {
+        code: Protocol.Audits.InspectorIssueCode.GenericIssue,
+        details: {
+          genericIssueDetails: {
+            errorType: Protocol.Audits.GenericIssueErrorType.FormEmptyIdAndNameAttributesForInputError,
+            frameId: 'main' as Protocol.Page.FrameId,
+            violatingNodeId: 2 as Protocol.DOM.BackendNodeId,
+          },
+        },
+      };
+      const issue = IssuesManager.GenericIssue.GenericIssue.fromInspectorIssue(mockModel, inspectorIssue)[0];
+      issuesManager.dispatchEventToListeners(
+          IssuesManager.IssuesManager.Events.ISSUE_ADDED, {issuesModel: mockModel, issue});
+      await deferredDOMNodeStub();
+      const tagElement = treeElement.listItemElement.getElementsByClassName('webkit-html-tag-name')[0];
+      assert.isTrue(tagElement.classList.contains('violating-element'));
+      const issues = treeElement.issuesByNodeElement.get(tagElement);
+      assert.strictEqual(issues?.length, 3);
+      // Reset tag to prepare for subsequent tests.
+      tagElement.classList.remove('violating-element');
+    }
+
     // Test that non-supported issue won't be added to the tree element.
     {
       const inspectorIssue = {

front_end/panels/elements/ElementsTreeOutline.ts

@@ -42,6 +42,7 @@ import * as CodeHighlighter from '../../ui/components/code_highlighter/code_high
 import * as IconButton from '../../ui/components/icon_button/icon_button.js';
 import * as IssueCounter from '../../ui/components/issue_counter/issue_counter.js';
 import * as UI from '../../ui/legacy/legacy.js';
+import {html, nothing, render} from '../../ui/lit/lit.js';
 import * as VisualLogging from '../../ui/visual_logging/visual_logging.js';
 
 import * as ElementsComponents from './components/components.js';
@@ -71,6 +72,10 @@ const UIStrings = {
    *@description Link text content in Elements Tree Outline of the Elements panel
    */
   reveal: 'reveal',
+  /**
+   * @description Text for popover that directs to Issues panel
+   */
+  viewIssue: 'View Issue:',
 } as const;
 const str_ = i18n.i18n.registerUIStrings('panels/elements/ElementsTreeOutline.ts', UIStrings);
 const i18nString = i18n.i18n.getLocalizedString.bind(undefined, str_);
@@ -106,7 +111,7 @@ export class ElementsTreeOutline extends
   #topLayerContainerByParent = new Map<UI.TreeOutline.TreeElement, TopLayerContainer>();
   #issuesManager?: IssuesManager.IssuesManager.IssuesManager;
   #popupHelper?: UI.PopoverHelper.PopoverHelper;
-  #nodeElementToIssue = new Map<Element, IssuesManager.Issue.Issue>();
+  #nodeElementToIssues = new Map<Element, IssuesManager.Issue.Issue[]>();
 
   constructor(omitRootDOMNode?: boolean, selectEnabled?: boolean, hideGutter?: boolean) {
     super();
@@ -194,40 +199,34 @@ export class ElementsTreeOutline extends
           return null;
         }
 
-        const issue = this.#nodeElementToIssue.get(hoveredNode);
-        if (!issue) {
-          return null;
-        }
-        const elementIssueDetails = getElementIssueDetails(issue);
-        if (!elementIssueDetails) {
+        const issues = this.#nodeElementToIssues.get(hoveredNode);
+        if (!issues) {
           return null;
         }
-        const issueKindIcon = new IconButton.Icon.Icon();
-        issueKindIcon.data = IssueCounter.IssueCounter.getIssueKindIconData(issue.getKind());
-        issueKindIcon.style.cursor = 'pointer';
-        const viewIssueElement = document.createElement('a');
-        viewIssueElement.href = '#';
-        viewIssueElement.textContent = 'View issue:';
-
-        const issueTitle = document.createElement('span');
-        issueTitle.textContent = elementIssueDetails.tooltip;
-
-        const element = document.createElement('div');
-        element.appendChild(issueKindIcon);
-        element.appendChild(viewIssueElement);
-        element.appendChild(issueTitle);
-        element.style.display = 'flex';
-        element.style.alignItems = 'center';
-        element.style.gap = '5px';
 
         return {
           box: hoveredNode.boxInWindow(),
           show: async (popover: UI.GlassPane.GlassPane) => {
             popover.setIgnoreLeftMargin(true);
-            const openIssueEvent = (): Promise<void> => Common.Revealer.reveal(issue);
-            viewIssueElement.addEventListener('click', () => openIssueEvent());
-            issueKindIcon.addEventListener('click', () => openIssueEvent());
-            popover.contentElement.appendChild(element);
+            // clang-format off
+            render(html`
+              <div class="squiggles-content">
+                ${issues.map(issue => {
+                  const elementIssueDetails = getElementIssueDetails(issue);
+                  if (!elementIssueDetails) {
+                    // This shouldn't happen, but add this if check to pass ts check.
+                    return nothing;
+                  }
+                  const issueKindIconData = IssueCounter.IssueCounter.getIssueKindIconData(issue.getKind());
+                  const openIssueEvent = (): Promise<void> => Common.Revealer.reveal(issue);
+                  return html`
+                    <div class="squiggles-content-item">
+                    <devtools-icon .data=${issueKindIconData} @click=${openIssueEvent}></devtools-icon>
+                    <x-link class="link" @click=${openIssueEvent}>${i18nString(UIStrings.viewIssue)}</x-link>
+                    <span>${elementIssueDetails.tooltip}</span>
+                    </div>`;})}
+              </div>`, popover.contentElement);
+            // clang-format on
             return true;
           },
         };
@@ -276,7 +275,7 @@ export class ElementsTreeOutline extends
       const treeElementNodeElementsToIssue = treeElement.issuesByNodeElement;
       // This element could be the treeElement tags name or an attribute.
       for (const [element, issue] of treeElementNodeElementsToIssue) {
-        this.#nodeElementToIssue.set(element, issue);
+        this.#nodeElementToIssues.set(element, issue);
       }
     }
   }

front_end/ui/legacy/popover.css

@@ -22,3 +22,19 @@
 .widget.has-padding {
   padding: 6px;
 }
+
+.squiggles-content {
+  display: flex;
+  flex-direction: column;
+  gap: 5px;
+}
+
+.squiggles-content-item {
+  display: flex;
+  align-items: center;
+  gap: 5px;
+
+  & devtools-icon {
+    cursor: pointer;
+  }
+}

front_end/ui/visual_logging/KnownContextValues.ts

@@ -1292,6 +1292,7 @@ export const knownContextValues = new Set([
   'elements.hide-element',
   'elements.image-preview',
   'elements.invalid-property-decl-popover',
+  'elements.issue',
   'elements.jump-to-style',
   'elements.layout',
   'elements.length-popover',

test/e2e/elements/violating-element-and-attributes_test.ts

@@ -4,7 +4,7 @@
 
 import {assert} from 'chai';
 
-import {click, enableExperiment, waitFor, waitForMany} from '../../shared/helper.js';
+import {click, enableExperiment, goToResource, waitFor, waitForMany} from '../../shared/helper.js';
 import {expandSelectedNodeRecursively, goToResourceAndWaitForStyleSection} from '../helpers/elements-helpers.js';
 
 /**
@@ -53,3 +53,64 @@ describe.skip('[crbug.com/1399414]: Element has violating properties', function(
     assert.strictEqual(issueTitle, 'A form field element should have an id or name attribute');
   });
 });
+
+describe('The elements panel', function() {
+  beforeEach(async function() {
+    await enableExperiment('highlight-errors-elements-panel');
+    await goToResource('elements/select-element-with-issues.html');
+    await expandSelectedNodeRecursively();
+  });
+
+  it('displays squiggly lines on elements with issue', async () => {
+    const elements = await waitForMany('.violating-element', 2);
+    const violatingElement = await elements[0].evaluate(node => node.textContent);
+    assert.strictEqual(violatingElement, 'button');
+  });
+
+  it('displays multiple issues when hoverring over squiggly line', async () => {
+    const elements = await waitForMany('.violating-element', 2);
+    const violatingElement = elements[1];
+    await violatingElement.hover();
+    const popupParent = await waitFor('div.vbox.flex-auto.no-pointer-events');
+    const popupText = await popupParent.evaluate(async (node: Element) => {
+      if (!node.shadowRoot) {
+        throw new Error('Node shadow root not found.');
+      }
+      const popup = node.shadowRoot.querySelector('div.widget.has-padding');
+      if (!popup) {
+        throw new Error('Popup not found.');
+      }
+      const spans = popup.querySelectorAll('span');
+      return Array.from(spans).map(span => span.textContent);
+    });
+    assert.strictEqual(popupText[0], 'Element with invalid attributes within a <select> element');
+    assert.strictEqual(popupText[1], 'No label associated with a form field');
+  });
+
+  it('navigates to issues panel when hovering over problematic element', async () => {
+    const elements = await waitForMany('.violating-element', 2);
+    const violatingElement = elements[0];
+    await violatingElement.hover();
+    const popupParent = await waitFor('div.vbox.flex-auto.no-pointer-events');
+    const popupText = await popupParent.evaluate(async (node: Element) => {
+      if (!node.shadowRoot) {
+        throw new Error('Node shadow root not found.');
+      }
+      const popup = node.shadowRoot.querySelector('div.widget.has-padding');
+      if (!popup) {
+        throw new Error('Popup not found.');
+      }
+      const span = popup.querySelector('span');
+      if (!span) {
+        throw new Error('Span not found.');
+      }
+      return span.textContent;
+    });
+
+    assert.strictEqual(popupText, 'Interactive element inside of a <legend> element');
+    await click('div.widget.has-padding x-link');
+    const highlitedIssue = await waitFor('.issue .header .title');
+    const issueTitle = await highlitedIssue.evaluate(async (node: Element) => node.textContent);
+    assert.strictEqual(issueTitle, 'Interactive element inside of a <legend> element');
+  });
+});

test/e2e/resources/elements/BUILD.gn

@@ -54,6 +54,7 @@ copy_to_gen("elements") {
     "page-error.html",
     "quirks-mode-iframes.html",
     "quirks-mode.html",
+    "select-element-with-issues.html",
     "selection-after-delete.html",
     "shadow-dom-modify-chardata.html",
     "shadow-roots.html",

test/e2e/resources/elements/select-element-with-issues.html

@@ -0,0 +1,26 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <style>
+      select, ::picker(select) {
+        appearance: base-select;
+      }
+    </style>
+  </head>
+  <body>
+    <select id="interactive-content-legend-child">
+        <optgroup>
+            <legend>
+              <button></button>
+            </legend>
+        </optgroup>
+    </select>
+    <select id="disallowed-select-child">
+      <optgroup>
+        <legend>
+          <label tabindex="1"></label>
+        </legend>
+      </optgroup>
+    </select>
+  </body>
+</html>