Bug 178488

Summary: Web Inspector: Styles Redesign: Newly added invalid property isn't immediately shown as invalid
Product: WebKit Reporter: Nikita Vasilyev <nvasilyev>
Component: Web InspectorAssignee: Nikita Vasilyev <nvasilyev>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, inspector-bugzilla-changes, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
[Animated GIF] Bug
none
Patch
bburg: review+, bburg: commit-queue-
[Animated GIF] With patch applied
none
Patch none

Description Nikita Vasilyev 2017-10-18 15:22:30 PDT
Steps:
1. Add a property by "foo"
2. Press Enter
3. Type value name "bar"
4. Press Enter

Expected:
Property "foo: bar;" has a red strike-through.

Actual:
Property "foo: bar;" doesn't have a red strike-through.

Updating the rule by toggling some other properties makes it show the red strike-through as expected.
Comment 1 Nikita Vasilyev 2017-10-18 15:23:15 PDT
Created attachment 324175 [details]
[Animated GIF] Bug
Comment 2 Radar WebKit Bug Importer 2017-10-18 15:23:44 PDT
<rdar://problem/35062131>
Comment 3 Nikita Vasilyev 2017-10-23 17:54:26 PDT
Created attachment 324620 [details]
Patch
Comment 4 Nikita Vasilyev 2017-10-23 17:55:39 PDT
Created attachment 324621 [details]
[Animated GIF] With patch applied

HTML page for manual testing: http://nv.github.io/webkit-inspector-bugs/styles-redesign/tests/invalid.html
Comment 5 BJ Burg 2017-10-25 09:06:42 PDT
Comment on attachment 324620 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=324620&action=review

r=me

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:367
> +        let index = insertAfterIndex + 1;

Nit: propertyIndex

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:371
> +        const dontFireEvents = true;

Nit: suppressEvents or suppressEventDispatch

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:-128
> -        this._element.classList.add(...classNames);

I'd keep this as-is, unless there is some reason not to.
Comment 6 Nikita Vasilyev 2017-10-25 11:24:15 PDT
Comment on attachment 324620 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=324620&action=review

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:-128
>> -        this._element.classList.add(...classNames);
> 
> I'd keep this as-is, unless there is some reason not to.

It used to be:

    this.element.className = "";
    ...
    this._element.classList.add(...classNames);

I replaced it with

    this._element.className = classNames.join(" ");

to make it more concise.
Comment 7 Nikita Vasilyev 2017-10-25 11:25:51 PDT
Created attachment 324856 [details]
Patch
Comment 8 WebKit Commit Bot 2017-10-25 12:02:32 PDT
Comment on attachment 324856 [details]
Patch

Clearing flags on attachment: 324856

Committed r223970: <https://trac.webkit.org/changeset/223970>
Comment 9 WebKit Commit Bot 2017-10-25 12:02:34 PDT
All reviewed patches have been landed.  Closing bug.