Bug 178488 - Web Inspector: Styles Redesign: Newly added invalid property isn't immediately shown as invalid
Summary: Web Inspector: Styles Redesign: Newly added invalid property isn't immediatel...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-10-18 15:22 PDT by Nikita Vasilyev
Modified: 2017-10-25 12:02 PDT (History)
4 users (show)

See Also:


Attachments
[Animated GIF] Bug (58.10 KB, image/gif)
2017-10-18 15:23 PDT, Nikita Vasilyev
no flags Details
Patch (8.25 KB, patch)
2017-10-23 17:54 PDT, Nikita Vasilyev
bburg: review+
bburg: commit-queue-
Details | Formatted Diff | Diff
[Animated GIF] With patch applied (81.56 KB, image/gif)
2017-10-23 17:55 PDT, Nikita Vasilyev
no flags Details
Patch (8.27 KB, patch)
2017-10-25 11:25 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.