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)

Product:
DevTools
Component:
Inspector
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox42 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 42
Tracking Flags:
Tracking Status
firefox42 --- fixed

People

(Reporter: bgrins, Assigned: gl)

References

(
URL
)

Details

(Whiteboard: [rule-view-correctness])

Attachments

(1 file, 1 obsolete file)

1187443.patch [2.0]
7.86 KB, 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.
Depends on: 1184628, 1186138
Assignee: nobody → gabriel.luong
Status: NEW → ASSIGNED
Attached 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.

    
  
See Also:  → 1188125

    
  
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: 9 years ago

          status-firefox42:
          affected → fixed
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: