Bug 198033

Summary:

ErrorObjectView display black link in dark mode

Product:

WebKit

Reporter:

Zhifei Fang <zhifei_fang>

Component:

Web Inspector

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, hi, inspector-bugzilla-changes, nvasilyev, webkit-bug-importer

Priority:

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

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.