913509 - [rule view] Papercuts - Inconsistent behavior when modifying CSS declarations

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Brian Grinstead [:bgrins]]

* When changing css with valid property/value to have invalid property, it does not get line through applied.  The warning shows up, but no line through. If you type the same thing in as a new rule, it gets the line through.  Change 'background: red' to 'background2: red' to see this.
* When clicking out of an invalid value, like 'background: asdf', the asdf disappears from the UI.  This is different than when you press enter.
* If you have a declaration like background: red; than change red to red2, the background color does not get changed (it only applies it if the value is valid).

All of these changes can be handled together as rule view papercuts, as much of the code is shared.

Comment 1

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #0)
> * When changing css with valid property/value to have invalid property, it
> does not get line through applied.  The warning shows up, but no line
> through. If you type the same thing in as a new rule, it gets the line
> through.  Change 'background: red' to 'background2: red' to see this.

Correct, the line through should be applied here too.

> * When clicking out of an invalid value, like 'background: asdf', the asdf
> disappears from the UI.  This is different than when you press enter.

I'm guessing the inplace editor might be considering this case as a non commit, therefore reverting the value. I have to say I was not completely convinced it should commit when clicking out, but it seems the Chrome devtools do that as well as firebug, so let's go.

> * If you have a declaration like background: red; than change red to red2,
> the background color does not get changed (it only applies it if the value
> is valid).

From what I know, live preview happens while you type, as well as validation of the value, but the preview will only show values that do validate. So in the example you gave, I think it's normal the color doesn't change cause as far as the inspector is concerned, the value is still 'red'. I would tend to keep it the way it is now so that your page doesn't "flicker" too much while you type. In any case, we also have the suggestions dropdown that also auto-complete values.
What would you do instead?

Comment 2

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset from comment #1)

> From what I know, live preview happens while you type, as well as validation
> of the value, but the preview will only show values that do validate. So in
> the example you gave, I think it's normal the color doesn't change cause as
> far as the inspector is concerned, the value is still 'red'. I would tend to
> keep it the way it is now so that your page doesn't "flicker" too much while
> you type. In any case, we also have the suggestions dropdown that also
> auto-complete values.
> What would you do instead?

I have now noticed that it actually doesn't seem to be changing the value on anything invalid even after committing on nightly.  Set an element to 'background:red', then change it to 'background:red2'.  This should show red2 with a line through it, and the actual background should be white.  Right now the actual background stays as red even after the change.

My proposal would be for the live preview update to match exactly what will happen when you commit the change.  So if you typed an invalid property it would still apply.  However, with this technique I've noticed a flickering that happens during autocomplete as it changes from 'r' to 'red' during typing with the inplace editor.  I would think we could throttle the validation event so that it only fires every X ms to prevent this from happening (the inplace editor uses a setTimeout of 0 to update the value with the autocompleted suggestion).

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> (In reply to Patrick Brosset from comment #1)
> 
> > From what I know, live preview happens while you type, as well as validation
> > of the value, but the preview will only show values that do validate. So in
> > the example you gave, I think it's normal the color doesn't change cause as
> > far as the inspector is concerned, the value is still 'red'. I would tend to
> > keep it the way it is now so that your page doesn't "flicker" too much while
> > you type. In any case, we also have the suggestions dropdown that also
> > auto-complete values.
> > What would you do instead?
> 
> I have now noticed that it actually doesn't seem to be changing the value on
> anything invalid even after committing on nightly.  Set an element to
> 'background:red', then change it to 'background:red2'.  This should show
> red2 with a line through it, and the actual background should be white. 
> Right now the actual background stays as red even after the change.
> 
> My proposal would be for the live preview update to match exactly what will
> happen when you commit the change.  So if you typed an invalid property it
> would still apply.  However, with this technique I've noticed a flickering
> that happens during autocomplete as it changes from 'r' to 'red' during
> typing with the inplace editor.  I would think we could throttle the
> validation event so that it only fires every X ms to prevent this from
> happening (the inplace editor uses a setTimeout of 0 to update the value
> with the autocompleted suggestion).

There may be another way to work around this as well having the inplace editor notify us if it has autocomplete suggestions and not apply changes immediately in this case.  Assuming we could get rid of a flicker problem, I do think that it should be applying the style whether or not it is valid to prevent confusion of it changing after you press enter.  I will put together a patch to address these issues and ask for some feedback.

Comment 4

[Patrick Brosset <:pbro>]

Agreed, whatever has been typed should be applied while typing (live preview), that makes more sense. We just need to ensure that ESCaping out of the field reverts to the previous value (the test browser_ruleview_bug_902966_revert_value_on_ESC.js should ensure this is the case anyway).

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset from comment #4)
> Agreed, whatever has been typed should be applied while typing (live
> preview), that makes more sense. We just need to ensure that ESCaping out of
> the field reverts to the previous value (the test
> browser_ruleview_bug_902966_revert_value_on_ESC.js should ensure this is the
> case anyway).

What do you think should happen when you press escape on a property you just added?  So you type: "background: " then in the new editor that appears you type: "red" and press escape.  Should it leave it as "background": "" or just remove the property entirely?  FWIW, if you press escape while editing the name it removes the property.

Comment 6

[Patrick Brosset <:pbro>]

My proposal is that it removes the property entirely, it makes more sense to me.
Another use case: When focusing a value and deleting all the text, then pressing ENTER, for me it should remove the whole property:value line, not just leave the empty property as it does today. I think this would be more consistent with the rest.

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset from comment #6)

> My proposal is that it removes the property entirely, it makes more sense to
> me.

Agreed, I will make pressing escape on a new value remove the property.

> Another use case: When focusing a value and deleting all the text, then
> pressing ENTER, for me it should remove the whole property:value line, not
> just leave the empty property as it does today. I think this would be more
> consistent with the rest.

I could see this too.  I'm not sure how useful leaving the declaration with an empty value is.  Also, it can make clicking into the empty inplace editor tricky since there is such a small space.

Comment 8

[Brian Grinstead [:bgrins]]

Other things this patch does:

* When a property is removed (either by value being empty or pressing escape on a new property), it will automatically being editing the name field inside of the next property.
** If the property being removed is the final property in the list, it will begin editing the previous property.
** If the property being removed is the *only* property in the list, it will focus the close brace, which allows you to press enter to start adding a new one.

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: ruleview-papercuts1.patch

There are many changes to ruleview to fix some of the issues addressed discussed in this case.  I'm looking for some feedback about how the CSS editing looks/feels with the patch applied versus not.  Is there anything else it should be doing? Does it break or fail to live up to your expectations at any point?

A few things to try:

* Add a new property, pressing escape while editing value (with and without other sibling properties)
* Add two properties with the same name (like background: red; background: green;).
** Make these invalid one at a time to see how it is resetting.
* Modify property values using nonstandard input and see live preview (autocompletion, incremetable values - they should be the same as typing it manually)
* Modify the name of an existing property and make sure the line goes through
* Modify the value of an existing property to "" and make sure it disappears (with and without other sibling properties).  Does the focus go to where you expect?

Comment 10

[Brian Grinstead [:bgrins]]

Attachment: ruleview-papercuts1.patch

Uploading actual patch. See Comment 9 for feedback instructions.

Comment 11

[Patrick Brosset <:pbro>]

Imported and tested the patch. The rule view feels a lot better with it! Awesome.
There's only one detail that I'm not sure about: add a new property, press escape while editing the value: the property disappear, this is great, however, focus goes back to the previous (if any) property name.
I tend to think it should go the previous property value, as this is the previous focusable field.

Comment 12

[Patrick Brosset <:pbro>]

As for the code, I couldn't spot any problem.
There's jut a `console.log(...);` in a test that might need to be removed.

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: ruleview-papercuts2.patch

Comment 14

[Brian Grinstead [:bgrins]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=58be41b99093 and ready for a review.

(In reply to Patrick Brosset from comment #11)
> There's only one detail that I'm not sure about: add a new property, press
> escape while editing the value: the property disappear, this is great,
> however, focus goes back to the previous (if any) property name.
> I tend to think it should go the previous property value, as this is the
> previous focusable field.

OK, I have updated it to jump to the previous value or next name.  It seems better this way.

Comment 15

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

Comment on attachment 803179 [details] [diff] [review]
ruleview-papercuts2.patch

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

Looks great, only one tiny nit:

::: browser/devtools/styleinspector/rule-view.js
@@ +1989,5 @@
> +      if (val.value.trim() === "") {
> +        this.remove();
> +      } else {
> +        // Reset to original before setting, since the rule has been set by preview
> +        //this.prop.setValue(this.committed.value, this.committed.priority);

Should this line not be removed?

Comment 16

[Brian Grinstead [:bgrins]]

Attachment: ruleview-papercuts3.patch

Removes unused comments from Attachment #803179 [details] [diff]

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/1b2aa92e9b91

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1b2aa92e9b91