Bug 188119 – Web Inspector: Dark Mode: SourceCodeTextEditor thread indicator widget is too light

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

[Matt Baker]

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.

[Radar WebKit Bug Importer]

<rdar://problem/42670811>

[Nikita Vasilyev]

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/

[Nikita Vasilyev]

Created attachment 345978 [details]
Patch

[Nikita Vasilyev]

Created attachment 345979 [details]
[Screenshot] With patch applied

[EWS Watchlist]

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

[EWS Watchlist]

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

[Matt Baker]

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.

[Matt Baker]

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.

[Matt Baker]

Comment on attachment 345978 [details]
Patch

r-, only because of the color consistency issue mentioned above.

[Nikita Vasilyev]

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

[Nikita Vasilyev]

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

[Nikita Vasilyev]

Created attachment 346228 [details]
Patch

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

Created attachment 346242 [details]
Patch

[Nikita Vasilyev]

Created attachment 346243 [details]
[Image] With patch applied

[EWS Watchlist]

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

[EWS Watchlist]

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

[Matt Baker]

Comment on attachment 346242 [details]
Patch

r=me, very nice.

[WebKit Commit Bot]

Comment on attachment 346242 [details]
Patch

Clearing flags on attachment: 346242

Committed r234465: <https://trac.webkit.org/changeset/234465>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.