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

RESOLVED FIXED in Firefox 42

Status

RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: bgrins, Assigned: gl)

Tracking

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

Firefox Tracking Flags

(firefox42 fixed)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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.
(Reporter)

Updated

3 years ago
Depends on: 1184628, 1186138
(Assignee)

Updated

3 years ago
Assignee: nobody → gabriel.luong
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

3 years ago
Created attachment 8638986 [details] [diff] [review]
1187443.patch [1.0]
Attachment #8638986 - Flags: review?(bgrinstead)
(Assignee)

Updated

3 years ago
Attachment #8638986 - Flags: review?(bgrinstead)
(Reporter)

Comment 2

3 years ago
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?
(Assignee)

Comment 3

3 years ago
(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.
(Reporter)

Comment 4

3 years ago
(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?
(Assignee)

Comment 5

3 years ago
(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.
(Reporter)

Comment 6

3 years ago
(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?
(Assignee)

Comment 7

3 years ago
(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.
(Reporter)

Updated

3 years ago
See Also: → bug 1188125
(Reporter)

Updated

3 years ago
Whiteboard: [rule-view-correctness]
(Assignee)

Updated

3 years ago
Summary: Rule becomes unapplied after clicking into the value editor → [Rule View] Remove preview value on start editing of a property
(Assignee)

Comment 8

3 years ago
Created attachment 8639740 [details] [diff] [review]
1187443.patch [2.0]
Attachment #8638986 - Attachment is obsolete: true
Attachment #8639740 - Flags: review?(bgrinstead)
(Reporter)

Comment 10

3 years ago
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
Last Resolved: 3 years ago
status-firefox42: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.