+++ This bug was initially created as a clone of Bug #1217301 +++ - When adding the 'filter' property the icon for to open the tooltip is not shown. That means you first have to enter a valid value like 'none' before you get the icon. This is bad for people that don't know which values are available. - When the 'filter' property has an invalid value, the icon to open the tooltip is not shown. This results in the same UX as above. Sebastian

I think this just needs a small tweak in output-parser.

One question is what should happen when the filter editor is opened on an invalid property value. There are a few cases. If I have "filter: hithere", then opening the filter editor (with my simple patch applied) immediately changes this to "filter: none". Seems reasonable for a specialized editor. If I write "filter: contrast('xxx')", opening the editor shows the "contrast" function (good) with an empty argument (bad). Immediately closing the editor changes the property to "filter: contrast('NaNxxx')" - obviously this has to change, though it's quite fun to open and close it many times and get "filter: contrast('NaNNaNNaNNaNNaNxxx')" If I have something invalid in the middle of the property, the editor just drops it. Before: filter: contrast(5%) whatever After: filter: contrast(5%) That seems reasonable enough. I'm leaning toward preserving the current behavior of just dropping things that the editor doesn't understand; and then changing the various filter functions to also drop invalid arguments in favor of some default. This will fix the NaN problem.

(In reply to Tom Tromey :tromey from comment #2) > One question is what should happen when the filter editor is opened on an > invalid property value. > > There are a few cases. > > If I have "filter: hithere", then opening the filter editor (with my simple > patch applied) > immediately changes this to "filter: none". Seems reasonable for a > specialized editor. Agree. > If I write "filter: contrast('xxx')", opening the editor shows the > "contrast" function > (good) with an empty argument (bad). Immediately closing the editor changes > the property > to "filter: contrast('NaNxxx')" - obviously this has to change, though it's > quite fun to > open and close it many times and get "filter: contrast('NaNNaNNaNNaNNaNxxx')" In that case I'd expect the tooltip to generate an output like this: filter: contrast(0); > If I have something invalid in the middle of the property, the editor just > drops it. > Before: filter: contrast(5%) whatever > After: filter: contrast(5%) > That seems reasonable enough. So to be clear on this: Before: filter: contrast(5%) whatever drop-shadow(0 0 2px red) invalidvalue; After: filter: contrast(5%) drop-shadow(0 0 2px red); and Before: filter: contrast(5%) whatever invert('xxx'); After: filter: contrast(5%) whatever invert(0); Correct? > I'm leaning toward preserving the current behavior of just dropping things > that the > editor doesn't understand; and then changing the various filter functions to > also drop > invalid arguments in favor of some default. This will fix the NaN problem. In my eyes, being smart on invalid filter arguments is definitely better than throwing them all away. So, I agree with you here. Sebastian

(In reply to Sebastian Zartner [:sebo] from comment #3) > So to be clear on this: > > Before: filter: contrast(5%) whatever drop-shadow(0 0 2px red) invalidvalue; > After: filter: contrast(5%) drop-shadow(0 0 2px red); > > and > > Before: filter: contrast(5%) whatever invert('xxx'); > After: filter: contrast(5%) whatever invert(0); > > Correct? Yes.

(In reply to Tom Tromey :tromey from comment #4) > (In reply to Sebastian Zartner [:sebo] from comment #3) > > > So to be clear on this: > > > > Before: filter: contrast(5%) whatever drop-shadow(0 0 2px red) invalidvalue; > > After: filter: contrast(5%) drop-shadow(0 0 2px red); > > > > and > > > > Before: filter: contrast(5%) whatever invert('xxx'); > > After: filter: contrast(5%) whatever invert(0); > > > > Correct? > > Yes. I spoke too soon, I think that second one is slightly incorrect and should be: Before: filter: contrast(5%) whatever invert('xxx'); After: filter: contrast(5%) invert(0); That is, removing the "whatever".

Comment on attachment 8678211 [details] [diff] [review] let filter editor work on invalid values Review of attachment 8678211 [details] [diff] [review]: ----------------------------------------------------------------- This change looks good.

(In reply to Tom Tromey :tromey from comment #5) > (In reply to Tom Tromey :tromey from comment #4) > > (In reply to Sebastian Zartner [:sebo] from comment #3) > > > > > So to be clear on this: > > > > > > Before: filter: contrast(5%) whatever drop-shadow(0 0 2px red) invalidvalue; > > > After: filter: contrast(5%) drop-shadow(0 0 2px red); > > > > > > and > > > > > > Before: filter: contrast(5%) whatever invert('xxx'); > > > After: filter: contrast(5%) whatever invert(0); > > > > > > Correct? > > > > Yes. > > I spoke too soon, I think that second one is slightly incorrect and should > be: > > Before: filter: contrast(5%) whatever invert('xxx'); > After: filter: contrast(5%) invert(0); > > That is, removing the "whatever". Oh, yes, my fault. Of course 'whatever' should also be removed. Sebastian

Add r=pbrosset.

Add r=pbrosset.

https://hg.mozilla.org/integration/fx-team/rev/396ed5175222 https://hg.mozilla.org/integration/fx-team/rev/150b4bb5d07b

https://hg.mozilla.org/mozilla-central/rev/396ed5175222 https://hg.mozilla.org/mozilla-central/rev/150b4bb5d07b

(In reply to Sebastian Zartner [:sebo] from comment #0) > - When adding the 'filter' property the icon for to open the tooltip is not > shown. That means you first have to enter a valid value like 'none' before > you get the icon. This is bad for people that don't know which values are This use case may have been solved by showing the door hanger immediately after entering the property name and switching to the value field, but it is acceptable when you first have to enter an invalid value, hit Enter and are then able to click the icon. The only thing that is not correct yet, is the warning icon for an invalid value is still shown after opening the tooltip and having the value autocorrected by it. Therefore I opened bug 1219571 including more details on this. Sebastian