Bug 183097 - Web Inspector: Styles: Newly added unsupported properties sometimes don't have warnings
Summary: Web Inspector: Styles: Newly added unsupported properties sometimes don't hav...
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: 2018-02-23 16:41 PST by Nikita Vasilyev
Modified: 2018-05-04 13:51 PDT (History)
5 users (show)

See Also:


Attachments
[Animated GIF] Bug (226.66 KB, image/gif)
2018-02-23 16:41 PST, Nikita Vasilyev
no flags Details
Patch (3.63 KB, patch)
2018-03-16 13:50 PDT, Nikita Vasilyev
mattbaker: review-
Details | Formatted Diff | Diff
Patch (3.63 KB, patch)
2018-05-01 16:11 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews205 for win-future (12.74 MB, application/zip)
2018-05-01 18:08 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2018-02-23 16:41:57 PST
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!
Comment 1 Radar WebKit Bug Importer 2018-02-23 16:42:12 PST
<rdar://problem/37843816>
Comment 2 Nikita Vasilyev 2018-03-16 13:50:46 PDT
Created attachment 335965 [details]
Patch
Comment 3 Matt Baker 2018-03-16 15:34:03 PDT
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.
Comment 4 Matt Baker 2018-03-16 15:44:22 PDT
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.
Comment 5 Nikita Vasilyev 2018-04-30 18:19:11 PDT
(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.
Comment 6 Nikita Vasilyev 2018-05-01 16:11:42 PDT
Created attachment 339237 [details]
Patch

Setting back to "r?" as per the comment above.
Comment 7 EWS Watchlist 2018-05-01 18:07:49 PDT
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
Comment 8 EWS Watchlist 2018-05-01 18:08:00 PDT
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
Comment 9 Matt Baker 2018-05-04 13:47:03 PDT
Comment on attachment 339237 [details]
Patch

r=me
Comment 10 WebKit Commit Bot 2018-05-04 13:51:52 PDT
Comment on attachment 339237 [details]
Patch

Clearing flags on attachment: 339237

Committed r231377: <https://trac.webkit.org/changeset/231377>
Comment 11 WebKit Commit Bot 2018-05-04 13:51:54 PDT
All reviewed patches have been landed.  Closing bug.