1325145 - Rule view warns about invalid property value if value is clicked, cleared and escaped without change

Status: NEW

(DevTools :: Inspector: Rules, defect, P2)

Description

[[:Towkir] Ahmed]

Attachment: color_property_value.gif

STR:
1. Open Developer tools, pick any element on any webpage that has a color property with a value.
2. Click on the value.
3. Press backspace from keyboard.
4. Press esc key from keyboard.

Result:
An error icon will appear right to the value of color.

Expectation:
There should be no error icons, it should have been remain the same, as you did not change anything.

Interesting fact:
While there is the icon visible, click on the value again and press enter. The error disappears ;)

Comment 1

[[:Towkir] Ahmed]

Hi Tim,
Have a look at this

Comment 2

[Tom Tromey :tromey]

I think what is happening is that the inplace editor is calling its destroy() method,
which winds up in TextPropertyEditor.update, which then calls updatePropertyState.
At this point, the property value reversion has been sent to the server but not received,
so isValid returns false, leading the warning sign.
Offhand I'm not sure what the fix is.  Best may be to have a way for the property editor
to wait until the server has responded before updating the UI.  Another way might be to
have the editor refresh when the response arrives.

Comment 3

[Tim Nguyen :ntim]

I can reproduce the bug. Tom provided some details if you're interested in fixing the bug.

Comment 4

[Julian Descottes [:jdescottes]]

Towkir, are you interested in working on this bug? I'd be happy to work with you on this one!

Comment 5

[[:Towkir] Ahmed]

Hi Julian,
Thanks for being a mentor here. I would also be happy to work on this, Assigning to me for now.

Comment 6

[Julian Descottes [:jdescottes]]

I took a look at the bug and it's not an easy one. The root cause of the issue is what Tom mentioned, so I'll just give more details about the classes involved here.

So our main issue is that text-property-editor calls isValid() on models/text-property which forwards this to models/rule (or rather to the StyleRule actor of models/rule).

Since we sent an invalid value to the StyleRule actor for preview purposes just before, the actor still has this wrong value and from its point of view, the rule is invalid.

I think the easiest path forward is to fire an event when a rule is updated, so that the text-property-editor can listen to such events and update itself.

The event here should be emitted by the models/rule object. To generate generic events, we can use the EventEmitter (you can find many examples in the code base that rely on EventEmitter and the API is pretty straightforward). In models/rule, you can see the rule notifies its ElementStyle parent about changes by calling this.elementStyle._changed() in 2 places. I think this is where the new event should be fired (and this event could optionally replace calling elementStyle._changed, instead elementStyle could listen on the event as well!).

Once we have our new event, we need to add a listener to this event in text-property-editor (which has a pointer to its corresponding rule via this.rule). Simply calling this.update() when we get a change should do the trick.

After you manage to implement this, it would be nice to get a try run and see if this has side-effects on existing tests. We will be potentially updating text-property-editors more frequently, and the inspector being a bit shaky when it comes to events and race conditions, we need to assert if this is a viable solution as soon as we can.

Comment 7

[Patrick Brosset <:pbro>]

2 years with no updates, resetting the assignee field here. Feel free to pick up again if you do intend to work on it after all.