The DOM panel should also support Firebug theme. Honza

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 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 on attachment 8740987 [details] [diff] [review] bug1264312.patch Actually reassigning (see comment 2)

Will take a look tomorrow or Saturday.

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.

(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 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/controls@2x.png"); > +} nit: indent this block properly

(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/controls@2x.png"); > > +} > > nit: indent this block properly Fixed Thanks Tim! Honza

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

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

(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)