Closed Bug 1217328 Opened 4 years ago Closed 4 years ago

[rule view] Show door hanger for filter property even when an invalid or no value was entered

Categories

(DevTools :: Inspector, enhancement)

enhancement
Not set

Tracking

(firefox44 fixed)

VERIFIED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: sebo, Assigned: tromey)

References

(Depends on 1 open bug)

Details

(Whiteboard: [devtools-ux])

Attachments

(2 files, 2 obsolete files)

+++ 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
See Also: → 1217301
I think this just needs a small tweak in output-parser.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
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".
Attachment #8678210 - Flags: review?(pbrosset)
Attachment #8678211 - Flags: review?(pbrosset)
Attachment #8678210 - Flags: review?(pbrosset) → review+
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.
Attachment #8678211 - Flags: review?(pbrosset) → review+
(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.
Attachment #8678210 - Attachment is obsolete: true
Attachment #8678211 - Attachment is obsolete: true
Add r=pbrosset.
Attachment #8678820 - Flags: review+
Attachment #8678821 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/396ed5175222
https://hg.mozilla.org/mozilla-central/rev/150b4bb5d07b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Depends on: 1219571
(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
Severity: normal → enhancement
Status: RESOLVED → VERIFIED
Summary: Show door hanger for filter property even when an invalid or no value was entered → [rule view] Show door hanger for filter property even when an invalid or no value was entered
See Also: → 1221156
See Also: → 1268427
See Also: → 1268429
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.