Bug 198033

Summary: ErrorObjectView display black link in dark mode
Product: WebKit Reporter: Zhifei Fang <zhifei_fang>
Component: Web InspectorAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, hi, inspector-bugzilla-changes, nvasilyev, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
link in black
none
Patch
none
[HTML] Reduction
none
[Image] Screenshot of reduction
none
No black link
none
Patch
none
Patch none

Description Zhifei Fang 2019-05-20 00:51:11 PDT
Created attachment 370241 [details]
link in black

<html>
    <body>
        <script>
            console.log(new Error("Wrong style in dark mode"));
        </script>
    </body>
</html>


see output in attachment
Comment 1 Zhifei Fang 2019-05-20 01:03:12 PDT
Created attachment 370242 [details]
Patch
Comment 2 Zhifei Fang 2019-05-20 01:04:28 PDT
Please also let me know how should I test my patch.

How to setup a local web inspector instance.

thanks!
Comment 3 Nikita Vasilyev 2019-05-20 01:51:55 PDT
(In reply to Zhifei Fang from comment #0)
> Created attachment 370241 [details]
> link in black

What version of Safari are you using? This isn't what I'm seeing in Safari Technology Preview 82.
Comment 4 Nikita Vasilyev 2019-05-20 01:52:22 PDT
Created attachment 370243 [details]
[HTML] Reduction
Comment 5 Nikita Vasilyev 2019-05-20 01:54:04 PDT
Created attachment 370244 [details]
[Image] Screenshot of reduction

The link is appropriate color. The em dash is black and it shouldn't be!
Comment 6 Zhifei Fang 2019-05-20 01:59:13 PDT
I tried 
Release 82 (Safari 12.2, WebKit 14608.1.23.1)

it is still like that, you will need first go to console tab, and then a refresh to see the link.
Comment 7 Zhifei Fang 2019-05-20 02:02:54 PDT
it is also weird that if you first open the file, and then open inspector, it doesn't give me that link...
Comment 8 Zhifei Fang 2019-05-20 02:03:26 PDT
Created attachment 370245 [details]
No black link
Comment 9 Nikita Vasilyev 2019-05-20 02:07:16 PDT
(In reply to Zhifei Fang from comment #6)
> I tried 
> Release 82 (Safari 12.2, WebKit 14608.1.23.1)
> 
> it is still like that, you will need first go to console tab, and then a
> refresh to see the link.

Hm, I still don't see it.

On your 1st screenshot you somehow have:

<span class="error-object-link-container">
  — link.html:1
</span>

Instead of 

<span class="error-object-link-container">
  —
  <a href="..." class="error-object-link">link.html:1</a>
</span>

I'm not sure why I can't get into this state but we should fix the text color issue regardless.
Comment 10 Nikita Vasilyev 2019-05-20 02:08:41 PDT
(In reply to Zhifei Fang from comment #2)
> Please also let me know how should I test my patch.
> 
> How to setup a local web inspector instance.
> 
> thanks!

You'll need to build WebKit locally.
https://trac.webkit.org/wiki/HackingWebInspector
Please let me know if this is helpful.
Comment 11 Nikita Vasilyev 2019-05-20 02:11:44 PDT
Comment on attachment 370242 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ErrorObjectView.css:60
> +@media (prefers-dark-interface) {
> +    .error-object-link-container {
> +        color: var(--text-color);
> +    }
> +}

Generally, we try to use the same variables for both dark and light modes.
I recommend modifying the existing .error-object-link-container CSS rule to use --text-color-secondary variables.
Comment 12 Zhifei Fang 2019-05-20 11:16:51 PDT
Created attachment 370265 [details]
Patch
Comment 13 Devin Rousso 2019-05-20 13:53:35 PDT
Comment on attachment 370265 [details]
Patch

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

r-

> Source/WebInspectorUI/UserInterface/Views/ErrorObjectView.css:57
> +    .error-object-link-container {

This doesn't fix the issue for me, as there's a `.console-message .go-to-arrow` that has a higher specificity.  Please match the styling of `.call-frame .subtitle`, `.call-frame .separator`, and `.call-frame .subtitle .source-link` so we have the same coloring everywhere.

I also agree with @Nikita in that we should always use the CSS variable in light and dark mode.
Comment 14 Zhifei Fang 2019-05-20 16:21:04 PDT
(In reply to Nikita Vasilyev from comment #10)
> (In reply to Zhifei Fang from comment #2)
> > Please also let me know how should I test my patch.
> > 
> > How to setup a local web inspector instance.
> > 
> > thanks!
> 
> You'll need to build WebKit locally.
> https://trac.webkit.org/wiki/HackingWebInspector
> Please let me know if this is helpful.

I tried this part:

For the Mac port, set the following defaults to allow inspecting the inspector.

defaults write com.apple.Safari WebKitDeveloperExtrasEnabled -bool YES
After updating these settings, run the ​Safari Technology Preview. Then, open the Web Inspector and right-click to inspect the inspector itself.


But I cannot open an inspector for an inspector...
Comment 15 Zhifei Fang 2019-05-20 16:22:09 PDT
(In reply to Devin Rousso from comment #13)
> Comment on attachment 370265 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=370265&action=review
> 
> r-
> 
> > Source/WebInspectorUI/UserInterface/Views/ErrorObjectView.css:57
> > +    .error-object-link-container {
> 
> This doesn't fix the issue for me, as there's a `.console-message
> .go-to-arrow` that has a higher specificity.  Please match the styling of
> `.call-frame .subtitle`, `.call-frame .separator`, and `.call-frame
> .subtitle .source-link` so we have the same coloring everywhere.
> 
> I also agree with @Nikita in that we should always use the CSS variable in
> light and dark mode.

Thanks, I am not 100% sure if my fix works, trying to make inspector can inspect another inspector to verify it.
Comment 16 Devin Rousso 2019-05-20 16:25:51 PDT
(In reply to Zhifei Fang from comment #14)
> (In reply to Nikita Vasilyev from comment #10)
> > (In reply to Zhifei Fang from comment #2)
> > > Please also let me know how should I test my patch.
> > > 
> > > How to setup a local web inspector instance.
> > > 
> > > thanks!
> > 
> > You'll need to build WebKit locally.
> > https://trac.webkit.org/wiki/HackingWebInspector
> > Please let me know if this is helpful.
> 
> I tried this part:
> 
> For the Mac port, set the following defaults to allow inspecting the inspector.
> 
> defaults write com.apple.Safari WebKitDeveloperExtrasEnabled -bool YES
> 
> After updating these settings, run the ​Safari Technology Preview. Then, open the Web Inspector and right-click to inspect the inspector itself.
> 
> But I cannot open an inspector for an inspector...

If you're trying to inspect Web Inspector (what we call inspector2 or inspector^2), you'll need to change the `com.apple.Safari` to be `com.apple.SafariTechnologyPreview`, but that won't have any changes you've made to your local checkout of WebKit.  You should follow the "Now Do Your Hacking" part of the wiki page <https://trac.webkit.org/wiki/HackingWebInspector#NowDoYourHacking>.
Comment 17 Zhifei Fang 2019-05-20 16:30:36 PDT
(In reply to Devin Rousso from comment #16)
> (In reply to Zhifei Fang from comment #14)
> > (In reply to Nikita Vasilyev from comment #10)
> > > (In reply to Zhifei Fang from comment #2)
> > > > Please also let me know how should I test my patch.
> > > > 
> > > > How to setup a local web inspector instance.
> > > > 
> > > > thanks!
> > > 
> > > You'll need to build WebKit locally.
> > > https://trac.webkit.org/wiki/HackingWebInspector
> > > Please let me know if this is helpful.
> > 
> > I tried this part:
> > 
> > For the Mac port, set the following defaults to allow inspecting the inspector.
> > 
> > defaults write com.apple.Safari WebKitDeveloperExtrasEnabled -bool YES
> > 
> > After updating these settings, run the ​Safari Technology Preview. Then, open the Web Inspector and right-click to inspect the inspector itself.
> > 
> > But I cannot open an inspector for an inspector...
> 
> If you're trying to inspect Web Inspector (what we call inspector2 or
> inspector^2), you'll need to change the `com.apple.Safari` to be
> `com.apple.SafariTechnologyPreview`, but that won't have any changes you've
> made to your local checkout of WebKit.  You should follow the "Now Do Your
> Hacking" part of the wiki page
> <https://trac.webkit.org/wiki/HackingWebInspector#NowDoYourHacking>.

Thanks, I did use com.apple.SafariTechnologyPreview

zfdemo:Scripts zhifei_fang$ defaults read com.apple.SafariTechnologyPreview
{
    WebKitDeveloperExtrasEnabled = 1;
}
Comment 18 Zhifei Fang 2019-05-22 11:31:07 PDT
Created attachment 370428 [details]
Patch
Comment 19 Devin Rousso 2019-06-19 11:15:58 PDT
Comment on attachment 370428 [details]
Patch

rs=me
Comment 20 WebKit Commit Bot 2019-06-19 11:45:28 PDT
Comment on attachment 370428 [details]
Patch

Clearing flags on attachment: 370428

Committed r246601: <https://trac.webkit.org/changeset/246601>
Comment 21 WebKit Commit Bot 2019-06-19 11:45:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Radar WebKit Bug Importer 2019-06-19 11:48:32 PDT
<rdar://problem/51906133>