1264312 - DOM panel support for Firebug Theme

Status: RESOLVED FIXED

(DevTools :: DOM, defect, P1)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

The DOM panel should also support Firebug theme.

Honza

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1264312.patch

This depends on the Firebug theme (bug 1244054) as well as the DOM panel (bug 1201475), but as soon as those land we should try to land this one too.

Honza

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8740987 [details] [diff] [review]
bug1264312.patch

Re-assigning to :ntim who's been doing a lot of the firebug theme reviews.  Note that you need patches from Bugs 1201475 and 1244054 applied.  Tim, let me know if you don't have time to look at this, or just send it back over.

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8740987 [details] [diff] [review]
bug1264312.patch

Actually reassigning (see comment 2)

Comment 4

[Tim Nguyen :ntim]

Will take a look tomorrow or Saturday.

Comment 5

[Tim Nguyen :ntim]

Comment on attachment 8740987 [details] [diff] [review]
bug1264312.patch

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

The code changes look good to me are r+ to me, but I feel like the default theme colors are not appropriately chosen.

::: devtools/client/shared/components/reps/reps.css
@@ +9,5 @@
> +  --string-color: var(--theme-highlight-orange);
> +  --null-color: var(--theme-comment);
> +  --object-color: var(--theme-highlight-blue);
> +  --caption-color: var(--theme-highlight-blue);
> +  --location-color: #555555;

This color will probably have bad contrast with the dark theme. Can you change it?

@@ +10,5 @@
> +  --null-color: var(--theme-comment);
> +  --object-color: var(--theme-highlight-blue);
> +  --caption-color: var(--theme-highlight-blue);
> +  --location-color: #555555;
> +  --source-link-color: blue;

Can you use var(--theme-highlight-blue) ? It has better contrast on the dark theme than blue.

@@ +11,5 @@
> +  --object-color: var(--theme-highlight-blue);
> +  --caption-color: var(--theme-highlight-blue);
> +  --location-color: #555555;
> +  --source-link-color: blue;
> +  --node-color: rgb(0, 0, 136);

This color has very low contrast on the dark theme, can you change it ? --theme-highlight-bluegrey seems like a good candidate

@@ +12,5 @@
> +  --caption-color: var(--theme-highlight-blue);
> +  --location-color: #555555;
> +  --source-link-color: blue;
> +  --node-color: rgb(0, 0, 136);
> +  --reference-color: rgb(102, 102, 255);

This is close to var(--theme-highlight-purple). Can you use that instead.

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1264312.patch

(In reply to Tim Nguyen [:ntim] from comment #5)
> Comment on attachment 8740987 [details] [diff] [review]
> bug1264312.patch
> 
> Review of attachment 8740987 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The code changes look good to me are r+ to me, but I feel like the default
> theme colors are not appropriately chosen.
> 
> ::: devtools/client/shared/components/reps/reps.css
> @@ +9,5 @@
> > +  --string-color: var(--theme-highlight-orange);
> > +  --null-color: var(--theme-comment);
> > +  --object-color: var(--theme-highlight-blue);
> > +  --caption-color: var(--theme-highlight-blue);
> > +  --location-color: #555555;
> 
> This color will probably have bad contrast with the dark theme. Can you
> change it?
I used var(--theme-content-color1), bug feel free to suggest different.

> 
> @@ +10,5 @@
> > +  --null-color: var(--theme-comment);
> > +  --object-color: var(--theme-highlight-blue);
> > +  --caption-color: var(--theme-highlight-blue);
> > +  --location-color: #555555;
> > +  --source-link-color: blue;
> 
> Can you use var(--theme-highlight-blue) ? It has better contrast on the dark
> theme than blue.
Done

> 
> @@ +11,5 @@
> > +  --object-color: var(--theme-highlight-blue);
> > +  --caption-color: var(--theme-highlight-blue);
> > +  --location-color: #555555;
> > +  --source-link-color: blue;
> > +  --node-color: rgb(0, 0, 136);
> 
> This color has very low contrast on the dark theme, can you change it ?
> --theme-highlight-bluegrey seems like a good candidate
Done

> 
> @@ +12,5 @@
> > +  --caption-color: var(--theme-highlight-blue);
> > +  --location-color: #555555;
> > +  --source-link-color: blue;
> > +  --node-color: rgb(0, 0, 136);
> > +  --reference-color: rgb(102, 102, 255);
> 
> This is close to var(--theme-highlight-purple). Can you use that instead.
Done

Thanks!

Honza

Comment 7

[Tim Nguyen :ntim]

Comment on attachment 8742421 [details] [diff] [review]
bug1264312.patch

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

Looks good to me ! Stealing review from bgrins :)

::: devtools/client/shared/components/tree/tree-view.css
@@ +162,5 @@
> +@media (min-resolution: 1.1dppx) {
> +.theme-dark .treeTable .treeRow.hasChildren > .treeLabelCell > .treeIcon,
> +.theme-light .treeTable .treeRow.hasChildren > .treeLabelCell > .treeIcon {
> +  background-image: url("chrome://devtools/skin/images/[e-mail]");
> +}

nit: indent this block properly

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1264312.patch

(In reply to Tim Nguyen [:ntim] (busy, email me instead) from comment #7)
> Comment on attachment 8742421 [details] [diff] [review]
> bug1264312.patch
> 
> Review of attachment 8742421 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me ! Stealing review from bgrins :)
> 
> ::: devtools/client/shared/components/tree/tree-view.css
> @@ +162,5 @@
> > +@media (min-resolution: 1.1dppx) {
> > +.theme-dark .treeTable .treeRow.hasChildren > .treeLabelCell > .treeIcon,
> > +.theme-light .treeTable .treeRow.hasChildren > .treeLabelCell > .treeIcon {
> > +  background-image: url("chrome://devtools/skin/images/[e-mail]");
> > +}
> 
> nit: indent this block properly
Fixed

Thanks Tim!

Honza

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c1a9497a212d

Comment 10

[Carsten Book [:Tomcat]]

seems this has conflicts:

patching file devtools/client/shared/components/reps/reps.css
Hunk #1 FAILED at 0
Hunk #2 FAILED at 127
2 out of 2 hunks FAILED -- saving rejects to file devtools/client/shared/components/reps/reps.css.rej
patching file devtools/client/shared/components/tree/tree-view.css
Hunk #1 FAILED at 40
Hunk #2 FAILED at 76
Hunk #3 FAILED at 143
3 out of 3 hunks FAILED -- saving rejects to file devtools/client/shared/components/tree/tree-view.css.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug1264312.patch

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Yep, it depends on bug 1201475 that has been backed out so, we need to wait till it lands again. I am looking into it.

Honza

Comment 12

[Tim Nguyen :ntim]

(In reply to Jan Honza Odvarko [:Honza] from comment #11)
> Yep, it depends on bug 1201475 that has been backed out so, we need to wait
> till it lands again. I am looking into it.
> 
> Honza

This is because I've already landed the patch (see comment 9)

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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