1494789 - Improve color for strings (inside errors) in Console

Status: VERIFIED FIXED

(DevTools :: Console, enhancement, P3)

Description

[Victoria Wang [:victoria]]

As discussed and designed in: https://github.com/devtools-html/ux/issues/19
It's now ready for implementation.

Comment 1

[Victoria Wang [:victoria]]

(Assigned to Kristen who did the UX)

Comment 2

[Kristin Taylor [:kristin]]

Thanks for claiming for me!

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Kristin, are you waiting on something or blocked by an issue (or simply don't have the time yet) ?
No rush at all, I just want to make sure we are not blocking you in any way 🙂

Comment 4

[Kristin Taylor [:kristin]]

Nicholas, Thanks for checking up on me. I just got local firefox setup and am working on this now. I should have a patch up for review in a few days, if not sooner.

Comment 5

[Kristin Taylor [:kristin]]

Attachment: 1494789.patch

Would you mind reviewing, Nicolas? Thanks!

Comment 6

[Kristin Taylor [:kristin]]

Attachment: Light Theme Version

Comment 7

[Kristin Taylor [:kristin]]

Attachment: Dark Theme Version

Here are screen shots with this patch.

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9017760 [details] [diff] [review]
1494789.patch

Review of attachment 9017760 [details] [diff] [review]:
-----------------------------------------------------------------

Hello Kristin, thanks a lot for working on this !

The --theme-highlight-red variable is used in many place (https://searchfox.org/mozilla-central/search?q=--theme-highlight-red&case=false&regexp=false&path=), so here we would probably cause unwanted UI changes.

Could we instead create a new variable (maybe `--theme-error`) that we could then use in webconsole.css ?
Thanks !

Comment 9

[Kristin Taylor [:kristin]]

Oh! Good to know. I'll make that change and upload a new patch. 
Kristin

Comment 10

[Kristin Taylor [:kristin]]

Nicolas, 
Looking at webconsole.css on line 46 --error-color is set to --red-70 (which is the color we are using) for light mode. Also in webconsole.css on line 29 --error-color is defined with a HSL color. 
https://searchfox.org/mozilla-central/source/devtools/client/themes/webconsole.css

Would it be ok to use that already defined variable for my changes to keep the code DRY? To be consistent and readable, going this route I would change the dark mode color for --error-color to --red-20. 

Searchfox indicates that --error-color is not widely used. Would changing the defined --error-color in dark mode and calling it for my changes be ok? 

https://searchfox.org/mozilla-central/search?q=--error-color&path=

From what I understand, I also have to change line 9 in reps.css to reflect the new variable name. So, having the same name for the variable in dark and light mode is necessary.

https://searchfox.org/mozilla-central/source/devtools/client/shared/components/reps/reps.css

Otherwise I can create a new variable (--warning-color) as you suggested, and go from there.

Any suggestions as this is the first time I have worked on a library this large.

Thanks!

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

> Searchfox indicates that --error-color is not widely used. Would changing the defined --error-color in dark mode and calling it for my changes be ok? 

Yes, it looks safe, let's do that :)

For reps.css, it's copied over from Github (https://github.com/devtools-html/debugger.html/blob/master/packages/devtools-reps/src/reps/reps.css). So you can do the change in this repo, but we should also make it in Github so the change does not get overridden when the debugger team does the next release :)

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

(also, side-note, in Bugzilla, you can use the "Need more information from" input at the bottom of the textarea. This is the equivalent of @, and will put a reminder on people's bugzilla dashboard that they need to answer to someone. This is pretty useful because we usually get a lot of bugmails and might miss a comment requiring an answer :) )

Comment 13

[Kristin Taylor [:kristin]]

Thanks for the Bugzilla tip! 
I'll get the new patch submitted early next week.

Comment 14

[Kristin Taylor [:kristin]]

Attachment: 1494789-1.patch

Comment 15

[Kristin Taylor [:kristin]]

Attachment: 1494789-2.patch

Comment 16

[Kristin Taylor [:kristin]]

Attachment: 1494789-3.patch

Comment 17

[Kristin Taylor [:kristin]]

I'll make a pull request against the debugger.html repo once we're happy with this.

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9019229 [details] [diff] [review]
1494789-2.patch

Review of attachment 9019229 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks a lot Kristin, this looks good to me!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7556fb1b0ca
Part 1: Add --red-20 to variables.css. r=nchevobbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/9be54dc6759d
Part 2: Change --error-color to --red-20. r=nchevobbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/5566199eda70
Part 3: Use --error-color for --string-color. r=nchevobbe

Comment 20

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/d7556fb1b0ca
https://hg.mozilla.org/mozilla-central/rev/9be54dc6759d
https://hg.mozilla.org/mozilla-central/rev/5566199eda70

Comment 21

[Nicolas Chevobbe [:nchevobbe]]

This was reverted by Bug 1502224.
Let's re-use this bug to only apply the color change to `.message.error` (definitely my fault to not have checked that!)

Comment 22

[Kristin Taylor [:kristin]]

(In reply to Nicolas Chevobbe from comment #21)
> This was reverted by Bug 1502224.
> Let's re-use this bug to only apply the color change to `.message.error`
> (definitely my fault to not have checked that!)

Thanks Nicolas! Working an updated patch now.

Comment 23

[Kristin Taylor [:kristin]]

Attachment: new.patch

Is this the right approach? I've tested this locally and it looks good to me.

Comment 24

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9022023 [details] [diff] [review]
new.patch

Hello Kristin, thanks for taking care of this :)

This is the right approach indeed, but it would be better to put these rules in webconsole.css instead of reps.css, since those rules should only impact the console (reps are used in several places)

Comment 25

[Kristin Taylor [:kristin]]

Attachment: 1494789-3.patch

I moved the rules to webconsole.css. Can you take a look?

Comment 26

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9022821 [details] [diff] [review]
1494789-3.patch

Review of attachment 9022821 [details] [diff] [review]:
-----------------------------------------------------------------

I tested it and it looks perfect, thanks a lot Kristin!

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9bec2561346
Use --error-color for warning and error objectBox-string. r=nchevobbe

Comment 28

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/c9bec2561346

Comment 29

[Cristian Fogel, QA [:cfogel]]

Verified the issue on 65.0b12 on Windows 10, macOS 10.13, Ubuntu 18.04.
Can confirm that the --red-70 color scheme is used.