Created attachment 363101 [details] [Image] Screenshot of Issue .
<rdar://problem/48440678>
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.
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.
Created attachment 363143 [details] Patch
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 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.
Created attachment 363149 [details] Patch
(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.
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 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
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
Created attachment 363186 [details] Patch
Created attachment 363187 [details] Patch
Created attachment 363190 [details] [Image] with patch applied New UI: - White outline around selected breakpoint status image - Indicator/marker on auto-continue breakpoint
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 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.
Created attachment 363256 [details] Patch for landing
(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 on attachment 363256 [details] Patch for landing Clearing flags on attachment: 363256 Committed r242228: <https://trac.webkit.org/changeset/242228>
All reviewed patches have been landed. Closing bug.