Closed Bug 1264312 Opened 4 years ago Closed 4 years ago

DOM panel support for Firebug Theme

Categories

(DevTools :: DOM, defect, P1)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: Honza, Assigned: Honza)

References

Details

Attachments

(1 file, 2 obsolete files)

The DOM panel should also support Firebug theme.

Honza
Assignee: nobody → odvarko
Priority: -- → P1
Depends on: 1244054
Summary: DOM pane support for Firebug Theme → DOM panel support for Firebug Theme
Attached patch bug1264312.patch (obsolete) — Splinter Review
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
Attachment #8740987 - Flags: review?(bgrinstead)
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)
Attachment #8740987 - Flags: review?(bgrinstead) → review?(ntim.bugs)
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.
Attachment #8740987 - Flags: review?(ntim.bugs) → feedback+
Attached patch bug1264312.patch (obsolete) — Splinter Review
(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
Attachment #8740987 - Attachment is obsolete: true
Attachment #8742421 - Flags: review?(bgrinstead)
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
Attachment #8742421 - Flags: review?(bgrinstead) → review+
Attached patch bug1264312.patchSplinter Review
(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
Attachment #8742421 - Attachment is obsolete: true
Attachment #8742645 - Flags: review+
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
Flags: needinfo?(odvarko)
Keywords: checkin-needed
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
Flags: needinfo?(odvarko)
(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)
https://hg.mozilla.org/mozilla-central/rev/c1a9497a212d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Component: Developer Tools → Developer Tools: DOM
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.