From dfb8d25b1dad481297cff61bb94e9f783b305695 Mon Sep 17 00:00:00 2001 From: Julian Descottes Date: Fri, 22 Apr 2022 08:17:02 +0000 Subject: [PATCH] Bug 1750446 - [devtools] Wait for the proper mutation in browser_rules_custom.js r=nchevobbe We usually need three mutations to reach the final state of addProperty and usually the 2nd and 3rd come in the same batch. Meaning that simply waiting for receivedMutations >= 2 works in most cases. But the intermittent screenshots show that the markup view was not updated yet with the correct content meaning it either didn't receive or processed the last mutation. Let's try to wait for the proper mutation instead of using an arbitrary number. Hopefully this fixes it. Differential Revision: https://phabricator.services.mozilla.com/D144181 --- .../browser_rules_add-property-cancel_02.js | 5 ++- .../test/browser_rules_edit-property_02.js | 2 +- devtools/client/inspector/rules/test/head.js | 35 ++++++++++++------- 3 files changed, 28 insertions(+), 14 deletions(-) diff --git a/devtools/client/inspector/rules/test/browser_rules_add-property-cancel_02.js b/devtools/client/inspector/rules/test/browser_rules_add-property-cancel_02.js index 5c7c039b4615..cc2419997e19 100644 --- a/devtools/client/inspector/rules/test/browser_rules_add-property-cancel_02.js +++ b/devtools/client/inspector/rules/test/browser_rules_add-property-cancel_02.js @@ -20,7 +20,10 @@ add_task(async function() { await selectNode("#testid", inspector); info("Test creating a new property and escaping"); - await addProperty(view, 1, "color", "red", "VK_ESCAPE", false); + await addProperty(view, 1, "color", "red", { + commitValueWith: "VK_ESCAPE", + blurNewProperty: false, + }); is( view.styleDocument.activeElement, diff --git a/devtools/client/inspector/rules/test/browser_rules_edit-property_02.js b/devtools/client/inspector/rules/test/browser_rules_edit-property_02.js index b53ab7e68b1c..979c779ec6b4 100644 --- a/devtools/client/inspector/rules/test/browser_rules_edit-property_02.js +++ b/devtools/client/inspector/rules/test/browser_rules_edit-property_02.js @@ -94,7 +94,7 @@ async function testEditProperty(inspector, ruleView) { is(newValue, "red", "border-color should have been set."); ruleView.styleDocument.activeElement.blur(); - await addProperty(ruleView, 1, "color", "red", ";"); + await addProperty(ruleView, 1, "color", "red", { commitValueWith: ";" }); const props = ruleView.element.querySelectorAll(".ruleview-property"); for (let i = 0; i < props.length; i++) { diff --git a/devtools/client/inspector/rules/test/head.js b/devtools/client/inspector/rules/test/head.js index 59827260f415..8f1fd11d9d67 100644 --- a/devtools/client/inspector/rules/test/head.js +++ b/devtools/client/inspector/rules/test/head.js @@ -240,11 +240,12 @@ var openCubicBezierAndChangeCoords = async function( * The name for the new property * @param {String} value * The value for the new property - * @param {String} commitValueWith + * @param {Object=} options + * @param {String=} options.commitValueWith * Which key should be used to commit the new value. VK_RETURN is used by * default, but tests might want to use another key to test cancelling * for exemple. - * @param {Boolean} blurNewProperty + * @param {Boolean=} options.blurNewProperty * After the new value has been added, a new property would have been * focused. This parameter is true by default, and that causes the new * property to be blurred. Set to false if you don't want this. @@ -255,8 +256,7 @@ var addProperty = async function( ruleIndex, name, value, - commitValueWith = "VK_RETURN", - blurNewProperty = true + { commitValueWith = "VK_RETURN", blurNewProperty = true } = {} ) { info("Adding new property " + name + ":" + value + " to rule " + ruleIndex); @@ -265,21 +265,32 @@ var addProperty = async function( const numOfProps = ruleEditor.rule.textProps.length; const onMutations = new Promise(r => { - // If we're adding the property to a non-element style rule, we don't need to wait - // for mutations. + // If the rule index is 0, then we are updating the rule for the "element" + // selector in the rule view. + // This rule is actually updating the style attribute of the element, and + // therefore we can expect mutations. + // For any other rule index, no mutation should be created, we can resolve + // immediately. if (ruleIndex !== 0) { r(); } - // Otherwise, adding the property to the element style rule causes 2 mutations to the - // style attribute on the element: first when the name is added with an empty value, - // and then when the value is added. - let receivedMutations = 0; + // Use CSS.escape for the name in order to match the logic at + // devtools/client/fronts/inspector/rule-rewriter.js + // This leads to odd values in the style attribute and might change in the + // future. See https://bugzilla.mozilla.org/show_bug.cgi?id=1765943 + const expectedAttributeValue = `${CSS.escape(name)}: ${value}`; view.inspector.walker.on("mutations", function onWalkerMutations( mutations ) { - receivedMutations += mutations.length; - if (receivedMutations >= 2) { + // Wait until we receive a mutation which updates the style attribute + // with the expected value. + const receivedLastMutation = mutations.some( + mut => + mut.attributeName === "style" && + mut.newValue.includes(expectedAttributeValue) + ); + if (receivedLastMutation) { view.inspector.walker.off("mutations", onWalkerMutations); r(); }