Closed
Bug 1187443
Opened 9 years ago
Closed 9 years ago
[Rule View] Remove preview value on start editing of a property
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(firefox42 fixed)
RESOLVED
FIXED
Firefox 42
| Tracking | Status | |
|---|---|---|
| firefox42 | --- | fixed |
People
(Reporter: bgrins, Assigned: gl)
References
()
Details
(Whiteboard: [rule-view-correctness])
Attachments
(1 file, 1 obsolete file)
|
(deleted),
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
|
Assignee | |
Updated•9 years ago
|
Assignee: nobody → gabriel.luong
|
|
Assignee | |
Updated•9 years ago
|
Status: NEW → ASSIGNED
|
|
Assignee | |
Comment 1•9 years ago
|
||
Attachment #8638986 -
Flags: review?(bgrinstead)
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8638986 -
Flags: review?(bgrinstead)
|
|
Reporter | |
Comment 2•9 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•9 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•9 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•9 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•9 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•9 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•9 years ago
|
|
|
Reporter | |
Updated•9 years ago
|
Whiteboard: [rule-view-correctness]
|
|
Assignee | |
Updated•9 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•9 years ago
|
||
Attachment #8638986 -
Attachment is obsolete: true
Attachment #8639740 -
Flags: review?(bgrinstead)
|
|
Assignee | |
Comment 9•9 years ago
|
||
|
|
Reporter | |
Comment 10•9 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+
|
||
Comment 12•9 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
|
||
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•