Bug 183097 – Web Inspector: Styles: Newly added unsupported properties sometimes don't have warnings

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Created attachment 334553 [details]
[Animated GIF] Bug

Steps:
1. Open http://nv.github.io/webkit-inspector-bugs/styles-redesign/tests/color.html
2. Inspect .item-1
3. Click after "Style Attribute {" to add a new property
4. Type "foo: bar"
5. Click on the white space right next to "foo: bar"

Expected:
"foo: bar" is shown is invalid.

Actual:
"foo: bar" is shown as a valid property.

Notes:
If you replace the 5th step with pressing Tab or Enter, it works as expected!

[Radar WebKit Bug Importer]

<rdar://problem/37843816>

[Nikita Vasilyev]

Created attachment 335965 [details]
Patch

[Matt Baker]

Comment on attachment 335965 [details]
Patch

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

This fixes the issue, but I'm not sure it is the right approach so r- for now. While debugging I noticed the following:

1. Click after "Style Attribute {" to add new property
2. Type "foo: b", then hit backspace to clear "b" from the value field.
3. Type "b" in value field again
  => Warning appears

So the first time text is entered in the new property's value field, it isn't validated. But clearing the field and starting to type again triggers an update. This seems wrong.

> Source/WebInspectorUI/ChangeLog:9
> +        Update properties warnings every time focus moves.

I'd reword this, since SpreadsheetStyleProperty.updateStatus does more than update warnings:

"Update status of properties warnings every time focus moves.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:232
> +        // Focus moved by clicking outside of the currently focused property.

This comment is inaccurate. The direction will also be null when this is called in response to a property text field "blur" event.

[Matt Baker]

Comment on attachment 335965 [details]
Patch

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

>> Source/WebInspectorUI/ChangeLog:9
>> +        Update properties warnings every time focus moves.
> 
> I'd reword this, since SpreadsheetStyleProperty.updateStatus does more than update warnings:
> 
> "Update status of properties warnings every time focus moves.

Sorry, ignore my original comment. I started writing it before I'd finished the review.

[Nikita Vasilyev]

(In reply to Matt Baker from comment #3)
> Comment on attachment 335965 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335965&action=review
> 
> This fixes the issue, but I'm not sure it is the right approach so r- for
> now. While debugging I noticed the following:
> 
> 1. Click after "Style Attribute {" to add new property
> 2. Type "foo: b", then hit backspace to clear "b" from the value field.
> 3. Type "b" in value field again
>   => Warning appears
> 
> So the first time text is entered in the new property's value field, it
> isn't validated. But clearing the field and starting to type again triggers
> an update. This seems wrong.

In the past, I experimented with showing all warnings immediately as I type. It felt obnoxious.

Imagine typing:
colo <- Unsupported property name
color: r <- Unsupported property value
color: re <- Unsupported property value
color: red

The warnings flash before I finish typing a valid CSS property.

The case you found doesn't seem that odd to me. I don't see warning flashing for new properties
but I can see errors right away for existing properties. The logic is a bit complex, but it seems
like a lesser evil to me.

[Nikita Vasilyev]

Created attachment 339237 [details]
Patch

Setting back to "r?" as per the comment above.

[EWS Watchlist]

Comment on attachment 339237 [details]
Patch

Attachment 339237 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7530685

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

[EWS Watchlist]

Created attachment 339250 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Matt Baker]

Comment on attachment 339237 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 339237 [details]
Patch

Clearing flags on attachment: 339237

Committed r231377: <https://trac.webkit.org/changeset/231377>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.