| Summary: | Web Inspector: Dark Mode: SourceCodeTextEditor thread indicator widget is too light | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Matt Baker <mattbaker> | ||||||||||||||||||||||
| Component: | Web Inspector | Assignee: | Nikita Vasilyev <nvasilyev> | ||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||
| Severity: | Normal | CC: | commit-queue, ews-watchlist, inspector-bugzilla-changes, webkit-bug-importer | ||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||
| Bug Blocks: | 188126 | ||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
This is good time to refactor HTML and CSS:
<div class="line-indicator-widget thread-widget inline">
<span class="arrow"></span>
<span class="text">Page</span>
</div>
The arrow is drawn using CSS borders (https://css-tricks.com/snippets/css/css-triangle/), the 2000s tech :)
We can get rid of the arrow div and use masks via clip-path. https://bennettfeely.com/clippy/
Created attachment 345978 [details]
Patch
Created attachment 345979 [details]
[Screenshot] With patch applied
Comment on attachment 345978 [details] Patch Attachment 345978 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8682620 New failing tests: http/tests/security/video-poster-cross-origin-crash2.html Created attachment 345991 [details]
Archive of layout-test-results from ews206 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 345978 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=345978&action=review > Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.css:57 > + -webkit-clip-path: polygon(0% 50%, 8px 100%, 100% 100%, 100% 0, 8px 0); Nice! Can't we drop the -webkit prefix? > Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.css:190 > +} Why can't this go in DarkMode.css? Is it a priority issue? > Source/WebInspectorUI/UserInterface/Views/Variables.css:35 > + --text-color: black; This var isn't being used. Created attachment 346080 [details]
[Image] Light vs dark indicators
I think the target indicator and expression highlight colors should match, like they do in light mode (see screenshot). Looking closer, the gutter indicator is slightly different, but I had to sample pixels to tell.
I like the bright green you used for the target indicator. Maybe we could use that color for the other parts too.
Comment on attachment 345978 [details]
Patch
r-, only because of the color consistency issue mentioned above.
(In reply to Matt Baker from comment #7) > Comment on attachment 345978 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=345978&action=review > > > Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.css:57 > > + -webkit-clip-path: polygon(0% 50%, 8px 100%, 100% 100%, 100% 0, 8px 0); > > Nice! Can't we drop the -webkit prefix? Unfortunately not. In WebKit, clip-path without the prefix appears to only work for SVG. (In reply to Matt Baker from comment #8) > Created attachment 346080 [details] > [Image] Light vs dark indicators > > I think the target indicator and expression highlight colors should match, > like they do in light mode (see screenshot). Looking closer, the gutter > indicator is slightly different, but I had to sample pixels to tell. > > I like the bright green you used for the target indicator. Maybe we could > use that color for the other parts too. I like the bright green, too. I think. Since it has higher luminosity, I need to test how it works with the syntax highlighting. Created attachment 346228 [details]
Patch
Created attachment 346229 [details] [Image] With patch applied (In reply to Matt Baker from comment #8) > I like the bright green you used for the target indicator. Maybe we could > use that color for the other parts too. I used a slightly darker green for the execution range highlight so the code has enough contract with the background. Created attachment 346242 [details]
Patch
Created attachment 346243 [details]
[Image] With patch applied
Comment on attachment 346242 [details] Patch Attachment 346242 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8718888 New failing tests: imported/blink/transitions/unprefixed-transform.html Created attachment 346252 [details]
Archive of layout-test-results from ews206 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 346242 [details]
Patch
r=me, very nice.
Comment on attachment 346242 [details] Patch Clearing flags on attachment: 346242 Committed r234465: <https://trac.webkit.org/changeset/234465> All reviewed patches have been landed. Closing bug. |
Created attachment 345937 [details] [Image] SourceCodeTextEditor thread indicator Summary: SourceCodeTextEditor thread indicator widget is too light. Note: The thread indicator is only shown when debugging pages with multiple threads (workers), such as the Inspector.