Closed
Bug 1136257
Opened 6 years ago
Closed 6 years ago
Switch between color unit format in place
Categories
(DevTools :: Inspector, defect, P3)
DevTools
Inspector
Tracking
(firefox40 fixed)
RESOLVED
FIXED
Firefox 40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: andrewskad, Assigned: miker)
References
Details
(Whiteboard: [polish-backlog][difficulty=easy])
Attachments
(1 file, 7 obsolete files)
23.18 KB,
patch
|
miker
:
review+
|
Details | Diff | Splinter Review |
It would be a nice thing to add a way to switch between color unit formats (hex, rgb, hls and color name) in place, meaning with a shortcut I should be able to click the color and change it's format. I'm not talking about choosing my preferred color format in the settings tab. This is a feature presented in chrom* dev tools, they choose shift+click as the shortcut.
Reporter | ||
Updated•6 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Comment 1•6 years ago
|
||
Choosing shift+click too would be wise, to avoid adding a new keybinding users would have to learn. Mike, you know about color handling in devtools, do you mind commenting a bit on this bug so someone can easily pick it up?
Flags: needinfo?(mratcliffe)
Updated•6 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 2•6 years ago
|
||
Quite a few things to change in a few places. This is one of those things that is cool and reasonably simple to implement although difficult to explain so I will do it... in fact, I have already done 75% of it.
Assignee: nobody → mratcliffe
Flags: needinfo?(mratcliffe)
Assignee | ||
Comment 3•6 years ago
|
||
As I thought, we had to tweak things all over the place to make this work (tooltip.js, computed view, rule view and css-color). Works perfectly even when there are multiple colors e.g. box shadow space invader. I wish it was more discoverable but the fact that Chrome does it using the same key combination should be enough. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0401a4adde66
Attachment #8572058 -
Flags: review?(bgrinstead)
Reporter | ||
Comment 4•6 years ago
|
||
Thank you, Michael. Is that try for me though? I don't know what's that, I'm kinda new to this.
Comment 5•6 years ago
|
||
Comment on attachment 8572058 [details] [diff] [review] 1136257-shift-click-switch-color-type.patch Review of attachment 8572058 [details] [diff] [review]: ----------------------------------------------------------------- There is a problem when cycling through a swatch that has changed. Open up about:home, then select the body element. Now see the #000 color inherited from html - shift click that a few times. Now, type in "red" into the field. Now shift click some more. Notice that "black" is part of the rotation. Interestingly, it's not a problem when shift-clicking through the initial swatch. ::: browser/devtools/styleinspector/computed-view.js @@ +1152,5 @@ > }); > this.valueNode.innerHTML = ""; > this.valueNode.appendChild(frag); > > + let swatch = this.valueNode.querySelector(".computedview-colorswatch"); I believe almost all of this logic (click handling and color switching) should be encoded into the SwatchColorPickerTooltip so it could be shared between the rule-view and computed-view. Could this work? ::: toolkit/devtools/css-color.js @@ +278,5 @@ > this.authored = color.toLowerCase(); > return this; > }, > > + nextColorUnit: function() { I think this would do the same thing but without having to do the deduplication or grab checking: // Reorder the formats array to have the current format at the // front so we can cycle through. let formats = ["authored", "hex", "hsl", "rgb", "name"]; let putOnEnd = formats.splice(0, formats.indexOf(this.colorUnit)); formats = formats.concat(putOnEnd); let currentDisplayedColor = this[formats[0]]; for (let format of formats) { if (this[format] !== currentDisplayedColor) { this._colorUnit = CssColor.COLORUNIT[format]; break; } } return this.toString();
Attachment #8572058 -
Flags: review?(bgrinstead) → review-
Comment 6•6 years ago
|
||
(In reply to andrepaulino.santos from comment #4) > Is that try for me though? No, it's just a push to the test server to make sure all the tests are passing.
Assignee | ||
Comment 7•6 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #5) > Comment on attachment 8572058 [details] [diff] [review] > 1136257-shift-click-switch-color-type.patch > > Review of attachment 8572058 [details] [diff] [review]: > ----------------------------------------------------------------- > > There is a problem when cycling through a swatch that has changed. Open up > about:home, then select the body element. Now see the #000 color inherited > from html - shift click that a few times. Now, type in "red" into the > field. Now shift click some more. Notice that "black" is part of the > rotation. Interestingly, it's not a problem when shift-clicking through the > initial swatch. > Fixed... just needed to reset the stored color object when the property was updated. > ::: browser/devtools/styleinspector/computed-view.js > @@ +1152,5 @@ > > }); > > this.valueNode.innerHTML = ""; > > this.valueNode.appendChild(frag); > > > > + let swatch = this.valueNode.querySelector(".computedview-colorswatch"); > > I believe almost all of this logic (click handling and color switching) > should be encoded into the SwatchColorPickerTooltip so it could be shared > between the rule-view and computed-view. Could this work? > That was my original approach but it falls short because: 1. The computed view uses DOM Templater. This templater clones everything as part of processing so any added eventlisteners would be lost. We would have to migrate away from DOM Templater in order for this approach to work. 2. The markup for the rule and computed views is slightly different when it comes to color swatches. Of course we could work around that but then we would have code specific to the rule and computed views in Tooltip.js. > ::: toolkit/devtools/css-color.js > @@ +278,5 @@ > > this.authored = color.toLowerCase(); > > return this; > > }, > > > > + nextColorUnit: function() { > > I think this would do the same thing but without having to do the > deduplication or grab checking: > > // Reorder the formats array to have the current format at the > // front so we can cycle through. > let formats = ["authored", "hex", "hsl", "rgb", "name"]; > let putOnEnd = formats.splice(0, formats.indexOf(this.colorUnit)); > formats = formats.concat(putOnEnd); > let currentDisplayedColor = this[formats[0]]; > > for (let format of formats) { > if (this[format] !== currentDisplayedColor) { > this._colorUnit = CssColor.COLORUNIT[format]; > break; > } > } > > return this.toString(); I hadn't put a huge amount of thought into it but that does seem like a far better approach. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f66636a17eb6
Attachment #8572058 -
Attachment is obsolete: true
Attachment #8572582 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 8•6 years ago
|
||
Fixed color order issue: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7664ca2176bd
Attachment #8572582 -
Attachment is obsolete: true
Attachment #8572582 -
Flags: review?(bgrinstead)
Attachment #8572643 -
Flags: review?(bgrinstead)
Comment 9•6 years ago
|
||
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #7) > > ::: browser/devtools/styleinspector/computed-view.js > > @@ +1152,5 @@ > > > }); > > > this.valueNode.innerHTML = ""; > > > this.valueNode.appendChild(frag); > > > > > > + let swatch = this.valueNode.querySelector(".computedview-colorswatch"); > > > > I believe almost all of this logic (click handling and color switching) > > should be encoded into the SwatchColorPickerTooltip so it could be shared > > between the rule-view and computed-view. Could this work? > > > > That was my original approach but it falls short because: > > 1. The computed view uses DOM Templater. This templater clones everything as > part of processing so any added eventlisteners would be lost. We would have > to migrate away from DOM Templater in order for this approach to work. > 2. The markup for the rule and computed views is slightly different when it > comes to color swatches. Of course we could work around that but then we > would have code specific to the rule and computed views in Tooltip.js. What about output-parser.js? It has an _appendColor function already that creates the swatch in all cases. Maybe we could put the shift+click logic there and prevent default on the event such that the tooltip never opens at all.
Comment 10•6 years ago
|
||
Comment on attachment 8572643 [details] [diff] [review] 1136257-shift-click-switch-color-type.patch Review of attachment 8572643 [details] [diff] [review]: ----------------------------------------------------------------- See comment 9 - it seems like we could share most of this logic in the output-parser and move it out of the tooltip/rule-view/computed-view. This would also make any future swatches work automatically. ::: browser/devtools/styleinspector/computed-view.js @@ +1167,5 @@ > + } > + > + // Shift-click causes text selection and we need that for copy paste so all > + // we can do here is quickly clear it. > + this.tree.styleDocument.getSelection().removeAllRanges(); From a quick test I think we can get rid of the range flicker here (and in rule view) by binding to mousedown instead of click and calling preventDefault. @@ +1271,5 @@ > * event. > */ > onMatchedToggle: function PropertyView_onMatchedToggle(aEvent) > { > + if (aEvent.shiftKey) { I'd prefer to see a preventDefault on a dblclick event on the swatch element since it's a more direct way to do this. Or maybe preventing default on mousedown as in the comment above will do the same thing? ::: browser/devtools/styleinspector/rule-view.js @@ +2561,5 @@ > // Adding this swatch to the list of swatches our colorpicker knows about > this.ruleEditor.ruleView.tooltips.cubicBezier.addSwatch(span, { > onPreview: () => this._previewValue(this.valueSpan.textContent), > onCommit: () => this._applyNewValue(this.valueSpan.textContent), > + onRevert: () => this._applyNewValue(originalValue, true) Looks like an unintentional change to the cubic bezier tooltip
Attachment #8572643 -
Flags: review?(bgrinstead)
Reporter | ||
Comment 11•6 years ago
|
||
What's the status of this?
Assignee | ||
Comment 12•6 years ago
|
||
(In reply to André Santos from comment #11) > What's the status of this? Funny you should ask. I decided that this really does belong in the output parser as this is the place that the color swatches are created. This means that we get this behaviour for free wherever we use the output parser to create color swatches. I worked around the problem with DOM Templater removing the listeners in the computed view by stopping using the DOM Templater for creating matched selectors. Of course I have used event.preventDefault() etc. to prevent highlight flickering. bgrins mentioned using a doubleclick event but we should copy Chrome here to make Chrome users feel warm and fuzzy. I have also removed the change from the cubic bezier tooltip. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f81dff5e806
Attachment #8572643 -
Attachment is obsolete: true
Attachment #8584544 -
Flags: review?(bgrinstead)
Comment 13•6 years ago
|
||
Comment on attachment 8584544 [details] [diff] [review] 1136257-shift-click-switch-color-type.patch Review of attachment 8584544 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/shared/test/browser_css_color.js @@ -47,5 @@ > testProcessCSSString(); > testSetAlpha(); > } > > function testToString(color, name, hex, hsl, rgb) { Do we have a separate test for the default color unit pref? If not, we should keep around a similar test to this one that initializes a new color after setting the default color pref and making sure the correct value comes out of toString(). ::: browser/devtools/shared/widgets/Tooltip.js @@ +968,5 @@ > _onSwatchClick: function(event) { > let swatch = this.swatches.get(event.target); > + > + if (event.shiftKey) { > + event.preventDefault(); I don't *think* the preventDefault() is necessary here, is it? I know the stopProp is needed so the inplaceEditor doesn't open up ::: browser/devtools/styleinspector/computed-view.js @@ +1540,5 @@ > + * The tag name. > + * @param {object} aAttributes > + * A set of attributes to set on the node. > + */ > +function createChild(aParent, aTag, aAttributes={}) { We should export this from a shared module since it's copy/pasted from the rule view. Surprisingly, I don't think we have a shared file for this type of thing yet. Maybe viewhelpers.jsm, or styleeditor/utils.js. We can do that in a follow up though ::: browser/devtools/styleinspector/test/browser_ruleview_colorpicker-cycle-color.js @@ +3,5 @@ > + http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +"use strict"; > + > +// Test cycling color types in the rule view color picker tooltip This file name and comment indicate that it's happening inside the tooltip, but really it's just testing the swatch in the rule view. Please rename to match the computedview test name and update this comment, along with all the other strings in this file that say 'color picker'. @@ +5,5 @@ > +"use strict"; > + > +// Test cycling color types in the rule view color picker tooltip > + > +const PAGE_CONTENT = [ Nit: The way this test and the computed view test are setup are a bit different - can you use one style (PAGE_CONTENT or setting innerHTML) so these two new tests match? @@ +39,5 @@ > + EventUtils.synthesizeMouseAtCenter(swatch, {type: "mousedown", shiftKey: true}, win); > + is(valueNode.textContent, "rgb(255, 0, 0)", > + "Color displayed as an RGB value."); > + > + EventUtils.synthesizeMouseAtCenter(swatch, {type: "mousedown", shiftKey: true}, win); This should cycle one more time to make sure it returns back to the original format ::: toolkit/devtools/output-parser.js @@ +422,5 @@ > + let val = color.nextColorUnit(); > + > + swatch.nextElementSibling.textContent = val; > + > + event.preventDefault(); You could move this call to event.preventDefault() to the top of the function so that it prevents text selection in the case of double-clicking (non-shift) a swatch. Also, please add a comment about how it's needed to prevent text selection. @@ +423,5 @@ > + > + swatch.nextElementSibling.textContent = val; > + > + event.preventDefault(); > + return false; Do we need to the return false here?
Attachment #8584544 -
Flags: review?(bgrinstead) → feedback+
Updated•6 years ago
|
Whiteboard: [devedition-40]
Comment 14•6 years ago
|
||
Setting devedition-40 bugs to p3, filter on FB20EAC4-3FC3-48D9-B15F-0587C3987C25
Priority: -- → P3
Assignee | ||
Comment 15•6 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #13) > Comment on attachment 8584544 [details] [diff] [review] > 1136257-shift-click-switch-color-type.patch > > Review of attachment 8584544 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/devtools/shared/test/browser_css_color.js > @@ -47,5 @@ > > testProcessCSSString(); > > testSetAlpha(); > > } > > > > function testToString(color, name, hex, hsl, rgb) { > > Do we have a separate test for the default color unit pref? If not, we > should keep around a similar test to this one that initializes a new color > after setting the default color pref and making sure the correct value comes > out of toString(). > A bunch of tests check this (e.g. the css text says red but the rule view shows #F00). > ::: browser/devtools/shared/widgets/Tooltip.js > @@ +968,5 @@ > > _onSwatchClick: function(event) { > > let swatch = this.swatches.get(event.target); > > + > > + if (event.shiftKey) { > > + event.preventDefault(); > > I don't *think* the preventDefault() is necessary here, is it? I know the > stopProp is needed so the inplaceEditor doesn't open up > Removed. > ::: browser/devtools/styleinspector/computed-view.js > @@ +1540,5 @@ > > + * The tag name. > > + * @param {object} aAttributes > > + * A set of attributes to set on the node. > > + */ > > +function createChild(aParent, aTag, aAttributes={}) { > > We should export this from a shared module since it's copy/pasted from the > rule view. Surprisingly, I don't think we have a shared file for this type > of thing yet. Maybe viewhelpers.jsm, or styleeditor/utils.js. We can do > that in a follow up though > Logged bug 1152721. > ::: > browser/devtools/styleinspector/test/browser_ruleview_colorpicker-cycle- > color.js > @@ +3,5 @@ > > + http://creativecommons.org/publicdomain/zero/1.0/ */ > > + > > +"use strict"; > > + > > +// Test cycling color types in the rule view color picker tooltip > > This file name and comment indicate that it's happening inside the tooltip, > but really it's just testing the swatch in the rule view. Please rename to > match the computedview test name and update this comment, along with all the > other strings in this file that say 'color picker'. > Done. Comments now say e.g. // Test cycling color types in the rule view > @@ +5,5 @@ > > +"use strict"; > > + > > +// Test cycling color types in the rule view color picker tooltip > > + > > +const PAGE_CONTENT = [ > > Nit: The way this test and the computed view test are setup are a bit > different - can you use one style (PAGE_CONTENT or setting innerHTML) so > these two new tests match? > Done > @@ +39,5 @@ > > + EventUtils.synthesizeMouseAtCenter(swatch, {type: "mousedown", shiftKey: true}, win); > > + is(valueNode.textContent, "rgb(255, 0, 0)", > > + "Color displayed as an RGB value."); > > + > > + EventUtils.synthesizeMouseAtCenter(swatch, {type: "mousedown", shiftKey: true}, win); > > This should cycle one more time to make sure it returns back to the original > format > Done > ::: toolkit/devtools/output-parser.js > @@ +422,5 @@ > > + let val = color.nextColorUnit(); > > + > > + swatch.nextElementSibling.textContent = val; > > + > > + event.preventDefault(); > > You could move this call to event.preventDefault() to the top of the > function so that it prevents text selection in the case of double-clicking > (non-shift) a swatch. Also, please add a comment about how it's needed to > prevent text selection. > Done > @@ +423,5 @@ > > + > > + swatch.nextElementSibling.textContent = val; > > + > > + event.preventDefault(); > > + return false; > > Do we need to the return false here? Not any more... removed.
Attachment #8584544 -
Attachment is obsolete: true
Attachment #8590195 -
Flags: review?(bgrinstead)
Comment 16•6 years ago
|
||
Comment on attachment 8590195 [details] [diff] [review] 1136257-shift-click-switch-color-type.patch Review of attachment 8590195 [details] [diff] [review]: ----------------------------------------------------------------- Nice. Looks ready to go if it's got a green try push
Attachment #8590195 -
Flags: review?(bgrinstead) → review+
Updated•6 years ago
|
Whiteboard: [devedition-40] → [devedition-40][difficulty=easy]
Assignee | ||
Comment 17•6 years ago
|
||
Rebased. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d25ef3533b4d
Attachment #8590195 -
Attachment is obsolete: true
Attachment #8590841 -
Flags: review+
Assignee | ||
Comment 18•6 years ago
|
||
Rebased. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc09f3b17672
Attachment #8590841 -
Attachment is obsolete: true
Attachment #8593356 -
Flags: review+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 19•6 years ago
|
||
Hi Michael, this seems still to have problems to apply: patching file browser/devtools/styleinspector/computedview.xhtml Hunk #1 FAILED at 74 1 out of 1 hunks FAILED -- saving rejects to file browser/devtools/styleinspector/computedview.xhtml.rej patching file browser/devtools/styleinspector/test/browser.ini Hunk #2 succeeded at 96 with fuzz 1 (offset 2 lines). patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh 1136257-shift-click-switch-color-type.patch maybe needs rebased against fx-team ?
Flags: needinfo?(mratcliffe)
Keywords: checkin-needed
Assignee | ||
Comment 20•6 years ago
|
||
(In reply to Carsten Book [:Tomcat] from comment #19) > Hi Michael, this seems still to have problems to apply: > > patching file browser/devtools/styleinspector/computedview.xhtml > Hunk #1 FAILED at 74 > 1 out of 1 hunks FAILED -- saving rejects to file > browser/devtools/styleinspector/computedview.xhtml.rej > patching file browser/devtools/styleinspector/test/browser.ini > Hunk #2 succeeded at 96 with fuzz 1 (offset 2 lines). > patch failed, unable to continue (try -v) > patch failed, rejects left in working dir > errors during apply, please fix and refresh > 1136257-shift-click-switch-color-type.patch > > maybe needs rebased against fx-team ? Yeah, computedview.xhtml changed shortly after adding checkin-needed. Rebased.
Attachment #8593356 -
Attachment is obsolete: true
Flags: needinfo?(mratcliffe)
Attachment #8593886 -
Flags: review+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/d321fbee8cd8
Keywords: checkin-needed
Whiteboard: [devedition-40][difficulty=easy] → [devedition-40][difficulty=easy][fixed-in-fx-team]
I backed this out in https://hg.mozilla.org/integration/fx-team/rev/bf544e556f37 under suspicion of it breaking e10s-dt: https://treeherder.mozilla.org/logviewer.html#?job_id=2751395&repo=fx-team
Flags: needinfo?(mratcliffe)
Whiteboard: [devedition-40][difficulty=easy][fixed-in-fx-team] → [devedition-40][difficulty=easy]
The dt orange kept going until I backed out bug 1098374. This can probably reland.
Keywords: checkin-needed
Comment 24•6 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/9c9c55112ff6
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [devedition-40][difficulty=easy] → [devedition-40][difficulty=easy][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9c9c55112ff6
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [devedition-40][difficulty=easy][fixed-in-fx-team] → [devedition-40][difficulty=easy]
Target Milestone: --- → Firefox 40
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(mratcliffe)
Updated•6 years ago
|
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]
Updated•3 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•