Bug 202253 - Web Inspector: Canvas: shader type header is white in dark mode
Summary: Web Inspector: Canvas: shader type header is white in dark mode
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: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-09-25 19:10 PDT by Devin Rousso
Modified: 2019-09-30 16:33 PDT (History)
6 users (show)

See Also:


Attachments
[Image] Screenshot of Issue (781.52 KB, image/png)
2019-09-25 19:10 PDT, Devin Rousso
no flags Details
Patch (1.92 KB, patch)
2019-09-25 19:13 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (2.17 KB, patch)
2019-09-30 15:02 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-09-25 19:10:23 PDT
Created attachment 379606 [details]
[Image] Screenshot of Issue

.
Comment 1 Devin Rousso 2019-09-25 19:13:19 PDT
Created attachment 379608 [details]
Patch
Comment 2 Nikita Vasilyev 2019-09-25 19:41:25 PDT
Comment on attachment 379608 [details]
Patch

It looks like <header> always has only one child.

Can this markup

    <header>
        <div class=shader-type">...</div>
    </header>

be replaced with

    <header class=shader-type">...</header>

?
Comment 3 Devin Rousso 2019-09-26 01:22:39 PDT
(In reply to Nikita Vasilyev from comment #2)
> Comment on attachment 379608 [details]
> Patch
> 
> It looks like <header> always has only one child.
I did this so that if we wanted to add additional editing items (e.g. an <input> to change a WebGPU shader pipeline's `entryPoint`), there'd be an easy way to do that.
Comment 4 Nikita Vasilyev 2019-09-26 11:13:35 PDT
Comment on attachment 379608 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379608&action=review

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.css:65
> +    .content-view.shader-program > .shader > header > * {

I try to avoid selectors ending on `*`. CSS selectors are applied right-to-left, so selectors ending on `*` aren't very effecient.

Looks like for this case `.content-view.shader-program > .shader > header` should be a sufficient selector.

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.css:69
> +    .content-view.shader-program > .shader > header > .shader-type {

Ditto. `.content-view.shader-program > .shader > header`.
Comment 5 Devin Rousso 2019-09-30 14:58:46 PDT
Comment on attachment 379608 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379608&action=review

>> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.css:65
>> +    .content-view.shader-program > .shader > header > * {
> 
> I try to avoid selectors ending on `*`. CSS selectors are applied right-to-left, so selectors ending on `*` aren't very effecient.
> 
> Looks like for this case `.content-view.shader-program > .shader > header` should be a sufficient selector.

Good point.  I'll merge this with the rule below (same with the light-mode variant).

>> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.css:69
>> +    .content-view.shader-program > .shader > header > .shader-type {
> 
> Ditto. `.content-view.shader-program > .shader > header`.

See comment #3:
> I did this so that if we wanted to add additional editing items (e.g. an <input> to change a WebGPU shader pipeline's `entryPoint`), there'd be an easy way to do that.
Comment 6 Devin Rousso 2019-09-30 15:02:46 PDT
Created attachment 379844 [details]
Patch
Comment 7 Nikita Vasilyev 2019-09-30 15:19:39 PDT
Comment on attachment 379608 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379608&action=review

Looks fine.

> Source/WebInspectorUI/ChangeLog:9
> +        (@media (prefers-color-scheme: dark) .content-view.shader-program > .shader > header > *): Added.

Nit: the changelog is out of date.

>>> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.css:69
>>> +    .content-view.shader-program > .shader > header > .shader-type {
>> 
>> Ditto. `.content-view.shader-program > .shader > header`.
> 
> See comment #3:

I saw it. I assumed it would go inside of the header.
Comment 8 Matt Baker 2019-09-30 15:48:17 PDT
Comment on attachment 379844 [details]
Patch

r=me
Comment 9 WebKit Commit Bot 2019-09-30 16:32:48 PDT
Comment on attachment 379844 [details]
Patch

Clearing flags on attachment: 379844

Committed r250533: <https://trac.webkit.org/changeset/250533>
Comment 10 WebKit Commit Bot 2019-09-30 16:32:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2019-09-30 16:33:20 PDT
<rdar://problem/55860197>