1186138 - [Rule View] Editing a disabled property and escaping re-enables the property

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

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

When we escape editing a disabled property, the property is re-enabled even though the property is crossed out.

Comment 1

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

Attachment: Part 1: [Rule View] Refactor this.prop.rule to this.rule [1.0]

Comment 2

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

Attachment: Part 1: [Rule View] Refactor this.prop.rule to this.rule [2.0]

Comment 3

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

Attachment: Part 1: Refactor this.prop.rule to this.rule [3.0]

Comment 4

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

Attachment: Part 2: Ensure the property remains disabled after ESC editing a disabled property [1.0]

Comment 5

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

Attachment: Part 3: Add unit test [1.0]

Comment 6

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

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

Comment 7

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

Attachment: Part 1: Refactor this.prop.rule to this.rule in rule-view.js [4.0]

Comment 8

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

Comment on attachment 8636849 [details] [diff] [review]
Part 2: Ensure the property remains disabled after ESC editing a disabled property [1.0]

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

::: browser/devtools/styleinspector/rule-view.js
@@ +3339,5 @@
>        if (this.removeOnRevert) {
>          this.remove();
>        } else {
> +        // Disable the property if the property was originally disabled.
> +        this.rule.setPropertyEnabled(this.prop, this.prop.enabled);

We could remove this.update here because this.update will be called after this._onValueDone from inplace-editor.js.

this.rule.setPropertyEnabled() will essentially perform previewPropertyValue()

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8637391 [details] [diff] [review]
Part 1: Refactor this.prop.rule to this.rule in rule-view.js [4.0]

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

The place where prop is getting reassigned in _updateTextProperty makes me a little suspicious of hardcoding `this.rule = this.prop.rule` in the constructor.  Seems like it may be safer to add a getter on the TPE object to return this.prop.rule

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #9)
> Comment on attachment 8637391 [details] [diff] [review]
> Part 1: Refactor this.prop.rule to this.rule in rule-view.js [4.0]
> 
> Review of attachment 8637391 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The place where prop is getting reassigned in _updateTextProperty makes me a
> little suspicious of hardcoding `this.rule = this.prop.rule` in the
> constructor.  Seems like it may be safer to add a getter on the TPE object
> to return this.prop.rule

But this was just at a glance, I haven't tracked down exactly what that code is doing so maybe it'd be fine after all

Comment 11

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

Attachment: Part 1: Refactor this.prop.rule to this.rule in rule-view.js [5.0]

I agree using a getter might be safer here.

Comment 12

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

Attachment: Part 2: Ensure the property remains disabled after ESC editing a disabled property [2.0]

Comment 13

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

Attachment: Part 3: Add unit test [2.0]

Comment 14

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

Attachment: Part 2: Ensure the property remains disabled after ESC editing a disabled property [3.0]

Comment 15

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

Attachment: Part 2: Ensure the property remains disabled after ESC editing a disabled property [4.0]

Comment 16

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

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

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8638400 [details] [diff] [review]
Part 2: Ensure the property remains disabled after ESC editing a disabled property [4.0]

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

::: browser/devtools/styleinspector/rule-view.js
@@ +3295,5 @@
> +    // Unlike the value editor, if a name is empty the entire property
> +    // should always be removed.
> +    if (aValue.trim() === "") {
> +      this.remove();
> +    } else {

Nit: could early return here also to reduce nesting

@@ +3348,5 @@
> +      val = parseSingleValue(parsedProperties.firstValue);
> +    }
> +
> +    if ((!aCommit && !this.ruleEditor.isEditing) ||
> +        (val && this.committed.value == val.value &&

What if there is multiple values but the value of this property hasn't changed?  It looks like in that case it would ignore the new values.

For example, old val: "100%", new val (pasted in): "100%; background: red;"

I think we'll need to split up the idea of re-disabling the property and removing the property if esc was pressed and removeOnRevert.

@@ -3335,5 @@
>        // A new property should be removed when escape is pressed.
>        if (this.removeOnRevert) {
>          this.remove();
>        } else {
> -        // update the editor back to committed value

I'm not even sure how pressing ESC is actually reverting anything right now with these lines gone but it seems to do so with the patch applied.

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8637569 [details] [diff] [review]
Part 3: Add unit test [2.0]

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

As discussed, I'm not sure if previewing disabled properties is even that useful of a thing, but given the context of Bug 1184628 where we will re-enable when starting an edit having test coverage for this is a step in the right direction

Comment 19

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

Attachment: Part 2: Ensure the property remains disabled after ESC editing a disabled property [5.0]

Comment 20

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

Attachment: Part 1: Refactor this.prop.rule to this.rule in rule-view.js [6.0]

Comment 21

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

Attachment: Part 2: Ensure the property remains disabled after ESC editing a disabled property [6.0]

Comment 22

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

Attachment: Part 3: Add unit test [3.0]

Comment 23

[Brian Grinstead [:bgrins]]

Comment on attachment 8639086 [details] [diff] [review]
Part 2: Ensure the property remains disabled after ESC editing a disabled property [6.0]

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

::: browser/devtools/styleinspector/rule-view.js
@@ +3361,5 @@
>     */
>    _onValueDone: function(aValue, aCommit) {
> +    let parsedProperties, val;
> +
> +    if (aValue !== undefined) {

In what case would aValue be undefined here?  I thought it would always pass a string in..

@@ +3367,5 @@
> +      val = parseSingleValue(parsedProperties.firstValue);
> +    }
> +
> +    if ((!aCommit && !this.ruleEditor.isEditing) ||
> +        (val && !parsedProperties.propertiesToAdd.length &&

Can you store this part of the OR into a separate variable with a descriptive name?

Comment 24

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

Attachment: Part 2: Ensure the property remains disabled after ESC editing a disabled property [7.0]

Comment 25

[Brian Grinstead [:bgrins]]

Comment on attachment 8639584 [details] [diff] [review]
Part 2: Ensure the property remains disabled after ESC editing a disabled property [7.0]

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

r=me with a green try push

Comment 26

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

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

Comment 27

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

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

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/49a2e0844098
https://hg.mozilla.org/integration/fx-team/rev/36ea982d46ab
https://hg.mozilla.org/integration/fx-team/rev/72b6857ff784

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/49a2e0844098
https://hg.mozilla.org/mozilla-central/rev/36ea982d46ab
https://hg.mozilla.org/mozilla-central/rev/72b6857ff784