Closed Bug 1186138 Opened 4 years ago Closed 4 years ago

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

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: gl, Assigned: gl)

References

Details

(Whiteboard: [rule-view-correctness])

Attachments

(3 files, 13 obsolete files)

2.49 KB, patch
Details | Diff | Splinter Review
3.89 KB, patch
Details | Diff | Splinter Review
4.82 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
When we escape editing a disabled property, the property is re-enabled even though the property is crossed out.
Assignee: nobody → gabriel.luong
Status: NEW → ASSIGNED
OS: Unspecified → All
Hardware: Unspecified → All
Attachment #8636751 - Attachment description: Part 1: [Rule View] Refactor this.prop.rule to this.rule → Part 1: [Rule View] Refactor this.prop.rule to this.rule [1.0]
Attachment #8636751 - Attachment is obsolete: true
Attachment #8636785 - Attachment is obsolete: true
Attached patch Part 3: Add unit test [1.0] (obsolete) — Splinter Review
Attachment #8637387 - Flags: review?(bgrinstead)
Attachment #8637387 - Attachment description: Part 3: Add unit test → Part 3: Add unit test [1.0]
Attachment #8636849 - Flags: review?(bgrinstead)
Attachment #8636848 - Flags: review?(bgrinstead)
Attachment #8636848 - Attachment is obsolete: true
Attachment #8636848 - Flags: review?(bgrinstead)
Attachment #8637391 - Flags: review?(bgrinstead)
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 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
Attachment #8637391 - Flags: review?(bgrinstead)
(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
I agree using a getter might be safer here.
Attachment #8637391 - Attachment is obsolete: true
Attachment #8637415 - Flags: review?(bgrinstead)
Blocks: 1184628
Attachment #8637415 - Flags: review?(bgrinstead) → review+
Attachment #8636849 - Attachment is obsolete: true
Attachment #8636849 - Flags: review?(bgrinstead)
Attachment #8637491 - Flags: review?(bgrinstead)
Attached patch Part 3: Add unit test [2.0] (obsolete) — Splinter Review
Attachment #8637387 - Attachment is obsolete: true
Attachment #8637387 - Flags: review?(bgrinstead)
Attachment #8637569 - Flags: review?(bgrinstead)
Attachment #8637491 - Attachment is obsolete: true
Attachment #8637491 - Flags: review?(bgrinstead)
Attachment #8638201 - Flags: review?(bgrinstead)
Attachment #8638201 - Flags: review?(bgrinstead)
Attachment #8637569 - Flags: review?(bgrinstead)
Attachment #8638201 - Attachment is obsolete: true
Attachment #8638400 - Flags: review?(bgrinstead)
Attachment #8637569 - Flags: review?(bgrinstead)
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 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
Attachment #8637569 - Flags: review?(bgrinstead) → review+
Attachment #8638400 - Attachment is obsolete: true
Attachment #8638400 - Flags: review?(bgrinstead)
Attachment #8638832 - Flags: review?(bgrinstead)
Attachment #8638832 - Attachment is obsolete: true
Attachment #8638832 - Flags: review?(bgrinstead)
Attachment #8639086 - Flags: review?(bgrinstead)
Attachment #8637569 - Attachment is obsolete: true
Attachment #8639087 - Flags: review+
See Also: → 1188125
Whiteboard: [rule-view-correctness]
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?
Attachment #8639086 - Attachment is obsolete: true
Attachment #8639086 - Flags: review?(bgrinstead)
Attachment #8639584 - Flags: review?(bgrinstead)
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
Attachment #8639584 - Flags: review?(bgrinstead) → review+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.