Bug 195103

Summary: Web Inspector: Debugger: disabled breakpoint color is too dark
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Matt Baker <mattbaker>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, hi, inspector-bugzilla-changes, mattbaker, nvasilyev, rniwa, webkit-bug-importer
Priority: P3 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 193507    
Bug Blocks:    
Attachments:
Description Flags
[Image] Screenshot of Issue
none
[Image] Breakpoint color comparison
none
[Image] CSS filter test results
none
Patch
none
[Image] Patch applied
none
Patch
none
[Video] Disabled breakpoint - light/dark/accents colors
none
Archive of layout-test-results from ews107 for mac-highsierra-wk2
none
Patch
none
Patch
none
[Image] with patch applied
none
Patch for landing none

Description Devin Rousso 2019-02-27 10:08:37 PST
Created attachment 363101 [details]
[Image] Screenshot of Issue

.
Comment 1 Radar WebKit Bug Importer 2019-02-27 10:11:03 PST
<rdar://problem/48440678>
Comment 2 Matt Baker 2019-02-27 13:42:36 PST
Created attachment 363124 [details]
[Image] Breakpoint color comparison

Comparing breakpoints in current Web Inspector and Xcode 10.2 (beta 2).

In Web Inspector we decrease the opacity of disabled breakpoints. This has only ever looked good for the unselected/light mode case.

Note: Xcode uses the system accent color for selected item background color.
Comment 3 Matt Baker 2019-02-27 15:22:31 PST
Created attachment 363138 [details]
[Image] CSS filter test results

Comparison of old (opacity) and new (filter) disabled breakpoint styles, shown next to Xcode color swatches.

Separate rules are required for light/dark modes.
Comment 4 Matt Baker 2019-02-27 15:37:13 PST
Created attachment 363143 [details]
Patch
Comment 5 Matt Baker 2019-02-27 15:41:21 PST
Created attachment 363146 [details]
[Image] Patch applied

opacity vs filter vs Xcode

An improvement, but the contrast is still too low for dark mode Inspector.

Also, auto continue breakpoints are styled with opacity and have always been extremely difficult to distinguish from disabled breakpoints.
Comment 6 Nikita Vasilyev 2019-02-27 15:47:57 PST
Comment on attachment 363143 [details]
Patch

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

Looks good!

> Source/WebInspectorUI/UserInterface/Views/BreakpointTreeElement.css:45
> +    filter: saturate(0.2) brightness(1.75);

I noticed that with yellow accent, disabled breakpoints in the sidebar are completely white. Yellow accent color has so many contrast issues that I don't even know if we should care about this case.
Comment 7 Matt Baker 2019-02-27 15:50:38 PST
Created attachment 363149 [details]
Patch
Comment 8 Matt Baker 2019-02-27 15:51:16 PST
(In reply to Matt Baker from comment #7)
> Created attachment 363149 [details]
> Patch

This looks pretty good for the default (blue) accent. I'll try the others now.
Comment 9 Matt Baker 2019-02-27 16:39:51 PST
Created attachment 363154 [details]
[Video] Disabled breakpoint - light/dark/accents colors

Taking a different approach.

The system derives a color from the control accent, -apple-system-selected-text-background, which works perfectly for disabled breakpoints for all mode/accent combinations.

Need to make a minor adjustment to make it work with pre-Mojave.
Comment 10 EWS Watchlist 2019-02-27 17:14:40 PST
Comment on attachment 363149 [details]
Patch

Attachment 363149 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11309764

New failing tests:
imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-constructor.html
Comment 11 EWS Watchlist 2019-02-27 17:14:42 PST
Created attachment 363161 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 12 Matt Baker 2019-02-27 19:50:45 PST
Created attachment 363186 [details]
Patch
Comment 13 Matt Baker 2019-02-27 19:53:56 PST
Created attachment 363187 [details]
Patch
Comment 14 Matt Baker 2019-02-27 20:47:58 PST
Created attachment 363190 [details]
[Image] with patch applied

New UI:
- White outline around selected breakpoint status image
- Indicator/marker on auto-continue breakpoint
Comment 15 Devin Rousso 2019-02-28 10:33:40 PST
Comment on attachment 363187 [details]
Patch

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

r=me, I like it!

Something I did notice, however, is that the breakpoint icon flickers when enabling it from a disabled state.

# STEPS TO REPRODUCE:
1. inspect any page
2. set a breakpoint on any line
3. click the breakpoint to disable it (either from the navigation sidebar or from a text editor gutter)
4. click the breakpoint again to enable it
 => the breakpoint icon briefly shows a black color before it's expected dark blue color

> Source/WebInspectorUI/UserInterface/Views/BreakpointTreeElement.css:41
> +    stroke: var(--selected-foreground-color);

We should only apply this if the element also has focus.

> Source/WebInspectorUI/UserInterface/Views/BreakpointTreeElement.css:48
> +    right: 8px;

NIT: I normally put `right` alongside positional properties (e.g. `top`).

> Source/WebInspectorUI/UserInterface/Views/BreakpointTreeElement.css:49
> +

Style: unnecessary whitespace.

> Source/WebInspectorUI/UserInterface/Views/BreakpointTreeElement.css:51
> +

Ditto (>49).

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.css:67
>  body:not(.window-inactive) .content-view.dom-tree .tree-outline.dom:focus li.selected .status-image.breakpoint {

We should probably still set this even if the element isn't `.selected`.  Otherwise, the disabled color and the hovered color are too similar.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.css:72
> +    fill: var(--breakpoint-color-disabled);

This doesn't mix well if `.breakpoints-disabled` is also applied and the current element isn't focused.  Setting the stroke as mentioned on 67 does make this a less of an issue though.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.css:81
> -    stroke: var(--glyph-color-active);
> +    stroke: var(--breakpoint-color);

This is almost impossible to see if the element is selected.  If anything, I'd imagine it to maybe be a hollow arrow (e.g. a `--background-color-content` fill).

> Source/WebInspectorUI/UserInterface/Views/TextEditor.css:76
> +

Style: unnecessary whitespace.

> Source/WebInspectorUI/UserInterface/Views/TextEditor.css:78
> +

Ditto (>76).

> Source/WebInspectorUI/UserInterface/Views/TextEditor.css:79
> +    z-index: -1;

NIT: I normally put `z-index` after positional properties (e.g. `top`).

> Source/WebInspectorUI/UserInterface/Views/TextEditor.css:80
> +

Ditto (>76).

> Source/WebInspectorUI/UserInterface/Views/TextEditor.css:83
> +    background-color: var(--selected-foreground-color);

NIT: I'd put `background-color` above `transform`.
Comment 16 Matt Baker 2019-02-28 13:08:21 PST
Comment on attachment 363187 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/BreakpointTreeElement.css:41
>> +    stroke: var(--selected-foreground-color);
> 
> We should only apply this if the element also has focus.

Will change.

>> Source/WebInspectorUI/UserInterface/Views/BreakpointTreeElement.css:48
>> +    right: 8px;
> 
> NIT: I normally put `right` alongside positional properties (e.g. `top`).

Makes sense.

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.css:67
>>  body:not(.window-inactive) .content-view.dom-tree .tree-outline.dom:focus li.selected .status-image.breakpoint {
> 
> We should probably still set this even if the element isn't `.selected`.  Otherwise, the disabled color and the hovered color are too similar.

I'll change this and the "hollow" subtree DOM breakpoint style to set their fill and stroke (respectively) to `--selected-foregraound-color` when selected or hovered.

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.css:72
>> +    fill: var(--breakpoint-color-disabled);
> 
> This doesn't mix well if `.breakpoints-disabled` is also applied and the current element isn't focused.  Setting the stroke as mentioned on 67 does make this a less of an issue though.

I'll change the rule to `.content-view.dom-tree .tree-outline.dom:not(.breakpoints-disabled) li .status-image.breakpoint.disabled`. I don't think it's useful to have to shades of gray breakpoints.

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.css:81
>> +    stroke: var(--breakpoint-color);
> 
> This is almost impossible to see if the element is selected.  If anything, I'd imagine it to maybe be a hollow arrow (e.g. a `--background-color-content` fill).

Fixed.

>> Source/WebInspectorUI/UserInterface/Views/TextEditor.css:83
>> +    background-color: var(--selected-foreground-color);
> 
> NIT: I'd put `background-color` above `transform`.

Makes sense.
Comment 17 Matt Baker 2019-02-28 13:08:39 PST
Created attachment 363256 [details]
Patch for landing
Comment 18 Matt Baker 2019-02-28 13:14:45 PST
(In reply to Devin Rousso from comment #15)
> Comment on attachment 363187 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363187&action=review
> 
> r=me, I like it!
> 
> Something I did notice, however, is that the breakpoint icon flickers when
> enabling it from a disabled state.

I couldn't reproduce this but will keep an eye out for it.
Comment 19 WebKit Commit Bot 2019-02-28 14:32:43 PST
Comment on attachment 363256 [details]
Patch for landing

Clearing flags on attachment: 363256

Committed r242228: <https://trac.webkit.org/changeset/242228>
Comment 20 WebKit Commit Bot 2019-02-28 14:32:45 PST
All reviewed patches have been landed.  Closing bug.