Added missing splitters.css...

Comment on attachment 8703660 [details] [diff] [review] patch v2 Review of attachment 8703660 [details] [diff] [review]: ----------------------------------------------------------------- Looks like a nice refactor, thanks! ::: browser/base/content/browser.xul @@ +7,5 @@ > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > <?xml-stylesheet href="chrome://browser/content/browser.css" type="text/css"?> > <?xml-stylesheet href="chrome://browser/content/places/places.css" type="text/css"?> > +<?xml-stylesheet href="chrome://devtools/skin/browser.css" type="text/css"?> What do you think about renaming this to something like devtools-browser.css? Concerned we might be in the way of the rest of the frontend team by reusing the browser.css name. ::: devtools/client/themes/browser.css @@ +1,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +@import url("splitters.css"); Hopefully this won't cause any talos regressions due to loading another CSS file. If so I guess we could have some code that only loads these styles if/when devtools is opened.

https://hg.mozilla.org/integration/fx-team/rev/bac08b0a467d8f4843a0bf147e3693a287832436 Bug 1235781 - Remove preprocessing from common.css. r=bgrins

Backout: https://hg.mozilla.org/integration/fx-team/rev/2ec18a7b7349

Backed out because it broke eyedropper test in dt tests: https://treeherder.mozilla.org/logviewer.html#?job_id=6514176&repo=fx-team 08:31:12 INFO - 70 INFO TEST-PASS | devtools/client/inspector/rules/test/browser_rules_eyedropper.js | swatch didn't change after pressing ESC - 08:31:12 INFO - 71 INFO Console message: [JavaScript Warning: "Property contained reference to invalid variable. Error in parsing value for 'font-family'. Falling back to 'inherit'." {file: "chrome://devtools/skin/common.css" line: 24 column: 505 source: " var(--monospace-font-family)"}] 08:31:12 INFO - 72 INFO TEST-PASS | devtools/client/inspector/rules/test/browser_rules_eyedropper.js | dropper opened - 08:31:12 INFO - 73 INFO Console message: [JavaScript Warning: "Property contained reference to invalid variable. Error in parsing value for 'font-family'. Falling back to 'inherit'." {file: "chrome://devtools/skin/common.css" line: 24 column: 505 source: " var(--monospace-font-family)"}] 08:31:12 INFO - 74 INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_eyedropper.js | swatch changed colors - Got rgb(228, 239, 250), expected rgb(255, 255, 85) 08:31:12 INFO - Stack trace: 08:31:12 INFO - chrome://mochikit/content/browser-test.js:test_is:979 08:31:12 INFO - chrome://mochitests/content/browser/devtools/client/inspector/rules/test/browser_rules_eyedropper.js:testSelect:104 08:31:12 INFO - Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:933:23 08:31:12 INFO - this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:812:7 08:31:12 INFO - this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:746:11 08:31:12 INFO - promise callback*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:9 08:31:12 INFO - this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:775:7 08:31:12 INFO - this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:710:7 08:31:12 INFO - openEyedropper/</<@chrome://mochitests/content/browser/devtools/client/inspector/rules/test/browser_rules_eyedropper.js:142:7 08:31:12 INFO - EventEmitter_once/handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:92:9 08:31:12 INFO - EventEmitter_emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:147:11 08:31:12 INFO - SwatchColorPickerTooltip.prototype<._openEyeDropper@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/Tooltip.js:1217:5 08:31:12 INFO - openEyedropper/<@chrome://mochitests/content/browser/devtools/client/inspector/rules/test/browser_rules_eyedropper.js:144:5 08:31:12 INFO - EventEmitter_once/handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:92:9 08:31:12 INFO - EventEmitter_emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:147:11 08:31:12 INFO - @resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/Tooltip.js:199:11 08:31:12 INFO - EventListener.handleEvent*Tooltip@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/Tooltip.js:203:5 08:31:12 INFO - SwatchBasedEditorTooltip@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/Tooltip.js:957:18 08:31:12 INFO - SwatchColorPickerTooltip@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/Tooltip.js:1128:3 08:31:12 INFO - TooltipsOverlay.prototype.addToView@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/styleinspector/style-inspector-overlays.js:281:26 08:31:12 INFO - CssRuleView@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/rules/rules.js:570:3 08:31:12 INFO - RuleViewTool@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/styleinspector/style-inspector.js:31:15 08:31:12 INFO - window.setPanel@chrome://devtools/content/inspector/rules/rules.xhtml:27:25 08:31:12 INFO - ToolSidebar.prototype.addTab/onIFrameLoaded@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/sidebar.js:244:9 08:31:12 INFO - EventListener.handleEvent*ToolSidebar.prototype.addTab@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/sidebar.js:249:5 08:31:12 INFO - InspectorPanel.prototype.setupSidebar@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:355:5 08:31:12 INFO - InspectorPanel.prototype._deferredOpen@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:212:5 08:31:12 INFO - InspectorPanel.prototype.open/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:109:14 08:31:12 INFO - Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:933:23 08:31:12 INFO - this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:812:7 08:31:12 INFO - Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:743:11 08:31:12 INFO - this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:775:7 08:31:12 INFO - this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:710:7 08:31:12 INFO - Front<.onPacket/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1236:9 08:31:12 INFO - DevTools RDP*Front<.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1178:7 08:31:12 INFO - frontProto/</proto[name]@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1338:14 08:31:12 INFO - exports.WalkerFront<.querySelector<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/inspector.js:3359:12 08:31:12 INFO - InspectorPanel.prototype._getDefaultNodeForSelection/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:265:14 08:31:12 INFO - Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:933:23 08:31:12 INFO - this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:812:7 08:31:12 INFO - Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:743:11 08:31:12 INFO - this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:775:7 08:31:12 INFO - this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:710:7 08:31:12 INFO - Front<.onPacket/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1236:9 08:31:12 INFO - DevTools RDP*Front<.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1178:7 08:31:12 INFO - frontProto/</proto[name]@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1338:14 08:31:12 INFO - exports.InspectorFront<.getPageStyle<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/inspector.js:3932:12 08:31:12 INFO - InspectorPanel.prototype._getPageStyle@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:226:12 08:31:12 INFO - InspectorPanel.prototype.open/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:105:14 08:31:12 INFO - Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:933:23 08:31:12 INFO - this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:812:7 08:31:12 INFO - Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:743:11 08:31:12 INFO - this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:775:7 08:31:12 INFO - Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:450:5 08:31:12 INFO - InspectorPanel.prototype.open@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:104:12 08:31:12 INFO - Toolbox.prototype.loadTool/onLoad@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox.js:1247:19 08:31:12 INFO - 75 INFO TEST-PASS | devtools/client/inspector/rules/test/browser_rules_eyedropper.js | div's color set to body color after dropper - 08:31:12 INFO - 76 INFO TEST-PASS | devtools/client/inspector/rules/test/browser_rules_eyedropper.js | eyedropper telemetry value correct for DEVTOOLS_PICKER_EYEDROPPER_OPENED_BOOLEAN - 08:31:12 INFO - 77 INFO TEST-PASS | devtools/client/inspector/rules/test/browser_rules_eyedropper.js | eyedropper telemetry value correct for DEVTOOLS_PICKER_EYEDROPPER_OPENED_PER_USER_FLAG - 08:31:12 INFO - 78 INFO Leaving test

I need these additional tweaks to fix the eyedropper... That's because we also pull common.css from this xul document but we don't use theme-switching in it. So that platform attribute isn't set. Also, .devtools-eyedropper-panel has to be in browser.xul scope as we insert the xul:panel there and not in toolbox documents. With that, I get a greener try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=97e282fdb5ea&selectedJob=15293383

I see a white 'border' around the eyedropper on osx with these patches applied

Comment on attachment 8706630 [details] eye-dropper-color.png Nevermind, looks like ./mach build faster wasn't working.. With the updated patch it looks as expected

Comment on attachment 8706604 [details] [diff] [review] Fix per platform css and browser css rule for eyedropper. Review of attachment 8706604 [details] [diff] [review]: ----------------------------------------------------------------- This works for me. I'd prefer to not have copy/pasted code for the 'platform' bit though ::: devtools/client/eyedropper/eyedropper.js @@ +369,5 @@ > else if (this.format == "hsl") { > valueBox.style.width = HSL_BOX_WIDTH + "px"; > } > > + let os; What about loading theme-switching.js into the eyedropper.xul panel? Then we wouldn't need to copy/paste this logic. To save from unnecessary css loading we could add a new attribute on the root element like "no-theme" that'd be similar to "force-theme" but prevented theme-switching from actually loading a file (instead it'd just set the platform attribute). I prefer that theme-switching is loaded in every frame for exactly this reason. Alternatively what about using shared/system -> getSystemInfo().os (or adding a new constant on that object that always returns "win"/"mac"/"linux") and using it in both places? ::: devtools/client/eyedropper/eyedropper.xul @@ +37,5 @@ > <label id="color-value" class="devtools-monospace"> > </label> > </hbox> > </hbox> > +</window> What's this change?

(In reply to Brian Grinstead [:bgrins] from comment #16) > ::: devtools/client/eyedropper/eyedropper.js > @@ +369,5 @@ > > else if (this.format == "hsl") { > > valueBox.style.width = HSL_BOX_WIDTH + "px"; > > } > > > > + let os; > > What about loading theme-switching.js into the eyedropper.xul panel? I did that with the "no-theme" attribute. > ::: devtools/client/eyedropper/eyedropper.xul > @@ +37,5 @@ > > <label id="color-value" class="devtools-monospace"> > > </label> > > </hbox> > > </hbox> > > +</window> > > What's this change? EOF space thing git does automagically. I removed it.

Comment on attachment 8707009 [details] [diff] [review] Fix per platform css and browser css rule for eyedropper - v2 Review of attachment 8707009 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

https://hg.mozilla.org/integration/fx-team/rev/5292f5305f46e69515d498c7221b5b6a42e2b601 Bug 1235781 - Remove preprocessing from common.css. r=bgrins https://hg.mozilla.org/integration/fx-team/rev/8cd5149485cb02d37d6dfac259af4eda6fa02f9e Bug 1235781 - Fix per platform css and browser css rule for eyedropper. r=bgrins

Had to rebase this one.

https://hg.mozilla.org/mozilla-central/rev/5292f5305f46 https://hg.mozilla.org/mozilla-central/rev/8cd5149485cb

[bugday-20160323] Status: RESOLVED,FIXED -> UNVERIFIED Comments: STR: Not clear. Developer specific testing Component: Name Firefox Version 46.0b9 Build ID 20160322075646 Update Channel beta User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0 OS Windows 7 SP1 x86_64 Expected Results: Developer specific testing Actual Results: As expected

[bugday-20160323] Status: RESOLVED,FIXED -> UNVERIFIED Comments: STR: Not clear. Developer specific testing Component: Name Firefox Version 46.0b9 Build ID 20160322075646 Update Channel beta User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0 OS Windows 7 SP1 x86_64 Expected Results: Developer specific testing Actual Results: As expected