DOM panel support for Firebug Theme

RESOLVED FIXED in Firefox 48

Status

P1
normal
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: Honza, Assigned: Honza)

Tracking

unspecified
Firefox 48
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
The DOM panel should also support Firebug theme.

Honza
(Assignee)

Updated

2 years ago
Blocks: 1263889
Depends on: 1201475
(Assignee)

Updated

2 years ago
Assignee: nobody → odvarko
(Assignee)

Updated

2 years ago
Priority: -- → P1
(Assignee)

Updated

2 years ago
Depends on: 1244054
Summary: DOM pane support for Firebug Theme → DOM panel support for Firebug Theme
(Assignee)

Comment 1

2 years ago
Created attachment 8740987 [details] [diff] [review]
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
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)

Comment 4

2 years ago
Will take a look tomorrow or Saturday.

Comment 5

2 years ago
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+
(Assignee)

Comment 6

2 years ago
Created attachment 8742421 [details] [diff] [review]
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
Attachment #8740987 - Attachment is obsolete: true
Attachment #8742421 - Flags: review?(bgrinstead)

Comment 7

2 years ago
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+
(Assignee)

Comment 8

2 years ago
Created attachment 8742645 [details] [diff] [review]
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/controls@2x.png");
> > +}
> 
> nit: indent this block properly
Fixed

Thanks Tim!

Honza
Attachment #8742421 - Attachment is obsolete: true
Attachment #8742645 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
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
(Assignee)

Comment 11

2 years ago
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)

Comment 12

2 years ago
(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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c1a9497a212d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Component: Developer Tools → Developer Tools: DOM

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.