1213412 - When entering a new rule in the rule view, it is crossed out before a value is entered

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Brian Grinstead [:bgrins]]

STR:
* open data:text/html,hi
* inspect the body element
* in the rule view type "height:"
* expected: there is a new height field on the left and a textbox on the right to enter a value (nothing crossed out)
* actual: the "height" name is crossed out

This is most likely a regression due to as-authored styles.  I'd say any time the value editor is focused we should remove the crossed out style.

Comment 1

[Tom Tromey :tromey]

Attachment: don't override property while editing

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8678220 [details] [diff] [review]
don't override property while editing

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

Nice and simple, fixes the reported problem and I don't see anything wrong with this approach.  Please update commit message to include reviewer

Comment 3

[Tom Tromey :tromey]

Attachment: don't override property while editing

Comment 4

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c36095cc98be

Comment 5

[Tom Tromey :tromey]

The revert-on-ESC tests failed here in the try run.
This happens because at the point at which the revert handler is run,
the popup is still open, meaning that the this.editing is true, even
though really editing has finished.

I'm investigating what to do, but I think maybe closing the popup first
will give the best behavior, in the sense of avoiding a dependency
by the rule-view on the internal behavior of the popup.

Comment 6

[Tom Tromey :tromey]

Attachment: don't override property while editing

Salvage the ".editing" approach by arranging for the revert callback
to be called only after the popup is closed.

However, this runs afoul of the editing check in _previewValue, so
also add a "reverting" flag there.

Comment 7

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef934c9be501

Comment 8

[Tom Tromey :tromey]

Comment on attachment 8679599 [details] [diff] [review]
don't override property while editing

A cleaner try run and maybe not too ugly.

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8679599 [details] [diff] [review]
don't override property while editing

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

Seems fine

Comment 10

[Pulsebot]

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

Comment 11

[Tom Tromey :tromey]

Comment on attachment 8679599 [details] [diff] [review]
don't override property while editing

Approval Request Comment
[Feature/regressing bug #]:
This fixes a regression introduced by bug 984880.
[User impact if declined]:
When adding a new property in the rule view, it will be marked as
invalid at first, even though it is actively being edited.  This is
confusing to users.
[Describe test coverage new/current, TreeHerder]:
There is a new test covering the new code; plus existing extensive
tests for the code that is being modified.
[Risks and why]: 
Reasonably low, IMO, due to the new test.
[String/UUID change made/needed]: No.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cd4361e0be3a

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8679599 [details] [diff] [review]
don't override property while editing

Given that we added a new test and have existing automated test coverage, approved for uplift to Aurora44.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/cd4361e0be3a

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3c27305c3ee6

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/3c27305c3ee6