Bug 189673 - Web Inspector: move DarkMode.css rules into appropriate CSS files
Summary: Web Inspector: move DarkMode.css rules into appropriate CSS files
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: 189766
  Show dependency treegraph
 
Reported: 2018-09-17 13:00 PDT by Nikita Vasilyev
Modified: 2018-09-21 13:23 PDT (History)
4 users (show)

See Also:


Attachments
WIP (134.51 KB, text/plain)
2018-09-17 13:06 PDT, Nikita Vasilyev
nvasilyev: review-
nvasilyev: commit-queue-
Details
[Screenshot] WIP (100.81 KB, image/png)
2018-09-17 13:07 PDT, Nikita Vasilyev
no flags Details
WIP (134.85 KB, patch)
2018-09-17 13:11 PDT, Nikita Vasilyev
nvasilyev: review-
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
Patch (135.58 KB, patch)
2018-09-19 15:32 PDT, Nikita Vasilyev
mattbaker: review+
Details | Formatted Diff | Diff
Patch (134.54 KB, patch)
2018-09-19 16:52 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2018-09-17 13:00:05 PDT
Move all CSS rules from DarkMode.css to appropriate CSS files, then remove DarkMode.css.

DarkMode.css consists of many sections, e.g.:

    /* ImageResourceContentView.css */
    .content-view.resource.image {
        background: var(--background-color-content);
    }

This CSS rule, for example, should move to ImageResourceContentView.css.

<rdar://problem/43648004>
Comment 1 Nikita Vasilyev 2018-09-17 13:06:44 PDT
Created attachment 349923 [details]
WIP

Well, this broke a few things since the order of the style rules changes. Priorities of some CSS rules need to be adjusted.
Comment 2 Nikita Vasilyev 2018-09-17 13:07:15 PDT
Created attachment 349924 [details]
[Screenshot] WIP
Comment 3 Nikita Vasilyev 2018-09-17 13:11:32 PDT
Created attachment 349927 [details]
WIP
Comment 4 Nikita Vasilyev 2018-09-19 15:32:01 PDT
Created attachment 350152 [details]
Patch

I filed Bug 189766 - Web Inspector: Dark Mode: use the same CSS variables for dark and light modes,
which depends on this patch.
Comment 5 Matt Baker 2018-09-19 16:14:10 PDT
Comment on attachment 350152 [details]
Patch

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

r=me, with a few minor things.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.css:336
> +    but close enough for now. It needs to use partial translucency so that the selection area shines through. */

Is there a bug that can be referenced here?

> Source/WebInspectorUI/UserInterface/Views/Main.css:136
> +    background-color: var(--background-color-content);

Can this change go in https://webkit.org/b/189766 instead?

> Source/WebInspectorUI/UserInterface/Views/SearchSidebarPanel.css:82
> +@media all {

So the banner style applies to both light and dark mode? Why do we need "@media all" here?

> Source/WebInspectorUI/UserInterface/Views/TabBar.css:368
> +        /* FIXME: Use semantic colors */

Is there a bug that can be referenced here?

> Source/WebInspectorUI/UserInterface/Views/Variables.css:144
> +    /* FIXME: Use CSS4 color functions once they're available. */

Could you file a bug for this and add a reference?

> Source/WebInspectorUI/UserInterface/Views/Variables.css:266
> +        /* FIXME: these properties are duplicated so that they have higher specificity than WebKit's stylesheet.

If the workaround isn't temporary, you can drop the FIXME from this explanatory comment.
Comment 6 Nikita Vasilyev 2018-09-19 16:50:27 PDT
Comment on attachment 350152 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SearchSidebarPanel.css:82
>> +@media all {
> 
> So the banner style applies to both light and dark mode? Why do we need "@media all" here?

Whoops! Should be prefers-dark-interface!

>> Source/WebInspectorUI/UserInterface/Views/Variables.css:144
>> +    /* FIXME: Use CSS4 color functions once they're available. */
> 
> Could you file a bug for this and add a reference?

AFAIK, there no syntax for this that vendors agreed on. I'll just remove this comment.
Comment 7 Nikita Vasilyev 2018-09-19 16:52:46 PDT
Created attachment 350160 [details]
Patch
Comment 8 WebKit Commit Bot 2018-09-19 19:31:50 PDT
Comment on attachment 350160 [details]
Patch

Clearing flags on attachment: 350160

Committed r236237: <https://trac.webkit.org/changeset/236237>
Comment 9 WebKit Commit Bot 2018-09-19 19:31:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Nikita Vasilyev 2018-09-21 13:23:11 PDT
(In reply to Matt Baker from comment #5)
> > Source/WebInspectorUI/UserInterface/Views/Main.css:136
> > +    background-color: var(--background-color-content);
> 
> Can this change go in https://webkit.org/b/189766 instead?

I re-added it in Bug 189852 - REGRESSION(r236237): Web Inspector: DarkMode: white background in Elements and Timelines 😬