From a3fd937cbdeeb3391698e29efc910f28b0dd40b3 Mon Sep 17 00:00:00 2001 From: Patrick Brosset Date: Tue, 22 Sep 2020 00:55:35 -0700 Subject: [PATCH] Don't display grid adorners on shadow roots If an element in the Elements tree is a grid, it will get a grid adorner but there's a bug where if that element contains a shadow root, then it will also be getting the same adorner. To avoid this, we skip document fragment nodes in the code. Before: https://imgur.com/YU041Po.jpg After: https://imgur.com/qaFdQle.jpg Bug: 1129760 Change-Id: Ice4d9044b2a8a77a20d1db05ba4fb2acaa17775c Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2418171 Reviewed-by: Alex Rudenko Commit-Queue: Patrick Brosset --- front_end/elements/ElementsTreeElement.js | 3 +- test/e2e/elements/adornment_test.ts | 26 +++++++----- test/e2e/elements/layout-pane_test.ts | 12 +++--- test/e2e/helpers/elements-helpers.ts | 40 +++++++++++-------- test/e2e/resources/elements/BUILD.gn | 1 + .../resources/elements/adornment-shadow.html | 16 ++++++++ 6 files changed, 65 insertions(+), 33 deletions(-) create mode 100644 test/e2e/resources/elements/adornment-shadow.html diff --git a/front_end/elements/ElementsTreeElement.js b/front_end/elements/ElementsTreeElement.js index c335d4f88e..338d86057f 100644 --- a/front_end/elements/ElementsTreeElement.js +++ b/front_end/elements/ElementsTreeElement.js @@ -1951,7 +1951,8 @@ export class ElementsTreeElement extends UI.TreeOutline.TreeElement { const node = this.node(); const nodeId = node.id; - if (node.nodeType() === Node.COMMENT_NODE || nodeId === undefined) { + if (node.nodeType() === Node.COMMENT_NODE || node.nodeType() === Node.DOCUMENT_FRAGMENT_NODE || + nodeId === undefined) { return; } diff --git a/test/e2e/elements/adornment_test.ts b/test/e2e/elements/adornment_test.ts index f534e786d9..7cfb84245a 100644 --- a/test/e2e/elements/adornment_test.ts +++ b/test/e2e/elements/adornment_test.ts @@ -4,7 +4,7 @@ import {click, enableExperiment, goToResource} from '../../shared/helper.js'; import {describe, it} from '../../shared/mocha-extensions.js'; -import {assertActiveAdorners, assertInactiveAdorners, expandSelectedNodeRecursively, INACTIVE_GRID_ADORNER_SELECTOR, waitForContentOfSelectedElementsNode, waitForElementsStyleSection} from '../helpers/elements-helpers.js'; +import {expandSelectedNodeRecursively, INACTIVE_GRID_ADORNER_SELECTOR, waitForAdorners, waitForContentOfSelectedElementsNode, waitForElementsStyleSection} from '../helpers/elements-helpers.js'; const prepareElementsTab = async () => { await waitForElementsStyleSection(); @@ -18,20 +18,28 @@ describe('Adornment in the Elements Tab', async () => { await enableExperiment('cssGridFeatures'); await prepareElementsTab(); - await assertInactiveAdorners([ - 'grid', - 'grid', + await waitForAdorners([ + {textContent: 'grid', isActive: false}, + {textContent: 'grid', isActive: false}, ]); - await assertActiveAdorners([]); // Toggle both grid adorners on and try to select them with the active selector await click(INACTIVE_GRID_ADORNER_SELECTOR); await click(INACTIVE_GRID_ADORNER_SELECTOR); - await assertInactiveAdorners([]); - await assertActiveAdorners([ - 'grid', - 'grid', + await waitForAdorners([ + {textContent: 'grid', isActive: true}, + {textContent: 'grid', isActive: true}, + ]); + }); + + it('does not display adorners on shadow roots when their parents are grids', async () => { + await goToResource('elements/adornment-shadow.html'); + await enableExperiment('cssGridFeatures'); + await prepareElementsTab(); + + await waitForAdorners([ + {textContent: 'grid', isActive: false}, ]); }); }); diff --git a/test/e2e/elements/layout-pane_test.ts b/test/e2e/elements/layout-pane_test.ts index 898dd2e573..618a3d783e 100644 --- a/test/e2e/elements/layout-pane_test.ts +++ b/test/e2e/elements/layout-pane_test.ts @@ -4,7 +4,7 @@ import {enableExperiment, goToResource, step} from '../../shared/helper.js'; import {describe, it} from '../../shared/mocha-extensions.js'; -import {assertActiveAdorners, assertInactiveAdorners, expandSelectedNodeRecursively, openLayoutPane, toggleElementCheckboxInLayoutPane, waitForContentOfSelectedElementsNode, waitForElementsStyleSection} from '../helpers/elements-helpers.js'; +import {expandSelectedNodeRecursively, openLayoutPane, toggleElementCheckboxInLayoutPane, waitForAdorners, waitForContentOfSelectedElementsNode, waitForElementsStyleSection} from '../helpers/elements-helpers.js'; describe('Layout Pane in the Elements Tab', async function() { it('displays Layout pane', async () => { @@ -15,15 +15,13 @@ describe('Layout Pane in the Elements Tab', async function() { await waitForContentOfSelectedElementsNode('\u200B'); await expandSelectedNodeRecursively(); }); - await assertInactiveAdorners([ - 'grid', + await waitForAdorners([ + {textContent: 'grid', isActive: false}, ]); - await assertActiveAdorners([]); await openLayoutPane(); await toggleElementCheckboxInLayoutPane(); - await assertInactiveAdorners([]); - await assertActiveAdorners([ - 'grid', + await waitForAdorners([ + {textContent: 'grid', isActive: true}, ]); }); }); diff --git a/test/e2e/helpers/elements-helpers.ts b/test/e2e/helpers/elements-helpers.ts index 8e822c7ba0..473c5316cf 100644 --- a/test/e2e/helpers/elements-helpers.ts +++ b/test/e2e/helpers/elements-helpers.ts @@ -21,6 +21,7 @@ const CLS_PANE_SELECTOR = '.styles-sidebar-toolbar-pane'; const CLS_BUTTON_SELECTOR = '[aria-label="Element Classes"]'; const CLS_INPUT_SELECTOR = '[aria-placeholder="Add new class"]'; const LAYOUT_PANE_TAB_SELECTOR = '[aria-label="Layout"]'; +const ADORNER_SELECTOR = 'devtools-adorner'; export const INACTIVE_GRID_ADORNER_SELECTOR = '[aria-label="Enable grid mode"]'; export const ACTIVE_GRID_ADORNER_SELECTOR = '[aria-label="Disable grid mode"]'; const ELEMENT_CHECKBOX_IN_LAYOUT_PANE_SELECTOR = '.elements input[type=checkbox]'; @@ -32,23 +33,30 @@ export const openLayoutPane = async () => { }); }; -export const assertInactiveAdorners = async (expectedAdorners: string[]) => { - await step('Assert inactive adorners in Elements panel', async () => { - const actualAdorners = await $$(INACTIVE_GRID_ADORNER_SELECTOR); - const actualAdornersContent = await Promise.all(actualAdorners.map(n => n.evaluate(node => node.textContent))); - assert.deepEqual( - actualAdornersContent, expectedAdorners, - `did not have exactly ${expectedAdorners.length} adorner(s) in the inactive state`); - }); -}; +export const waitForAdorners = async (expectedAdorners: {textContent: string, isActive: boolean}[]) => { + await waitForFunction(async () => { + const actualAdorners = await $$(ADORNER_SELECTOR); + const actualAdornersStates = await Promise.all(actualAdorners.map(n => { + return n.evaluate((node, activeSelector: string) => { + return {textContent: node.textContent, isActive: node.matches(activeSelector)}; + }, ACTIVE_GRID_ADORNER_SELECTOR); + })); -export const assertActiveAdorners = async (expectedAdorners: string[]) => { - await step('Assert active adorners in Elements panel', async () => { - const actualAdorners = await $$(ACTIVE_GRID_ADORNER_SELECTOR); - const actualAdornersContent = await Promise.all(actualAdorners.map(n => n.evaluate(node => node.textContent))); - assert.deepEqual( - actualAdornersContent, expectedAdorners, - `did not have exactly ${expectedAdorners.length} adorner(s) in the inactive state`); + if (actualAdornersStates.length !== expectedAdorners.length) { + return false; + } + + for (let i = 0; i < actualAdornersStates.length; i++) { + const index = expectedAdorners.findIndex(expected => { + const actual = actualAdornersStates[i]; + return expected.textContent === actual.textContent && expected.isActive === actual.isActive; + }); + if (index !== -1) { + expectedAdorners.splice(index, 1); + } + } + + return expectedAdorners.length === 0; }); }; diff --git a/test/e2e/resources/elements/BUILD.gn b/test/e2e/resources/elements/BUILD.gn index 32c836dc78..395deba572 100644 --- a/test/e2e/resources/elements/BUILD.gn +++ b/test/e2e/resources/elements/BUILD.gn @@ -6,6 +6,7 @@ import("../../../../scripts/build/ninja/copy.gni") copy_to_gen("elements") { sources = [ + "adornment-shadow.html", "adornment.html", "css-grid.html", "css-variables.html", diff --git a/test/e2e/resources/elements/adornment-shadow.html b/test/e2e/resources/elements/adornment-shadow.html new file mode 100644 index 0000000000..601094a061 --- /dev/null +++ b/test/e2e/resources/elements/adornment-shadow.html @@ -0,0 +1,16 @@ + + + +
+