[Rule View] Remove preview value on start editing of a property

RESOLVED FIXED in Firefox 42

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: bgrins, Assigned: gl)

Tracking

Trunk
Firefox 42
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed)

Details

(Whiteboard: [rule-view-correctness], )

Attachments

(1 attachment, 1 obsolete attachment)

Load this URL: data:text/html,<style> body {background: green !important } body {background: red}</style><body></body>

Inspect the <body> element
Click into the 'green' background value

Notice that the background turns red when you click into it (like it's unapplying the rule).  Clicking into a rule editor should *not* do that.
Assignee: nobody → gabriel.luong
Status: NEW → ASSIGNED
Posted patch 1187443.patch [1.0] (obsolete) — Splinter Review
Attachment #8638986 - Flags: review?(bgrinstead)
Attachment #8638986 - Flags: review?(bgrinstead)
Comment on attachment 8638986 [details] [diff] [review]
1187443.patch [1.0]

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

::: browser/devtools/styleinspector/rule-view.js
@@ +3161,5 @@
>      this.ruleView._updatePropertyHighlight(this);
>    },
>  
>    _onStartEditing: function() {
> +    this._previewValue(this.valueSpan.textContent);

I don't actually get why we need to previewValue at all here. Shouldn't the value already be applied when editing starts?
(In reply to Brian Grinstead [:bgrins] from comment #2)
> Comment on attachment 8638986 [details] [diff] [review]
> 1187443.patch [1.0]
> 
> Review of attachment 8638986 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/styleinspector/rule-view.js
> @@ +3161,5 @@
> >      this.ruleView._updatePropertyHighlight(this);
> >    },
> >  
> >    _onStartEditing: function() {
> > +    this._previewValue(this.valueSpan.textContent);
> 
> I don't actually get why we need to previewValue at all here. Shouldn't the
> value already be applied when editing starts?

Ya, I don't think we actually need previewValue at all here. I would propose that we only preview the value if the property is disabled.
(In reply to Gabriel Luong [:gl] from comment #3)
> (In reply to Brian Grinstead [:bgrins] from comment #2)
> > Comment on attachment 8638986 [details] [diff] [review]
> > 1187443.patch [1.0]
> > 
> > Review of attachment 8638986 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/devtools/styleinspector/rule-view.js
> > @@ +3161,5 @@
> > >      this.ruleView._updatePropertyHighlight(this);
> > >    },
> > >  
> > >    _onStartEditing: function() {
> > > +    this._previewValue(this.valueSpan.textContent);
> > 
> > I don't actually get why we need to previewValue at all here. Shouldn't the
> > value already be applied when editing starts?
> 
> Ya, I don't think we actually need previewValue at all here. I would propose
> that we only preview the value if the property is disabled.

Maybe we won't even need to do that after Bug 1184628?
(In reply to Brian Grinstead [:bgrins] from comment #4)
> (In reply to Gabriel Luong [:gl] from comment #3)
> > (In reply to Brian Grinstead [:bgrins] from comment #2)
> > > Comment on attachment 8638986 [details] [diff] [review]
> > > 1187443.patch [1.0]
> > > 
> > > Review of attachment 8638986 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: browser/devtools/styleinspector/rule-view.js
> > > @@ +3161,5 @@
> > > >      this.ruleView._updatePropertyHighlight(this);
> > > >    },
> > > >  
> > > >    _onStartEditing: function() {
> > > > +    this._previewValue(this.valueSpan.textContent);
> > > 
> > > I don't actually get why we need to previewValue at all here. Shouldn't the
> > > value already be applied when editing starts?
> > 
> > Ya, I don't think we actually need previewValue at all here. I would propose
> > that we only preview the value if the property is disabled.
> 
> Maybe we won't even need to do that after Bug 1184628?

Bug 1184628 would enable the property after editing, but we can either choose to enable the disabled preview or not. As a reference, Chrome currently does not preview disabled properties at the start of editing.
(In reply to Gabriel Luong [:gl] from comment #5)
> (In reply to Brian Grinstead [:bgrins] from comment #4)
> > (In reply to Gabriel Luong [:gl] from comment #3)
> > > (In reply to Brian Grinstead [:bgrins] from comment #2)
> > > > Comment on attachment 8638986 [details] [diff] [review]
> > > > 1187443.patch [1.0]
> > > > 
> > > > Review of attachment 8638986 [details] [diff] [review]:
> > > > -----------------------------------------------------------------
> > > > 
> > > > ::: browser/devtools/styleinspector/rule-view.js
> > > > @@ +3161,5 @@
> > > > >      this.ruleView._updatePropertyHighlight(this);
> > > > >    },
> > > > >  
> > > > >    _onStartEditing: function() {
> > > > > +    this._previewValue(this.valueSpan.textContent);
> > > > 
> > > > I don't actually get why we need to previewValue at all here. Shouldn't the
> > > > value already be applied when editing starts?
> > > 
> > > Ya, I don't think we actually need previewValue at all here. I would propose
> > > that we only preview the value if the property is disabled.
> > 
> > Maybe we won't even need to do that after Bug 1184628?
> 
> Bug 1184628 would enable the property after editing, but we can either
> choose to enable the disabled preview or not. As a reference, Chrome
> currently does not preview disabled properties at the start of editing.

So they don't preview right when you click into the editor but then begin to preview as you make changes?  That seems fine.

I'm in favor of never calling _previewValue at the beginning of editing for now.  Is that what you were thinking too?
(In reply to Brian Grinstead [:bgrins] from comment #6)
> (In reply to Gabriel Luong [:gl] from comment #5)
> > (In reply to Brian Grinstead [:bgrins] from comment #4)
> > > (In reply to Gabriel Luong [:gl] from comment #3)
> > > > (In reply to Brian Grinstead [:bgrins] from comment #2)
> > > > > Comment on attachment 8638986 [details] [diff] [review]
> > > > > 1187443.patch [1.0]
> > > > > 
> > > > > Review of attachment 8638986 [details] [diff] [review]:
> > > > > -----------------------------------------------------------------
> > > > > 
> > > > > ::: browser/devtools/styleinspector/rule-view.js
> > > > > @@ +3161,5 @@
> > > > > >      this.ruleView._updatePropertyHighlight(this);
> > > > > >    },
> > > > > >  
> > > > > >    _onStartEditing: function() {
> > > > > > +    this._previewValue(this.valueSpan.textContent);
> > > > > 
> > > > > I don't actually get why we need to previewValue at all here. Shouldn't the
> > > > > value already be applied when editing starts?
> > > > 
> > > > Ya, I don't think we actually need previewValue at all here. I would propose
> > > > that we only preview the value if the property is disabled.
> > > 
> > > Maybe we won't even need to do that after Bug 1184628?
> > 
> > Bug 1184628 would enable the property after editing, but we can either
> > choose to enable the disabled preview or not. As a reference, Chrome
> > currently does not preview disabled properties at the start of editing.
> 
> So they don't preview right when you click into the editor but then begin to
> preview as you make changes?  That seems fine.
> 
> I'm in favor of never calling _previewValue at the beginning of editing for
> now.  Is that what you were thinking too?

Yes, they only begin to preview as you make changes. My question was more geared towards whether or not we should limit the preview to only disabled properties when the editor is clicked, but yes I am in favor of never calling _previewValue in onStartEditing.
Whiteboard: [rule-view-correctness]
Summary: Rule becomes unapplied after clicking into the value editor → [Rule View] Remove preview value on start editing of a property
Attachment #8638986 - Attachment is obsolete: true
Attachment #8639740 - Flags: review?(bgrinstead)
Comment on attachment 8639740 [details] [diff] [review]
1187443.patch [2.0]

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

nice
Attachment #8639740 - Flags: review?(bgrinstead) → review+
https://hg.mozilla.org/mozilla-central/rev/aa144e36fab0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.