707940 - Rule view should maintain user entered values

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Kevin Dangoor]

In bug 703643, I asked for the ability to copy rules out of the rules view, something I saw in a few user studies.

As part of that, the value that a user enters should be maintained so that they can enter exactly what they want to appear in their stylesheet and then copy the result.

As an example, the rule view expands "background: yellow" into 'background: none repeat scroll 0% 0% yellow', which is inconvenient both for tweaking the value further and for copy/paste.

Comment 1

[Dave Camp (:dcamp)]

The style system doesn't store the user-entered value from parsed stylesheets.  Two additional notes:

* dbaron mentioned that the style system could coalescing shorthand properties differently - there's no reason cssText can't return 'yellow' instead of 'none repeat scroll 0% 0% yellow'[1].  That's probably better behavior, but still not what you're asking for here (if a developer enters 'none repeat scroll 0% 0% yellow' they'll still get 'yellow' back).

* We could at least remember what users enter in the rule view.  The rule view currently errs on the side of repeating back what the style system interpreted, but we could reconsider that.

[1] That's allowed by spec, and webkit's implementation does it.

Comment 2

[Kevin Dangoor]

Web Inspector maintains what the user entered (which maximizes the copy/paste-ability of the rules). For example, I made a stylesheet and then changed it a couple of times:

* background: none repeat scroll 0% 0% yellow;
* background: yellow;
* background: borscht repeat scroll 0% 0% yellow;

all show exactly what I entered. The last one is displayed with a little warning icon and crossed out.

For those same three variations, the rules view shows:

* background: none repeat scroll 0% 0% yellow;
* background: none repeat scroll 0% 0% yellow;
* (nothing)

Firebug behaves like this as well.

Arguably, changes that the user wants to save should be made in the Style Editor, but there may still be good reasons for being able to copy/paste a rule (working with a minified site or saving a rule to a LESS file).

Maintaining entries with errors and displaying an error message (the third case above) should probably be a separate bug, right?

Comment 3

[Dave Camp (:dcamp)]

(In reply to Kevin Dangoor from comment #2)
> Web Inspector maintains what the user entered (which maximizes the
> copy/paste-ability of the rules). For example, I made a stylesheet and then
> changed it a couple of times:

You mean "what the user entered in the rule view", right?  That's doable, as mentioned in my second bullet.  "what the user typed in their stylesheet that was loaded in the page" is more difficult.

Comment 4

[Kevin Dangoor]

(In reply to Dave Camp (:dcamp) from comment #3)
> (In reply to Kevin Dangoor from comment #2)
> > Web Inspector maintains what the user entered (which maximizes the
> > copy/paste-ability of the rules). For example, I made a stylesheet and then
> > changed it a couple of times:
> 
> You mean "what the user entered in the rule view", right?  That's doable, as
> mentioned in my second bullet.  "what the user typed in their stylesheet
> that was loaded in the page" is more difficult.

What I meant was that I had changed it in my text editor, not the rule view. Web Inspector displays what the user entered, no matter where they entered it.

Firebug appears to implement the "what the user entered in the rule view" solution (your second bullet, the easier case).

Since there's a difference in difficulty, we could leave this bug focused on the easier solution and have a separate bug for keeping track of what was originally in the user's style sheet.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch

The single line fix for this issue. Validation of the entered values will be done in bug 727867.

Comment 6

[Mihai Sucan [:msucan]]

The "updated purpose" of this bug and of the patch is not clear to me, from reading the comments, and from testing what it does.

While I understand the problem initially reported and the two proposed solutions (one being easier than the other), it's still not clear how far things need to go.

The implementation in the patch is limited to not updating the property value - it doesn't read it back from the platform code. However, when I inspect a different element and I come back, the property value I typed is not remembered - I get what Gecko gives out.

Chrome's Web Inspector remembers the value after inspecting other elements, even after reopen/close. Do we want to do this here and now? Depending on complexity, I would say yes. Mike, do you know how hard that would be? There's not too much value in a patch that simply doesn't update the property value in the current rule view after I press Enter (after I'm done editing). I don't think we need to go as far as remembering between Inspector open/close cycles, but we need to, at least, remember what the user typed after he inspects other elements and comes back to the same element (or to the same rule) - if we want to consider this bug as fixed.

In most cases designers change a number of properties in different rules, for different elements, in one go, then they copy/paste the updated properties to their stylesheets - they tend to make "atomic" changes. This is what I've seen my twin brother actually do with Firebug: he does a number of changes then, when he's happy with the overall result, he goes back and copies all the changes to his styles. The patch provided here would not cater to his needs (and I am sure to other designers).

Kevin / Dave: please let me know what's desired here. Thank you!

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

We can easily copy the Web Inspector behavior here, that is no problem. You are correct that we shouldn't store the data across inspector instances but we can preserve the user entered text for each node without any issues.

Comment 8

[Mihai Sucan [:msucan]]

(In reply to Michael Ratcliffe from comment #7)
> We can easily copy the Web Inspector behavior here, that is no problem. You
> are correct that we shouldn't store the data across inspector instances but
> we can preserve the user entered text for each node without any issues.

Great then! Please update the patch accordingly. Also, please include a test. Thank you!

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Retain values a la chrome

Comment 10

[Dave Camp (:dcamp)]

Comment on attachment 598834 [details] [diff] [review]
Retain values a la chrome

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

r- mostly for moving persistence into the TextProperty.

::: browser/devtools/styleinspector/CssRuleView.jsm
@@ +1146,5 @@
>      if (aCommit) {
> +      // Save the user entry to the store.
> +      let rule = this.prop.rule;
> +      let store = rule.elementStyle.store;
> +      let key = rule.domRule || rule.elementStyle.element;

Why not just use rule.style as your key?

@@ +1148,5 @@
> +      let rule = this.prop.rule;
> +      let store = rule.elementStyle.store;
> +      let key = rule.domRule || rule.elementStyle.element;
> +      store.userProperties.setProperty(key, this.prop.name, aValue);
> +

Seems like the userProperties manipulation should happen in the TextProperty, not TextPropertyEditor.  Other persistent-store stuff happens in the properties rather than the editor.

@@ -1365,2 +1379,5 @@
> >  
> >  /**
> > + * Store of elements mapped to properties that have been changed by the user.
> > + */
> > +function UserProperties()

How does all this work if I have two values with the same name?

@@ +1388,5 @@
> +UserProperties.prototype = {
> +  /**
> +   * Get a named property for a given element.
> +   *
> +   * @param {HTMLElement} aElement

This doesn't seem right - your key is sometimes a rule, right? (although as suggested above the style seems like a better key).

@@ +1415,5 @@
> +   * @param {String} aName The name of the property to set.
> +   * @param {String} aValue The value of the property to set.
> +   */
> +  setProperty: function UP_setProperty(aElement, aName, aValue) {
> +    let element = this.weakMap.get(aElement, null);

Should choose a better name than element... and couldn't you just store props directly rather than wrapping it in an otherwise-empty obj?

Comment 11

[Mihai Sucan [:msucan]]

Comment on attachment 598834 [details] [diff] [review]
Retain values a la chrome

Patch looks good Mike. It works really well. Thank you!

Review comments:


 function CssRuleView(aDoc, aStore)
-  this.store = aStore;
+  this.store = aStore || {};

Why is this change needed?

@@ -1004,16 +1004,23 @@ TextPropertyEditor.prototype = {

+    let key = rule.domRule || rule.elementStyle.element;

I agree with Dave's comment here: rule.style makes more sense.

+  getProperty: function UP_getProperty(aElement, aName, aDefault) {
...
+    if (element && element.props[aName]) {

This is not going to work as expected. element.props[aName] can have a falsy value. Better do if (element && aName in element.props) { ... }

+  hasProperty: function UP_hasProperty(aElement, aName) {
+    let element = this.weakMap.get(aElement, null);
+    return element && element.props[aName];

Similar to above, this is going to give back unexpected results when the value stored is flasy.

+  gBrowser.selectedBrowser.addEventListener("load", function(evt) {
+    gBrowser.selectedBrowser.removeEventListener(evt.type, arguments.callee, true);

Please do not use arguments.callee - this is deprecated.

+  ruleDialog.addEventListener("load", function onLoad(evt) {
+    ruleDialog.removeEventListener("load", onLoad);
...
+  }, true);

You add the event in the capturing phase, then you remove the event from the non-capturing phase. This doesn't work as expected. Please fix.

+ EventUtils.sendKey("return", ruleDialog);

Did you try EventUtils.synthesizeKey("VK_RETURN")? It should work. We should always favor sending events that are as close as possible to the native ones.

In browser_ruleview_editor.js: 
+  info("testReturnCommit");

This is the only change in the whole test file. Is this needed?


Giving r- before the above comments are addressed, but this is really close to r+. Great work!

Looking forward for the updated patch. Thank you!

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Addressed dcamp's comments

(In reply to Dave Camp (:dcamp) from comment #10)
> Comment on attachment 598834 [details] [diff] [review]
> Retain values a la chrome
> 
> Review of attachment 598834 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- mostly for moving persistence into the TextProperty.
> 
> ::: browser/devtools/styleinspector/CssRuleView.jsm
> @@ +1146,5 @@
> >      if (aCommit) {
> > +      // Save the user entry to the store.
> > +      let rule = this.prop.rule;
> > +      let store = rule.elementStyle.store;
> > +      let key = rule.domRule || rule.elementStyle.element;
> 
> Why not just use rule.style as your key?
> 

That makes heckuvalot of sense ... done.

> @@ +1148,5 @@
> > +      let rule = this.prop.rule;
> > +      let store = rule.elementStyle.store;
> > +      let key = rule.domRule || rule.elementStyle.element;
> > +      store.userProperties.setProperty(key, this.prop.name, aValue);
> > +
> 
> Seems like the userProperties manipulation should happen in the
> TextProperty, not TextPropertyEditor.  Other persistent-store stuff happens
> in the properties rather than the editor.
> 

Agreed, this complicated things a little when I made the change but I am much happier with it now ... done.

> @@ -1365,2 +1379,5 @@
> > >  
> > >  /**
> > > + * Store of elements mapped to properties that have been changed by the user.
> > > + */
> > > +function UserProperties()
> 
> How does all this work if I have two values with the same name?
> 

CSSStyleDeclarations cannot have 2 properties of the same name as they would overwrite one another. This confusion was probably caused by me calling the key aElement and the inaccurate comment.

> @@ +1388,5 @@
> > +UserProperties.prototype = {
> > +  /**
> > +   * Get a named property for a given element.
> > +   *
> > +   * @param {HTMLElement} aElement
> 
> This doesn't seem right - your key is sometimes a rule, right? (although as
> suggested above the style seems like a better key).
> 

aStyle it is ... fixed

> @@ +1415,5 @@
> > +   * @param {String} aName The name of the property to set.
> > +   * @param {String} aValue The value of the property to set.
> > +   */
> > +  setProperty: function UP_setProperty(aElement, aName, aValue) {
> > +    let element = this.weakMap.get(aElement, null);
> 
> Should choose a better name than element... and couldn't you just store
> props directly rather than wrapping it in an otherwise-empty obj?

Yup, not sure why I used props{}, probably made sense when I first wrote it. The name "element" was undoubtably the worst choice for a variable name ever. We now use the far more aptly named "entry" instead.

Comment 13

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Addressed msucan's comments

How the heck did I turn out asking msucan & dcamp for a review? Anyhow, thanks for that :o)

(In reply to Mihai Sucan [:msucan] from comment #11)
> Comment on attachment 598834 [details] [diff] [review]
> Retain values a la chrome
> 
> Patch looks good Mike. It works really well. Thank you!
> 
> Review comments:
> 
> 
>  function CssRuleView(aDoc, aStore)
> -  this.store = aStore;
> +  this.store = aStore || {};
> 
> Why is this change needed?
> 

Because tests do not pass in an aStore object which we now need for this.store.userProperties (in real life this is passed in by InspectorUI.openRuleView()).

> @@ -1004,16 +1004,23 @@ TextPropertyEditor.prototype = {
> 
> +    let key = rule.domRule || rule.elementStyle.element;
> 
> I agree with Dave's comment here: rule.style makes more sense.
> 

Yes, I changed that when addressing dcamp's comments.

> +  getProperty: function UP_getProperty(aElement, aName, aDefault) {
> ...
> +    if (element && element.props[aName]) {
> 
> This is not going to work as expected. element.props[aName] can have a falsy
> value. Better do if (element && aName in element.props) { ... }
> 

Done.

> +  hasProperty: function UP_hasProperty(aElement, aName) {
> +    let element = this.weakMap.get(aElement, null);
> +    return element && element.props[aName];
> 
> Similar to above, this is going to give back unexpected results when the
> value stored is flasy.
> 

I have removed the unused method so this no longer applies.

> +  gBrowser.selectedBrowser.addEventListener("load", function(evt) {
> +    gBrowser.selectedBrowser.removeEventListener(evt.type,
> arguments.callee, true);
> 
> Please do not use arguments.callee - this is deprecated.
> 

Changed

> +  ruleDialog.addEventListener("load", function onLoad(evt) {
> +    ruleDialog.removeEventListener("load", onLoad);
> ...
> +  }, true);
> 
> You add the event in the capturing phase, then you remove the event from the
> non-capturing phase. This doesn't work as expected. Please fix.
> 

Good spot, Fixed.

> + EventUtils.sendKey("return", ruleDialog);
> 
> Did you try EventUtils.synthesizeKey("VK_RETURN")? It should work. We should
> always favor sending events that are as close as possible to the native ones.
> 

Ah, the old EventUtils.synthesizeKey("VK_RETURN", {}, ruleDialog)? ... done.

> In browser_ruleview_editor.js: 
> +  info("testReturnCommit");
> 
> This is the only change in the whole test file. Is this needed?
> 
> 

No, I noticed this when addressing dcamp's and removed it.

> Giving r- before the above comments are addressed, but this is really close
> to r+. Great work!
> 
> Looking forward for the updated patch. Thank you!

msucan: I have asked you for review as Dave is in MV this week.

Comment 14

[Mihai Sucan [:msucan]]

Comment on attachment 599245 [details] [diff] [review]
Addressed msucan's comments

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

Thanks for your quick patch update Mike!

Giving r+ because the comments below are really easy to address.

I would note that the defaultValue concept you now introduce *might* be confusing. Maybe it's just because I am new to the code. I'll let Dave review that part more throughly.

::: browser/devtools/styleinspector/CssRuleView.jsm
@@ +576,5 @@
>    this.updateComputed();
>  }
>  
>  TextProperty.prototype = {
> +  get value()

Please document this new property.

@@ +623,5 @@
>        });
>      }
>    },
>  
> +  setValue: function TextProperty_setValue(aValue, aPriority, aUserRule)

This is an undocumented method. Please add documentation.

Comment 15

[Dave Camp (:dcamp)]

Comment on attachment 599245 [details] [diff] [review]
Addressed msucan's comments

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

Sorry, I should have caught this in the last review - tl;dr is "save properties to the store in Rule_applyProperties and read them in Rule_getTextProperties".

::: browser/devtools/styleinspector/CssRuleView.jsm
@@ +105,5 @@
> +
> +  // We don't want to overwrite aStore.userProperties if it was passed so we
> +  // only create it if this is not the case (e.g. by tests).
> +  this.store = aStore || { userProperties: new UserProperties() };
> +

This comment doesn't Quite match the implemetation - should probably do something like aStore || {} and then if (!(userProperties in store)) right?  In case someone passes in an empty store{}?

And if you did that, you could drop the userProperties setup in CssRuleView, and just let userProperties be something only ElementStyle and its children care about.

@@ +569,5 @@
>  function TextProperty(aRule, aName, aValue, aPriority)
>  {
>    this.rule = aRule;
>    this.name = aName;
> +  this.defaultValue = aValue;

I don't really understand this defaultValue change.  Why not just populate .value from the store at TextProperty creation time?

This causes problems when we consider multiple text properties of the same property name - while you're right that a given CSSRule will only have a given text property, the UI can have multiple properties of the same name.  Try adding a new property with the same property name but different value as a previous one - the code that's there now does the right thing (if you disable one the other is used, etc).  But with this code you'll break that behavior and things will get weird, because both editors will have the same underlying value (but unless you're really careful to propagate the change to editors, that won't be obvious by looking at it).

So we really should only be using this store when initially building the rule view for a given selection, not in the value getter for TextProperty.  The best place to use this store, I think, would be Rule_getTextProperties, right before creating the TextProperty.  That's the best point to replace the returned-by-the-system value with the stored-in-the-store value.

And actually, along the same lines the best place to *set* the value in the store would probably be in Rule_applyProperties().  That will ensure that the value saved to the store is the same one applied to the rule (taking into account disabled properties, duplicate properties, etc).

@@ +623,5 @@
>        });
>      }
>    },
>  
> +  setValue: function TextProperty_setValue(aValue, aPriority, aUserRule)

Rather than a boolean arg to setValue I'd prefer a new method (setEditedValue or something?) that does the aUserRule==true stuff and then calls the normal setValue.  Boolean args are hard to read.

Comment 16

[Dave Camp (:dcamp)]

(In reply to Dave Camp (:dcamp) from comment #15)
> > +  setValue: function TextProperty_setValue(aValue, aPriority, aUserRule)
> 
> Rather than a boolean arg to setValue I'd prefer a new method
> (setEditedValue or something?) that does the aUserRule==true stuff and then
> calls the normal setValue.  Boolean args are hard to read.

(wrote this part before I wrote the part before it - clearly if you move store manipulation up into the Rule you won't need this boolean arg at all).

Comment 17

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Addressed reviewers comments

(In reply to Dave Camp (:dcamp) from comment #15)
> Comment on attachment 599245 [details] [diff] [review]
> Addressed msucan's comments
> 
> Review of attachment 599245 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry, I should have caught this in the last review - tl;dr is "save
> properties to the store in Rule_applyProperties and read them in
> Rule_getTextProperties".
> 
> ::: browser/devtools/styleinspector/CssRuleView.jsm
> @@ +105,5 @@
> > +
> > +  // We don't want to overwrite aStore.userProperties if it was passed so we
> > +  // only create it if this is not the case (e.g. by tests).
> > +  this.store = aStore || { userProperties: new UserProperties() };
> > +
> 
> This comment doesn't Quite match the implemetation - should probably do
> something like aStore || {} and then if (!(userProperties in store)) right? 
> In case someone passes in an empty store{}?
> 
> And if you did that, you could drop the userProperties setup in CssRuleView,
> and just let userProperties be something only ElementStyle and its children
> care about.
> 

Done

> @@ +569,5 @@
> >  function TextProperty(aRule, aName, aValue, aPriority)
> >  {
> >    this.rule = aRule;
> >    this.name = aName;
> > +  this.defaultValue = aValue;
> 
> I don't really understand this defaultValue change.  Why not just populate
> .value from the store at TextProperty creation time?
> 

I thought that you said it had to be done from the property itself so I was bending over backwards in order to do so.

> This causes problems when we consider multiple text properties of the same
> property name - while you're right that a given CSSRule will only have a
> given text property, the UI can have multiple properties of the same name. 
> Try adding a new property with the same property name but different value as
> a previous one - the code that's there now does the right thing (if you
> disable one the other is used, etc).  But with this code you'll break that
> behavior and things will get weird, because both editors will have the same
> underlying value (but unless you're really careful to propagate the change
> to editors, that won't be obvious by looking at it).
> 

Didn't realize that we allowed that although it makes sense now you mention it.

> So we really should only be using this store when initially building the
> rule view for a given selection, not in the value getter for TextProperty. 
> The best place to use this store, I think, would be Rule_getTextProperties,
> right before creating the TextProperty.  That's the best point to replace
> the returned-by-the-system value with the stored-in-the-store value.
> 

Done.

> And actually, along the same lines the best place to *set* the value in the
> store would probably be in Rule_applyProperties().  That will ensure that
> the value saved to the store is the same one applied to the rule (taking
> into account disabled properties, duplicate properties, etc).
> 

Done

> @@ +623,5 @@
> >        });
> >      }
> >    },
> >  
> > +  setValue: function TextProperty_setValue(aValue, aPriority, aUserRule)
> 
> Rather than a boolean arg to setValue I'd prefer a new method
> (setEditedValue or something?) that does the aUserRule==true stuff and then
> calls the normal setValue.  Boolean args are hard to read.

See below

> (wrote this part before I wrote the part before it - clearly if you move
> store manipulation up into the Rule you won't need this boolean arg at all).

Removed

Comment 18

[Dave Camp (:dcamp)]

Comment on attachment 599567 [details] [diff] [review]
Addressed reviewers comments

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

::: browser/devtools/styleinspector/CssRuleView.jsm
@@ +433,4 @@
>  
>      for each (let prop in this.textProps) {
> +      store.userProperties.setProperty(this.style, prop.name, prop.value);
> +

I think this should be under the !prop.enabled clause - otherwise an enabled property followed by a disabled property of the same name will get the value of the disabled property in the store and enabled property in the rule.

With that change, looks good.

Comment 19

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Final change

You are right ... disabled properties should not override enabled ones.

Comment 20

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Green on try

Comment 21

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/integration/fx-team/rev/738f43c4668d

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/738f43c4668d

Comment 23

[Eric Shepherd [:sheppy]]

No actual change, just a fix to behave as expected, so no doc required.