Closed Bug 1391003 Opened 3 years ago Closed 2 years ago

Implement the new photon line selection and hover background colors in devtools

Categories

(DevTools :: General, enhancement, P3)

enhancement

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: gl, Assigned: gl)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

No description provided.
As a request, if you are changing the colors anyway, would it be possible to centralize the theme rules for codemirror into a single file (or at least one for variables, and one for cm- rules)? I recently wanted to create an add-on that uses the same syntax colors as Firefox and had to gather rules from 3-4 files.
Duplicate of this bug: 1339647
Part 1 - Line selection and hover behavior

The bright blue line background (Blue 55 on light, #204E8A on dark) is used for:
In the Inspector, selecting a row in the main view
In Debugger, for selecting a row in the source tree
The text in these rows should always be white

The hover blue line background (#F0F9FE on light, #353B48 on dark) is used for:
In Inspector, the row hover color
In debugger, the current line color (that one’s cursor is in). Debugger does not have a line hover color.
Text colors in these rows should remain the same as it usually is
Attachment #8903312 - Flags: review?(bgrinstead)
Comment on attachment 8903312 [details] [diff] [review]
Part 1: Implement the new photon line selection and hover background colors [1.0]

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

We discussed renaming the semitransparent variable to be more accurate in a part 1 of this bug
Attachment #8903312 - Flags: review?(bgrinstead)
Attachment #8903312 - Attachment is obsolete: true
Attachment #8903335 - Flags: review?(bgrinstead)
Attachment #8903335 - Attachment is obsolete: true
Attachment #8903335 - Flags: review?(bgrinstead)
Attachment #8903336 - Flags: review?(bgrinstead)
Attachment #8903336 - Attachment is obsolete: true
Attachment #8903336 - Flags: review?(bgrinstead)
Attachment #8903423 - Flags: review?(bgrinstead)
Comment on attachment 8903423 [details] [diff] [review]
Part 1: Implement the new photon line selection and hover background colors [4.0]

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

::: devtools/client/themes/animationinspector.css
@@ +377,5 @@
>  }
>  
>  .animation-timeline .animation.selected {
> +  background-color: var(--theme-selection-background-hover);
> +  z-index: -1;

I don't see a difference with or without the z-index applied here. Are you sure this rule is necessary?

::: devtools/client/themes/markup.css
@@ +176,5 @@
>  }
>  
> +.tag-hover:not(.theme-selected) {
> +  background: var(--theme-selection-background-hover);
> +  z-index: -1 !important;

I'd prefer to prefix the selector with .tag-line to be consistent with the selector above and avoid the !important rule. Like:

.tag-line .tag-hover:not(.theme-selected)
Attachment #8903423 - Flags: review?(bgrinstead) → review+
(In reply to Brian Grinstead [:bgrins] from comment #8)
> Comment on attachment 8903423 [details] [diff] [review]
> Part 1: Implement the new photon line selection and hover background colors
> [4.0]
> 
> Review of attachment 8903423 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/themes/animationinspector.css
> @@ +377,5 @@
> >  }
> >  
> >  .animation-timeline .animation.selected {
> > +  background-color: var(--theme-selection-background-hover);
> > +  z-index: -1;
> 
> I don't see a difference with or without the z-index applied here. Are you
> sure this rule is necessary?
> 

This one is very subtle, but basically the selected animation will overlap with the timeline lines.

> ::: devtools/client/themes/markup.css
> @@ +176,5 @@
> >  }
> >  
> > +.tag-hover:not(.theme-selected) {
> > +  background: var(--theme-selection-background-hover);
> > +  z-index: -1 !important;
> 
> I'd prefer to prefix the selector with .tag-line to be consistent with the
> selector above and avoid the !important rule. Like:
> 
> .tag-line .tag-hover:not(.theme-selected)

Fixed
Summary: Use the new photon colors for syntax highlighting in the developer tools → Implement the new photon line selection and hover background colors in devtools
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ed64fbf607c
Use the new photon line selection and hover background colors in devtool. r=bgrins
Flags: needinfo?(gl)
Attachment #8903596 - Attachment is obsolete: true
Attachment #8903677 - Flags: review+
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1299a669f1aa
Use the new photon line selection and hover background colors in devtool. r=bgrins
Backed out for failing devtools' browser_animation_target_highlight_select.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3cf93315452a7396b02a97a813a483b5d07a02ec

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1299a669f1aac95f3a8cf47ec21e4800e6deb5af&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=127806634&repo=mozilla-inbound

17:56:59     INFO -  92 INFO Select the simple animated node
17:56:59     INFO -  93 INFO Selecting the node for '.animated'
17:56:59     INFO -  94 INFO Retrieve the part of the widget that highlights the node on hover
17:56:59     INFO -  95 INFO Listen to node-highlight event and mouse over the widget
17:56:59     INFO -  Buffered messages logged at 17:55:16
17:56:59     INFO -  96 INFO Console message: [JavaScript Error: "1504288516262	Browser.Experiments.Experiments	ERROR	Experiments #0::httpGetRequest::onLoad() - Request to http://127.0.0.1:8888/experiments-dummy/manifest returned status 404" {file: "resource://gre/modules/Log.jsm" line: 752}]
17:56:59     INFO -  App_append@resource://gre/modules/Log.jsm:752:9
17:56:59     INFO -  log@resource://gre/modules/Log.jsm:390:7
17:56:59     INFO -  getLoggerWithMessagePrefix/proxy.log@resource://gre/modules/Log.jsm:505:44
17:56:59     INFO -  Experiments.Experiments/this._log.log@resource:///modules/experiments/Experiments.jsm:328:5
17:56:59     INFO -  error@resource://gre/modules/Log.jsm:398:5
17:56:59     INFO -  _httpGetRequest/</xhr.onload@resource:///modules/experiments/Experiments.jsm:973:11
17:56:59     INFO -  EventHandlerNonNull*_httpGetRequest/<@resource:///modules/experiments/Experiments.jsm:971:7
17:56:59     INFO -  _httpGetRequest@resource:///modules/experiments/Experiments.jsm:959:12
17:56:59     INFO -  _loadManifest@resource:///modules/experiments/Experiments.jsm:835:32
17:56:59     INFO -  async*_main@resource:///modules/experiments/Experiments.jsm:815:15
17:56:59     INFO -  async*_run/this._mainTask<@resource:///modules/experiments/Experiments.jsm:782:17
17:56:59     INFO -  async*_run@resource:///modules/experiments/Experiments.jsm:780:25
17:56:59     INFO -  updateManifest@resource:///modules/experiments/Experiments.jsm:868:12
17:56:59     INFO -  notify@jar:file:///Z:/task_1504287724/build/application/firefox/browser/omni.ja!/components/ExperimentsService.js:51:7
17:56:59     INFO -  TM_notify/<@jar:file:///Z:/task_1504287724/build/application/firefox/omni.ja!/components/nsUpdateTimerManager.js:217:11
17:56:59     INFO -  TM_notify@jar:file:///Z:/task_1504287724/build/application/firefox/omni.ja!/components/nsUpdateTimerManager.js:262:7
17:56:59     INFO -  97 INFO Console message: [JavaScript Error: "1504288516264	Browser.Experiments.Experiments	ERROR	Experiments #0::_loadManifest - failure to fetch/parse manifest (continuing anyway): Error: Experiments - XHR status for http://127.0.0.1:8888/experiments-dummy/manifest is 404" {file: "resource://gre/modules/Log.jsm" line: 752}]
17:56:59     INFO -  App_append@resource://gre/modules/Log.jsm:752:9
17:56:59     INFO -  log@resource://gre/modules/Log.jsm:390:7
17:56:59     INFO -  getLoggerWithMessagePrefix/proxy.log@resource://gre/modules/Log.jsm:505:44
17:56:59     INFO -  Experiments.Experiments/this._log.log@resource:///modules/experiments/Experiments.jsm:328:5
17:56:59     INFO -  error@resource://gre/modules/Log.jsm:398:5
17:56:59     INFO -  _loadManifest@resource:///modules/experiments/Experiments.jsm:845:7
17:56:59     INFO -  async*_main@resource:///modules/experiments/Experiments.jsm:815:15
17:56:59     INFO -  async*_run/this._mainTask<@resource:///modules/experiments/Experiments.jsm:782:17
17:56:59     INFO -  async*_run@resource:///modules/experiments/Experiments.jsm:780:25
17:56:59     INFO -  updateManifest@resource:///modules/experiments/Experiments.jsm:868:12
17:56:59     INFO -  notify@jar:file:///Z:/task_1504287724/build/application/firefox/browser/omni.ja!/components/ExperimentsService.js:51:7
17:56:59     INFO -  TM_notify/<@jar:file:///Z:/task_1504287724/build/application/firefox/omni.ja!/components/nsUpdateTimerManager.js:217:11
17:56:59     INFO -  TM_notify@jar:file:///Z:/task_1504287724/build/application/firefox/omni.ja!/components/nsUpdateTimerManager.js:262:7
17:56:59     INFO -  Buffered messages logged at 17:55:28
17:56:59     INFO -  98 INFO Longer timeout required, waiting longer...  Remaining timeouts: 1
17:56:59     INFO -  Buffered messages finished
17:56:59    ERROR -  99 INFO TEST-UNEXPECTED-FAIL | devtools/client/animationinspector/test/browser_animation_target_highlight_select.js | Test timed out -
Flags: needinfo?(gl)
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1030993d93e2
Use the new photon line selection and hover background colors in devtool. r=bgrins
Backed out for devtools failures like https://treeherder.mozilla.org/logviewer.html#?job_id=127855438&repo=mozilla-inbound

These failures happened with the previous landing as well.

Please do a try run with the full set of devtools tests run on at least linux before relanding.

https://hg.mozilla.org/integration/mozilla-inbound/rev/414abbf7e647780be988db0e84a3a547fd245dbf
Flags: needinfo?(gl)
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/95089cbed416
Use the new photon line selection and hover background colors in devtool. r=bgrins
https://hg.mozilla.org/mozilla-central/rev/95089cbed416
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.