Bug 195103 – Web Inspector: Debugger: disabled breakpoint color is too dark

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

Created attachment 363101 [details]
[Image] Screenshot of Issue

.

[Radar WebKit Bug Importer]

<rdar://problem/48440678>

[Matt Baker]

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.

[Matt Baker]

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.

[Matt Baker]

Created attachment 363143 [details]
Patch

[Matt Baker]

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.

[Nikita Vasilyev]

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.

[Matt Baker]

Created attachment 363149 [details]
Patch

[Matt Baker]

(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.

[Matt Baker]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Matt Baker]

Created attachment 363186 [details]
Patch

[Matt Baker]

Created attachment 363187 [details]
Patch

[Matt Baker]

Created attachment 363190 [details]
[Image] with patch applied

New UI:
- White outline around selected breakpoint status image
- Indicator/marker on auto-continue breakpoint

[Devin Rousso]

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`.

[Matt Baker]

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.

[Matt Baker]

Created attachment 363256 [details]
Patch for landing

[Matt Baker]

(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.

[WebKit Commit Bot]

Comment on attachment 363256 [details]
Patch for landing

Clearing flags on attachment: 363256

Committed r242228: <https://trac.webkit.org/changeset/242228>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.