Closed
Bug 1391003
Opened 7 years ago
Closed 7 years ago
Implement the new photon line selection and hover background colors in devtools
Categories
(DevTools :: General, enhancement, P3)
DevTools
General
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)
14.77 KB,
patch
|
gl
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Comment 1•7 years ago
|
||
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.
Assignee | ||
Comment 3•7 years ago
|
||
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 4•7 years ago
|
||
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)
Assignee | ||
Comment 5•7 years ago
|
||
Attachment #8903312 -
Attachment is obsolete: true
Attachment #8903335 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 6•7 years ago
|
||
Attachment #8903335 -
Attachment is obsolete: true
Attachment #8903335 -
Flags: review?(bgrinstead)
Attachment #8903336 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 7•7 years ago
|
||
Attachment #8903336 -
Attachment is obsolete: true
Attachment #8903336 -
Flags: review?(bgrinstead)
Attachment #8903423 -
Flags: review?(bgrinstead)
Comment 8•7 years ago
|
||
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+
Assignee | ||
Comment 9•7 years ago
|
||
(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
Assignee | ||
Comment 10•7 years ago
|
||
Attachment #8903423 -
Attachment is obsolete: true
Attachment #8903596 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
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
Assignee | ||
Comment 11•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe5219561adfd346421e8b94e974820b26ab1d3b
Comment 12•7 years ago
|
||
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
Backed out for dt failures like https://treeherder.mozilla.org/logviewer.html#?job_id=127756110&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/859f77f6e57f
Flags: needinfo?(gl)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(gl)
Assignee | ||
Comment 14•7 years ago
|
||
Attachment #8903596 -
Attachment is obsolete: true
Attachment #8903677 -
Flags: review+
Comment 15•7 years ago
|
||
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
Comment 16•7 years ago
|
||
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)
Comment 17•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(gl)
Comment 19•7 years ago
|
||
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
Comment 20•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/95089cbed416
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 21•7 years ago
|
||
Screenshots: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=39e9a6b187a3f71f46d02af000ff8203abe4fe71&newProject=mozilla-central&newRev=f5d99e9f3d7bf19c8a222db97bedc924da27fd58&filter=devtools
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•