1184628 - [Rule View] Editing a disabled property should enable it

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Gabriel Luong [:gl] (ΦωΦ)]

When we are editing any property, we are previewing the changes. It is especially strange when editing a disabled property to see the previewed changes as well. Chrome currently will enable a modified property even if it was disabled as long as the new value != old value.

I propose we should do the following for close to chrome parity:
(1) Hide the checkbox for enabling/disabling property when editing the property
(2) Enable the property if the property was modified

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1184628.patch

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1184628.patch

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1184628.patch [1.0]

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=db768222d859

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1184628.patch [2.0]

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=75eea33e58e8

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1184628.patch [3.0]

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1184628.patch [4.0]

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1184628.patch [5.0]

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cde088c9b84c

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8639088 [details] [diff] [review]
1184628.patch [5.0]

Review of attachment 8639088 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, just a few notes

::: browser/devtools/styleinspector/test/browser_ruleview_edit-property_05.js
@@ +3,5 @@
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Tests that a disabled property is re-enable if the property name or value is

"re-enabled"

@@ +30,5 @@
> +  info("Disabling background-color property");
> +  propEditor.enable.click();
> +  yield ruleEditor.rule._applyingModifications;
> +
> +  let newValue = yield executeInContent("Test:GetRulePropertyValue", {

I'd extract this into a helper for this test just to make it a bit easier to read, something like:

function* getRulePropertyValue(name) {
  let newValue = yield executeInContent("Test:GetRulePropertyValue", {
    styleSheetIndex: 0,
    ruleIndex: 0,
    name: name
  });
  return newValue;
}

::: browser/devtools/styleinspector/test/browser_ruleview_edit-property_06.js
@@ +29,5 @@
> +  let target = getNode("body");
> +  let ruleEditor = getRuleViewRuleEditor(view, 1);
> +  let propEditor = ruleEditor.rule.textProps[0].editor;
> +
> +  is(content.getComputedStyle(target).backgroundColor, "rgb(0, 128, 0)",

Calls to content.getComputedStyle in all tests should be replaced with the message Test:GetComputedStylePropertyValue - https://dxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/test/doc_frame_script.js#85

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1184628.patch [6.0]

Comment 13

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a8da94cd768

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2316676f94d5

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2316676f94d5