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
Created attachment 370242 [details] Patch
Please also let me know how should I test my patch. How to setup a local web inspector instance. thanks!
(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.
Created attachment 370243 [details] [HTML] Reduction
Created attachment 370244 [details] [Image] Screenshot of reduction The link is appropriate color. The em dash is black and it shouldn't be!
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.
it is also weird that if you first open the file, and then open inspector, it doesn't give me that link...
Created attachment 370245 [details] No black link
(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.
(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 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.
Created attachment 370265 [details] Patch
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.
(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...
(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.
(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>.
(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; }
Created attachment 370428 [details] Patch
Comment on attachment 370428 [details] Patch rs=me
Comment on attachment 370428 [details] Patch Clearing flags on attachment: 370428 Committed r246601: <https://trac.webkit.org/changeset/246601>
All reviewed patches have been landed. Closing bug.
<rdar://problem/51906133>