Bug 170281 - Web Inspector: Styles sidebar warning icon appears inside property value text
Summary: Web Inspector: Styles sidebar warning icon appears inside property value text
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: Matt Baker
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-30 04:25 PDT by Matt Baker
Modified: 2017-03-30 13:17 PDT (History)
4 users (show)

See Also:


Attachments
[Video] misplaced warning icon (306.87 KB, video/mp4)
2017-03-30 04:25 PDT, Matt Baker
no flags Details
Patch (3.22 KB, patch)
2017-03-30 05:10 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
[Image] w/ patch applied (100.78 KB, image/png)
2017-03-30 05:14 PDT, Matt Baker
no flags Details

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