Bug 188119 - Web Inspector: Dark Mode: SourceCodeTextEditor thread indicator widget is too light
Summary: Web Inspector: Dark Mode: SourceCodeTextEditor thread indicator widget is too...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks: 188126
  Show dependency treegraph
 
Reported: 2018-07-27 12:41 PDT by Matt Baker
Modified: 2018-08-01 11:12 PDT (History)
4 users (show)

See Also:


Attachments
[Image] SourceCodeTextEditor thread indicator (306.38 KB, image/png)
2018-07-27 12:41 PDT, Matt Baker
no flags Details
Patch (8.00 KB, patch)
2018-07-27 17:16 PDT, Nikita Vasilyev
mattbaker: review-
Details | Formatted Diff | Diff
[Screenshot] With patch applied (30.70 KB, image/png)
2018-07-27 17:17 PDT, Nikita Vasilyev
no flags Details
Archive of layout-test-results from ews206 for win-future (13.14 MB, application/zip)
2018-07-28 06:12 PDT, EWS Watchlist
no flags Details
[Image] Light vs dark indicators (21.96 KB, image/png)
2018-07-30 12:00 PDT, Matt Baker
no flags Details
Patch (7.52 KB, patch)
2018-07-31 17:01 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
[Image] With patch applied (30.03 KB, image/png)
2018-07-31 17:05 PDT, Nikita Vasilyev
no flags Details
Patch (8.28 KB, patch)
2018-07-31 18:16 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
[Image] With patch applied (36.99 KB, image/png)
2018-07-31 18:17 PDT, Nikita Vasilyev
no flags Details
Archive of layout-test-results from ews206 for win-future (12.95 MB, application/zip)
2018-07-31 20:44 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2018-07-27 12:41:40 PDT
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.
Comment 1 Radar WebKit Bug Importer 2018-07-27 12:42:04 PDT
<rdar://problem/42670811>
Comment 2 Nikita Vasilyev 2018-07-27 16:05:08 PDT
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/
Comment 3 Nikita Vasilyev 2018-07-27 17:16:39 PDT
Created attachment 345978 [details]
Patch
Comment 4 Nikita Vasilyev 2018-07-27 17:17:35 PDT
Created attachment 345979 [details]
[Screenshot] With patch applied
Comment 5 EWS Watchlist 2018-07-28 06:12:19 PDT
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
Comment 6 EWS Watchlist 2018-07-28 06:12:32 PDT
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 7 Matt Baker 2018-07-30 11:54:55 PDT
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.
Comment 8 Matt Baker 2018-07-30 12:00:28 PDT
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 9 Matt Baker 2018-07-30 12:01:28 PDT
Comment on attachment 345978 [details]
Patch

r-, only because of the color consistency issue mentioned above.
Comment 10 Nikita Vasilyev 2018-07-30 12:02:22 PDT
(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.
Comment 11 Nikita Vasilyev 2018-07-30 12:05:35 PDT
(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.
Comment 12 Nikita Vasilyev 2018-07-31 17:01:36 PDT
Created attachment 346228 [details]
Patch
Comment 13 Nikita Vasilyev 2018-07-31 17:05:12 PDT
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.
Comment 14 Nikita Vasilyev 2018-07-31 18:16:11 PDT
Created attachment 346242 [details]
Patch
Comment 15 Nikita Vasilyev 2018-07-31 18:17:21 PDT
Created attachment 346243 [details]
[Image] With patch applied
Comment 16 EWS Watchlist 2018-07-31 20:44:43 PDT
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
Comment 17 EWS Watchlist 2018-07-31 20:44:55 PDT
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 18 Matt Baker 2018-08-01 10:45:30 PDT
Comment on attachment 346242 [details]
Patch

r=me, very nice.
Comment 19 WebKit Commit Bot 2018-08-01 11:12:24 PDT
Comment on attachment 346242 [details]
Patch

Clearing flags on attachment: 346242

Committed r234465: <https://trac.webkit.org/changeset/234465>
Comment 20 WebKit Commit Bot 2018-08-01 11:12:25 PDT
All reviewed patches have been landed.  Closing bug.