From d5a3755f19aa248cffbcf61f1628362aaa6c0377 Mon Sep 17 00:00:00 2001 From: Changhao Han Date: Thu, 26 Mar 2020 01:17:06 +0100 Subject: [PATCH] Adds check to ensure style pane update Deleting the last property in a styles section incorrectly skips style pane update. This check ensures style pane update when the property's index is out-of-bound. Bug: chromium:1060267 Change-Id: Ie6f6a436c789745bcd1f5ece6162cc4ada82a5b3 Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2107525 Commit-Queue: Changhao Han Reviewed-by: Mathias Bynens Reviewed-by: Tim van der Lippe --- .../elements/StylePropertyTreeElement.js | 6 +- test/e2e/elements/BUILD.gn | 1 + test/e2e/elements/style-pane-properties.ts | 81 +++++++++++++++++++ test/e2e/helpers/elements-helpers.ts | 16 ++++ .../elements/style-pane-properties.html | 13 +++ test/e2e/test-list.ts | 1 + 6 files changed, 117 insertions(+), 1 deletion(-) create mode 100644 test/e2e/elements/style-pane-properties.ts create mode 100644 test/e2e/resources/elements/style-pane-properties.html diff --git a/front_end/elements/StylePropertyTreeElement.js b/front_end/elements/StylePropertyTreeElement.js index 9598734d1d..3296c63ccb 100644 --- a/front_end/elements/StylePropertyTreeElement.js +++ b/front_end/elements/StylePropertyTreeElement.js @@ -1151,8 +1151,12 @@ export class StylePropertyTreeElement extends UI.TreeOutline.TreeElement { } this._parentPane.setUserOperation(false); + // TODO: using this.property.index to access its containing StyleDeclaration's property will result in + // off-by-1 errors when the containing StyleDeclaration's respective property has already been deleted. + // These referencing logic needs to be updated to be more robust. const updatedProperty = property || this._style.propertyAt(this.property.index); - if (!success || !updatedProperty) { + const isPropertyWithinBounds = this.property.index < this._style.allProperties().length; + if (!success || (!updatedProperty && isPropertyWithinBounds)) { if (majorChange) { // It did not apply, cancel editing. if (this._newProperty) { diff --git a/test/e2e/elements/BUILD.gn b/test/e2e/elements/BUILD.gn index 05487c1bfc..2d6c7217a6 100644 --- a/test/e2e/elements/BUILD.gn +++ b/test/e2e/elements/BUILD.gn @@ -9,6 +9,7 @@ ts_library("elements") { sources = [ "pseudo-states.ts", "shadowroot-styles.ts", + "style-pane-properties.ts", ] deps = [ diff --git a/test/e2e/elements/style-pane-properties.ts b/test/e2e/elements/style-pane-properties.ts new file mode 100644 index 0000000000..5f4dfec2e6 --- /dev/null +++ b/test/e2e/elements/style-pane-properties.ts @@ -0,0 +1,81 @@ +// Copyright 2020 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import {assert} from 'chai'; +import {describe, it} from 'mocha'; +import * as puppeteer from 'puppeteer'; + +import {$, click, getBrowserAndPages, resetPages, resourcesPath, waitFor} from '../../shared/helper.js'; +import {assertContentOfSelectedElementsNode, getAriaLabelSelectorFromPropertiesSelector, getDisplayedCSSPropertyNames, waitForElementsStyleSection} from '../helpers/elements-helpers.js'; + +const PROPERTIES_TO_DELETE_SELECTOR = '#properties-to-delete'; +const FIRST_PROPERTY_NAME_SELECTOR = '.tree-outline li:nth-of-type(1) > .webkit-css-property'; +const SECOND_PROPERTY_VALUE_SELECTOR = '.tree-outline li:nth-of-type(2) > .value'; + +const deletePropertyByBackspace = async (selector: string, root?: puppeteer.JSHandle) => { + const {frontend} = getBrowserAndPages(); + await click(selector, {root}); + await frontend.keyboard.press('Backspace'); + await frontend.keyboard.press('Tab'); +}; + +describe('The Elements Tab', async () => { + beforeEach(async () => { + await resetPages(); + }); + + it('can remove a CSS property when its name or value is deleted', async () => { + const {target, frontend} = getBrowserAndPages(); + + await target.goto(`${resourcesPath}/elements/style-pane-properties.html`); + await click('#tab-elements'); + + // Sanity check to make sure we have the correct node selected after opening a file + await assertContentOfSelectedElementsNode('\u200B'); + + // Select div that we will remove the CSS properties from + await frontend.keyboard.press('ArrowRight'); + await assertContentOfSelectedElementsNode('
\u200B
\u200B'); + await waitForElementsStyleSection(); + { + const displayedNames = await getDisplayedCSSPropertyNames(PROPERTIES_TO_DELETE_SELECTOR); + assert.deepEqual( + displayedNames, + [ + 'height', + 'width', + ], + 'incorrectly displayed style after initialization'); + } + + const propertiesSection = await $(getAriaLabelSelectorFromPropertiesSelector(PROPERTIES_TO_DELETE_SELECTOR)); + // select second property's value and delete + await deletePropertyByBackspace(SECOND_PROPERTY_VALUE_SELECTOR, propertiesSection); + await waitForElementsStyleSection(); + await waitFor('.tree-outline .child-editing', propertiesSection); + + // verify the second CSS property entry has been removed + { + const displayedNames = await getDisplayedCSSPropertyNames(PROPERTIES_TO_DELETE_SELECTOR); + assert.deepEqual( + displayedNames, + [ + 'height', + ], + 'incorrectly displayed style after removing second property\'s value'); + } + + // select first property's name and delete + await deletePropertyByBackspace(FIRST_PROPERTY_NAME_SELECTOR, propertiesSection); + await waitForElementsStyleSection(); + await waitFor('.tree-outline .child-editing', propertiesSection); + + // verify the first CSS property entry has been removed + { + await waitForElementsStyleSection(); + const displayedValues = await getDisplayedCSSPropertyNames(PROPERTIES_TO_DELETE_SELECTOR); + assert.deepEqual(displayedValues, [], 'incorrectly displayed style after removing first property\'s name'); + } + }); +}); diff --git a/test/e2e/helpers/elements-helpers.ts b/test/e2e/helpers/elements-helpers.ts index 3cb8ef091c..ebf4896817 100644 --- a/test/e2e/helpers/elements-helpers.ts +++ b/test/e2e/helpers/elements-helpers.ts @@ -6,6 +6,7 @@ import {assert} from 'chai'; import {$, $$, click, getBrowserAndPages, waitFor} from '../../shared/helper.js'; const SELECTED_TREE_ELEMENT_SELECTOR = '.selected[role="treeitem"]'; +const CSS_PROPERTY_NAME_SELECTOR = '.webkit-css-property'; export const assertContentOfSelectedElementsNode = async (expectedTextContent: string) => { const selectedNode = await $(SELECTED_TREE_ELEMENT_SELECTOR); @@ -78,3 +79,18 @@ export const getDisplayedEventListenerNames = async(): Promise => { }); return eventListenerNames; }; + +export const getAriaLabelSelectorFromPropertiesSelector = (selectorForProperties: string) => + `[aria-label="${selectorForProperties}, css selector"]`; + +export const getDisplayedCSSPropertyNames = async (selectorForProperties: string) => { + const listNodesContent = (nodes: Element[]) => { + const rawContent = nodes.map(node => node.textContent); + const filteredContent = rawContent.filter(content => !!content); + return filteredContent; + }; + const propertiesSection = await $(getAriaLabelSelectorFromPropertiesSelector(selectorForProperties)); + const cssPropertyNames = await $$(CSS_PROPERTY_NAME_SELECTOR, propertiesSection); + const propertyNamesText = await cssPropertyNames.evaluate(listNodesContent); + return propertyNamesText; +}; diff --git a/test/e2e/resources/elements/style-pane-properties.html b/test/e2e/resources/elements/style-pane-properties.html new file mode 100644 index 0000000000..bff0f4341c --- /dev/null +++ b/test/e2e/resources/elements/style-pane-properties.html @@ -0,0 +1,13 @@ + + + +
diff --git a/test/e2e/test-list.ts b/test/e2e/test-list.ts index 15f6d8b0f8..7399c1b999 100644 --- a/test/e2e/test-list.ts +++ b/test/e2e/test-list.ts @@ -12,6 +12,7 @@ const tests = [ 'elements/pseudo-states.js', 'elements/shadowroot-styles.js', 'elements/sidebar-event-listeners.js', + 'elements/style-pane-properties.js', 'host/user-metrics.js', 'network/network-datagrid.js', 'rendering/vision-deficiencies.js',