Bug 170281

Summary: Web Inspector: Styles sidebar warning icon appears inside property value text
Product: WebKit Reporter: Matt Baker <mattbaker>
Component: Web InspectorAssignee: Matt Baker <mattbaker>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, drousso, inspector-bugzilla-changes, timothy
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
[Video] misplaced warning icon
none
Patch
none
[Image] w/ patch applied none

Description Matt Baker 2017-03-30 04:25:13 PDT
Created attachment 305854 [details]
[Video] misplaced warning icon

Summary:
Styles sidebar warning icon appears inside property value text

Steps to Reproduce:
1. Inspect test case
2. Elements tab > expand <body>
3. Open Styles sidebar
4. Select first <p> child of <body>
  => Note warning icon for invalid CSS property value
5. Use arrow down/up keys to cycle through sibling <p> nodes
  => Warning icon tramples through the property value

Test Case:
<body>
<p style="color:3.14"></p>
<p style="color: 3.14"></p>
<p style="color:  3.14"></p>
<p style="color:   3.14"></p>
<p style="color:    3.14"></p>
<p style="color:     3.14"></p>
</body>
Comment 1 Matt Baker 2017-03-30 04:48:46 PDT
Regressed in https://bugs.webkit.org/show_bug.cgi?id=146617.
Comment 2 Matt Baker 2017-03-30 05:05:38 PDT
The position of the warning icon accounted for whitespace in the text while editing, but after CodeMirror updates and applies formatting the icon (and strikethrough range) is incorrect.

Placing an icon near the value is error prone (it's potentially a moving target during editing), and doesn't look very good.

Proposal:
I suggest we not move the warning icon based on the location of the error (property name vs value). It's error prone, and visually jarring. The strikethrough indicates the part of the property with the error.
Comment 3 Matt Baker 2017-03-30 05:10:04 PDT
Created attachment 305855 [details]
Patch
Comment 4 Matt Baker 2017-03-30 05:14:04 PDT
Created attachment 305856 [details]
[Image] w/ patch applied
Comment 5 Devin Rousso 2017-03-30 10:03:23 PDT
(In reply to Matt Baker from comment #2)
> The position of the warning icon accounted for whitespace in the text while
> editing, but after CodeMirror updates and applies formatting the icon (and
> strikethrough range) is incorrect.
> 
> Placing an icon near the value is error prone (it's potentially a moving
> target during editing), and doesn't look very good.
> 
> Proposal:
> I suggest we not move the warning icon based on the location of the error
> (property name vs value). It's error prone, and visually jarring. The
> strikethrough indicates the part of the property with the error.

I personally would rather have the warning icon match the position of the strikethrough.  I think a simpler solution to this would be to place the warning icon immediately after the semicolon and before all spacing, since we know that the spaces will be collapsed anyways.  Having the warning icon always be at the beginning of the text implies to me an issue with the property as a whole (such as the name not existing), not just it's value.
Comment 6 WebKit Commit Bot 2017-03-30 13:17:18 PDT
Comment on attachment 305855 [details]
Patch

Clearing flags on attachment: 305855

Committed r214617: <http://trac.webkit.org/changeset/214617>
Comment 7 WebKit Commit Bot 2017-03-30 13:17:22 PDT
All reviewed patches have been landed.  Closing bug.