198033 – ErrorObjectView display black link in dark mode

RESOLVED FIXED 198033

ErrorObjectView display black link in dark mode

https://bugs.webkit.org/show_bug.cgi?id=198033

Summary ErrorObjectView display black link in dark mode

Zhifei Fang

Reported 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

Attachments
link in black (15.03 KB, image/jpeg) 2019-05-20 00:51 PDT, Zhifei Fang	no flags	Details
Patch (1.29 KB, patch) 2019-05-20 01:03 PDT, Zhifei Fang	no flags	Details Formatted Diff Diff
[HTML] Reduction (70 bytes, text/html) 2019-05-20 01:52 PDT, Nikita Vasilyev	no flags	Details
[Image] Screenshot of reduction (22.93 KB, image/png) 2019-05-20 01:54 PDT, Nikita Vasilyev	no flags	Details
No black link (30.08 KB, image/jpeg) 2019-05-20 02:03 PDT, Zhifei Fang	no flags	Details
Patch (1.30 KB, patch) 2019-05-20 11:16 PDT, Zhifei Fang	no flags	Details Formatted Diff Diff
Patch (2.00 KB, patch) 2019-05-22 11:31 PDT, Zhifei Fang	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Zhifei Fang

Comment 1 2019-05-20 01:03:12 PDT

Created attachment 370242 [details] Patch

Zhifei Fang

Comment 2 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!

Nikita Vasilyev

Comment 3 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.

Nikita Vasilyev

Comment 4 2019-05-20 01:52:22 PDT

Created attachment 370243 [details] [HTML] Reduction

Nikita Vasilyev

Comment 5 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!

Zhifei Fang

Comment 6 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.

Zhifei Fang

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

Zhifei Fang

Comment 8 2019-05-20 02:03:26 PDT

Created attachment 370245 [details] No black link

Nikita Vasilyev

Comment 9 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.

Nikita Vasilyev

Comment 10 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.

Nikita Vasilyev

Comment 11 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.

Zhifei Fang

Comment 12 2019-05-20 11:16:51 PDT

Created attachment 370265 [details] Patch

Devin Rousso

Comment 13 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.

Zhifei Fang

Comment 14 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...

Zhifei Fang

Comment 15 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.

Devin Rousso

Comment 16 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>.

Zhifei Fang

Comment 17 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; }

Zhifei Fang

Comment 18 2019-05-22 11:31:07 PDT

Created attachment 370428 [details] Patch

Devin Rousso

Comment 19 2019-06-19 11:15:58 PDT

Comment on attachment 370428 [details] Patch rs=me

WebKit Commit Bot

Comment 20 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>

WebKit Commit Bot

Comment 21 2019-06-19 11:45:30 PDT

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 22 2019-06-19 11:48:32 PDT

<rdar://problem/51906133>

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component Web Inspector

Assignee

Nobody

Reported

2019-05-20 00:51 PDT

Modified

2019-06-19 11:48 PDT History

CC List

5 users Show

URL

Keywords InRadar

Depends on

Blocks