Brian Grinstead [:bgrins] Assignee
	Description • 11 years ago

To reproduce: paste the following text into a new declaration in the rule view:

    background: orange;
    color: red;
    text-decoration: underline;

Current behavior: creates a single declaration with a property named: "background: orange;     color: red;     text-decoration: underline;" and an empty value.

What it should do: make 3 separate declarations, one for each one pasted in.

Mike, does this have anything to do with Bug 788458?  It appears that was focused on copying rules *out* of the editor, not pasting them in, but CCing just in case.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 1 • 11 years ago

(In reply to Brian Grinstead [:bgrins] from comment #0)
> Mike, does this have anything to do with Bug 788458?  It appears that was
> focused on copying rules *out* of the editor, not pasting them in, but CCing
> just in case.

Probably, IIRC we removed the onPaste handler in that bug but I don't think we ever handled pasting in multiple values.

	Brian Grinstead [:bgrins] Assignee
	Comment 2 • 11 years ago

Patrick,
I'm looking for some feedback on this patch, similar to what we did for Bug 913509.  Basically, I'm wanting to make sure the feel of the behavior introduced in this patch is expected.

What it does:
With this patch applied, you can enter in multiple new properties into name and value fields.  So, instead of a workflow of edit a property, save it's name or value, focus the next one, you can now add multiple properties from any of the inplace editors.  This is full of edge cases and weird input handling, as you will see.

a) the 'new property' editor (do this by pasting mutliple rules).  If the last rule is incomplete, focus it's value field, otherwise focus a new property.
b) an existing name editor (the "background" in "background:red").  This will wipe out the current name/value with the first new property in the set, and add the new ones directly below this rule.  If the last rule is incomplete, focus it's value field, otherwise focus a new property.
c) an existing value editor (the "red" in background:red").  This can get a little tricky with edge cases.  If you enter "red;color:green" it will set background:red; color:green; as two rules and focus a new property.  If you enter "color:green;width: 100%;" it will set background: color:green; width: 100%; as two different properties.  If you enter "red;width:" it will set background:red; width: || with the || being a new focused input.

Can you play around with it and let me know if everything works as you would hope / expect as a user?

NOTE: I've temporarily disabled the advance on character for the normal name and value editors, to make this easier to test.  So, typically the only way you could enter in multiple rules into the name or value field is pasting.  But it is *easier* to click into the 'red' here:

    background: |red;|

and type:

    background: |red; color: green;|

than to have to paste it in every time.

	Patrick Brosset <:pbro>
	Comment 3 • 11 years ago

First of all, I love it!

Now for the details:

> a) the 'new property' editor (do this by pasting mutliple rules).  If the
> last rule is incomplete, focus it's value field, otherwise focus a new
> property.

If you paste: "position: absolute; left:0; right: 0; heig", which means the last rule has an incomplete property, then the word "heig" is removed. It's probably a really minor case, but I would fill the inplace editor for the last new property with that word, if possible.

> b) an existing name editor (the "background" in "background:red").  This
> will wipe out the current name/value with the first new property in the set,
> and add the new ones directly below this rule.  If the last rule is
> incomplete, focus it's value field, otherwise focus a new property.

I think that makes sense, and it works for me.

> c) an existing value editor (the "red" in background:red").  This can get a
> little tricky with edge cases.  If you enter "red;color:green" it will set
> background:red; color:green; as two rules and focus a new property.  If you
> enter "color:green;width: 100%;" it will set background: color:green; width:
> 100%; as two different properties.  If you enter "red;width:" it will set
> background:red; width: || with the || being a new focused input.

The 1st and 3rd edge cases you're describing here make sense to me the way you've implemented them.
As for the 2nd (entering "color:green;width: 100%;" in a value editor), if possible I would probably do it as for (b): wipe out the current name/value and start adding rules for color:green and width:100%.
I don't know how easy that will be though.


I like the fact that if you paste rules in an existing value editor, but after the last ; character, it will keep that value untouched and insert the pasted rules below. This makes easier to enter new rules (in my opinion, clicking next to the closing } isn't always obvious when you want to add a new rule).

	Brian Grinstead [:bgrins] Assignee
	Comment 4 • 11 years ago

(In reply to Patrick Brosset from comment #3)

> If you paste: "position: absolute; left:0; right: 0; heig", which means the
> last rule has an incomplete property, then the word "heig" is removed. It's
> probably a really minor case, but I would fill the inplace editor for the
> last new property with that word, if possible.

Good idea, I have updated the code to allow the partially finished declaration.  It now creates a new prop with the name, and highlights the value field, just like it does if you had ended it with "heigh:".
 
> As for the 2nd (entering "color:green;width: 100%;" in a value editor), if
> possible I would probably do it as for (b): wipe out the current name/value
> and start adding rules for color:green and width:100%.
> I don't know how easy that will be though.

I think it would be possible, but I'm not sure if it would be desirable.  It is just a weird case because of bad user input.  I think if you've entered anything in the value field, it would be odd if the name next to it changed.  That said, it should be easy to recover from.  In this case, you could copy the new (invalid) value "color:green", click into the current name and paste, and it would wipe it out.

I have to make some updates to the inplace editor to clean up some of the code that is affecting blur/focus events, but this should be ready for review after that.

	Brian Grinstead [:bgrins] Assignee
	Comment 5 • 11 years ago

Here is a patch that follows behavior specified in Comment 2, along with the "position: absolute; left:0; right: 0; heig" case specified in Comment 3.

It also updates relevant tests, including:

* browser_ruleview_bug_902966_revert_value_on_ESC.js which was printing a color in the wrong format because the iterations were being called before CssRuleViewChanged was fired on the ruleview.
* browser_ruleview_ui.js - test that escape on a new property removes it (even after text has been entered).
* browser_ruleview_editor_changedvalues.js - test a few more input types to handle the multi input cases.

Ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=e2ff8f883f86

	Patrick Brosset <:pbro>
	Comment 6 • 11 years ago

Looking good after a quick glance at the code. I'll apply the patch and do some testing.
I should be sending back the review by tomorrow evening.

	Patrick Brosset <:pbro>
	Comment 7 • 11 years ago

This works beautifully, I've tested a number of edge cases and it behaves very naturally in all of them.
It makes it feel as if the rule contained a text editor where you could paste things freely.
Almost make you want to be able to select/delete rules with the keyboard (which could be the next step).

Going to review the code now.

	Patrick Brosset <:pbro>
	Comment 8 • 11 years ago

Comment on attachment 8360621 [details] [diff] [review]
paste-rules.patch

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

R+ with addressed comments and a try build (I've requested a few more bc runs just in case, since there were a couple of unrelated test failures).

::: browser/devtools/styleinspector/rule-view.js
@@ +1802,3 @@
>    {
> +    let prop = this.rule.createProperty(aName, aValue, aPriority, aSiblingProp);
> +    let ind = this.rule.textProps.indexOf(prop);

nit: I would name this index rather than ind

@@ +1805,2 @@
>      let editor = new TextPropertyEditor(this, prop);
> +    this.propertyList.insertBefore(editor.element, this.propertyList.children[ind]);

This surprised me at first, but I then realized that it would be similar to appendChild if the second argument was null. Is this the logic?

@@ +1840,5 @@
> +      this.newProperty();
> +    }
> +  },
> +
> +

nit: 3 line breaks here

@@ +1880,5 @@
>        popup: this.ruleView.popup
>      });
> +
> +    // Auto-close the input if multiple rules get pasted into new property.
> +    this.editor.input.addEventListener("paste", (e) => {

I would change 2 things here: 
- the callback is the same as the one used in TextPropertyEditor_create, so you could extract this function somewhere else to reuse it in both places
- and, what this would allow is to remove the event listener in the respective destroy functions.

@@ +1905,5 @@
>        return;
>      }
>  
> +    // Deal with adding declarations later (once editor has been destroyed).
> +    // If aValue is just a name, will make a new property with empty value.

So if I understand correctly, you're delaying the creation of properties to later in the _newPropertyDestroy function.
This is fine, but it can be a bit hard to understand at first, I think you should update the _onNewProperty and _onNewPropertyDestroy function comments as they are pretty short right now and outdated. I would use these comments to explain what the logic is.

@@ +2404,5 @@
> +    }
> +
> +    // The inplace editor will prevent manual typing of multiple properties,
> +    // but we need to deal with the case during a paste event.
> +    //

nit: empty comment line

@@ +2411,5 @@
> +    let properties = parseCSSText(aValue);
> +    let propertiesToAdd = [];
> +    let firstValue = aValue;
> +
> +    if (properties.length > 0) {

This part is a bit complex, but there's no way around it I guess, the complexity comes from the usecase and needs to be handled somewhere.
Perhaps one thing I'd do still is split this away in a separate function like _onValueWithMultiplePropsDone (or something better :))

@@ +2444,5 @@
> +
> +    // If the name or value is not actively being edited, and the value is
> +    // empty, then remove the whole property.
> +    // A timeout is used here to accurately check the state, since the inplace
> +    // editor `done` and `destroy` events fire before before the next editor

nit: twice the word before

::: browser/devtools/styleinspector/test/browser_ruleview_bug_902966_revert_value_on_ESC.js
@@ +19,5 @@
>  //  expected: what value is expected as a result
>  // }
>  let testData = [
>    {value: "red", commitKey: "VK_ESCAPE", modifiers: {}, expected: originalValue},
>    {value: "red", commitKey: "VK_RETURN", modifiers: {}, expected: "#F00"},

We discussed quickly about this, but after looking at the code I'm still not sure exactly why we expect #F00 here.
When testing this manually, it outputs red, which is expected.
Somehow when running the test, the property doesn't make it into the userProperties store, and therefore propDirty is false, which itself triggers the output-parser to use the default color type (which is hex when running tests).
I think your patch can be checked in without clearing this out cause it's been there for some time, but I think we should look into this.

In fact, I'm wondering if we should have a defaultColorType feature at all. I don't see why the output-parser should change the color type in any case! Whatever the ruleview gives to it, it should display back.
What do you think?

@@ +74,5 @@
> +        is(propEditor.valueSpan.textContent, testData[index].expected, "Value is same as expected: " + testData[index].expected);
> +        runTestData(index + 1);
> +      });
> +      EventUtils.synthesizeKey(testData[index].commitKey, testData[index].modifiers);
> +    }

Ah! good catch, that was why your test was failing.

::: browser/devtools/styleinspector/test/browser_ruleview_editor_changedvalues.js
@@ +137,5 @@
>                               ruleWindow);
>  }
>  
> +function testCreateNewMulti()
> +{

These new tests are fine, but I think we should also test pasting properties in a value editor. These tests always paste properties in a fresh new name editor.
For instance, we should test pasting "red;width:100%" in the [] part of color: [blue];

	Brian Grinstead [:bgrins] Assignee
	Comment 9 • 11 years ago

Great feedback, thanks.  I will be going through and updating the code.  A few notes:

> @@ +1805,2 @@
> >      let editor = new TextPropertyEditor(this, prop);
> > +    this.propertyList.insertBefore(editor.element, this.propertyList.children[ind]);
> 
> This surprised me at first, but I then realized that it would be similar to
> appendChild if the second argument was null. Is this the logic?

Yes, exactly.  If the second parameter is null, as it would be if this was being added onto the end of the list then it will behave like appendChild.  I've added a comment explaining that whole block.


> @@ +1880,5 @@
> >        popup: this.ruleView.popup
> >      });
> > +
> > +    // Auto-close the input if multiple rules get pasted into new property.
> > +    this.editor.input.addEventListener("paste", (e) => {
> 
> I would change 2 things here: 
> - the callback is the same as the one used in TextPropertyEditor_create, so
> you could extract this function somewhere else to reuse it in both places
> - and, what this would allow is to remove the event listener in the
> respective destroy functions.

Interesting - I don't actually see any destroy functionality on the TextPropertyEditor.  I will break this into a function and look into this.


> We discussed quickly about this, but after looking at the code I'm still not
> sure exactly why we expect #F00 here.
> When testing this manually, it outputs red, which is expected.
> Somehow when running the test, the property doesn't make it into the
> userProperties store, and therefore propDirty is false, which itself
> triggers the output-parser to use the default color type (which is hex when
> running tests).
> I think your patch can be checked in without clearing this out cause it's
> been there for some time, but I think we should look into this.
> 
> In fact, I'm wondering if we should have a defaultColorType feature at all.
> I don't see why the output-parser should change the color type in any case!
> Whatever the ruleview gives to it, it should display back.
> What do you think?

I think the feature was added to allow people to pick a default format for all colors.  I don't see any reason why it should change the format of a color that you added yourself in the ruleview, though.  I'll plan on leaving it this way for this patch so we don't change the specified behavior, but we can revisit.

	Brian Grinstead [:bgrins] Assignee
	Comment 10 • 11 years ago

Asking for review on the new test I added (I reverted changes to browser_ruleview_editor_changedvalues.js and added a more extensive test at: browser/devtools/styleinspector/test/browser_ruleview_multiple_properties.js).

Besides this, I've updated the code based on previous comments, with the exception of not removing the paste event listener.  There is actually not a place where this is cleaned up right now (the DOM node gets removed in inplace-editor before the destroy callback fires, so there is no node to remove from at this point).

There is an ongoing try build here: https://tbpl.mozilla.org/?tree=Try&rev=c1c7d576d2f7

	Brian Grinstead [:bgrins] Assignee
	Comment 11 • 11 years ago

Tests are green for the updated patch

	Patrick Brosset <:pbro>
	Comment 12 • 11 years ago

Comment on attachment 8361460 [details] [diff] [review]
paste-rules.patch

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

I see all my comments addressed + a new nice test. So, looking good.

	Brian Grinstead [:bgrins] Assignee
	Comment 13 • 11 years ago

https://hg.mozilla.org/integration/fx-team/rev/87f56ae05a16
https://tbpl.mozilla.org/?tree=Fx-Team&rev=87f56ae05a16

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/87f56ae05a16