[webkit-reviews] review granted: [Bug 195103] Web Inspector: Debugger: disabled breakpoint color is too dark : [Attachment 363187] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Feb 28 10:33:40 PST 2019

Devin Rousso <drousso at apple.com> has granted Matt Baker <mattbaker at apple.com>'s
request for review:
Bug 195103: Web Inspector: Debugger: disabled breakpoint color is too dark

Attachment 363187: Patch


--- Comment #15 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 363187
  --> https://bugs.webkit.org/attachment.cgi?id=363187

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.

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`

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

More information about the webkit-reviews mailing list