When clicking on a color swatch, a colorpicker will open to allow changing the CSS value with a colorpicker UI. This colorpicker will respect existing transparency and color formats as much as possible. There will still be the ability to switch color formats (as you currently can now by clicking on a swatch), but it will be caused by a right click, or control+click event instead of the current click event. This is based on the discussion here: http://groups.google.com/group/google-chrome-developer-tools/browse_thread/thread/4dd1e853b8051727 I have a version of this working with a hacked devtools_frontend, and I am working on turning this into a proper patch.
Created attachment 115923 [details] First attempted patch. Hopefully have included everything correctly This is ready for initial UI review, with a couple of caveats. * Still need to handle UI for cases where picker opens near the bottom of the panel (right now it still shows up below the swatch causing an extra scrollbar). * Also, there is a bug after adding a new property to the element (the picker hides while dragging). I am looking into this but since I am new to this project it is somewhat difficult to tell why that acts differently than the standard case. Even with these issues, I thought I would submit it in the current state to get some feedback while trying to handle them.
Created attachment 115979 [details] [IMAGE] Screenshot with patch applied.
Comment on attachment 115923 [details] First attempted patch. Hopefully have included everything correctly View in context: https://bugs.webkit.org/attachment.cgi?id=115923&action=review Thanks for putting it together. I made a first pass on the change to highlight some of the WebKit style guidelines and pointed to the UI utilities you could leverage. I'll comment on the UI separately. > Source/WebCore/ChangeLog:3 > + first commit for colorpicker functionality This should be below WebInspector: topic, above Reviewed by line. Should also describe what you are doing in 1-2 sentences. > Source/WebCore/ChangeLog:10 > + No new tests. (OOPS!) You should remove this line. There are currently no tests for inspector front-end UI components. > Source/WebCore/inspector/front-end/Spectrum.js:2 > +WebInspector.Spectrum = function(swatch, rgb) We are using Element suffix for all DOM element variables: swatch -> swatchElement Passing Color instead of rgb would make more sense: otherwise it is not clear whether this is rgb or rgba. > Source/WebCore/inspector/front-end/Spectrum.js:4 > + this.document = swatch.ownerDocument; We don't use iframes at the moment, so you should not need this. > Source/WebCore/inspector/front-end/Spectrum.js:5 > + this.swatch = swatch; Here and below: prefix all private variables that are not accessed from outside this file with "_". this._swatchElement in this case. > Source/WebCore/inspector/front-end/Spectrum.js:8 > + this.element.innerHTML = WebInspector.Spectrum.markup; We are not using any templating in the front-end, you should create your element using DOM API instead. There is Element.prototype.createChild = function(elementName, className) defined that is likely to make your code compact. > Source/WebCore/inspector/front-end/Spectrum.js:16 > + this.slider = this.element.querySelectorAll(".sp-hue")[0]; You won't need these once you create your dom programmatically. > Source/WebCore/inspector/front-end/Spectrum.js:31 > + this.updateUI(); Methods should also be private (this._updateUI()) > Source/WebCore/inspector/front-end/Spectrum.js:82 > +WebInspector.Spectrum.hsvToRgb = function(h, s, v, a) { Seems like this could belong to Color.js > Source/WebCore/inspector/front-end/Spectrum.js:116 > +WebInspector.Spectrum.rgbToHsv = function(r, g, b, a) { ditto > Source/WebCore/inspector/front-end/Spectrum.js:149 > +WebInspector.Spectrum.stopPropagation = function(e) { Please try to use existing framework methods where possible or define generic ones in utilities.js. > Source/WebCore/inspector/front-end/Spectrum.js:154 > + if (typeof name === "object") { We are preparing the front-end for closure compilation, so flexibility like this harms us. > Source/WebCore/inspector/front-end/Spectrum.js:176 > + return { left: el.totalOffsetLeft(), top: el.totalOffsetTop() }; should be defined on Element.prototype in utilities.js > Source/WebCore/inspector/front-end/Spectrum.js:179 > +WebInspector.Spectrum.getScrollOffset = function(el) { ditto > Source/WebCore/inspector/front-end/Spectrum.js:190 > +WebInspector.Spectrum.draggable = function(element, onmove, onstart, onstop) { There is WebInspector.elementDragStart that has somewhat similar logic. Have you seen it? > Source/WebCore/inspector/front-end/Spectrum.js:260 > +WebInspector.Spectrum.prototype = { This class should probably be called WebInspector.SpectrumControl to better reflect the nature of the component. > Source/WebCore/inspector/front-end/Spectrum.js:386 > + addChangeListener: function(listener) { Inheriting from Object.js provides you with the generic event mechanism. You would declare WebInspector.Spectrum.Event object with event names and dispatch them from here and from onhide. > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1680 > + var rgba = (color.rgba || color.rgb).slice(0); It sounds like Color class itself is broken. It should maintain single data entry for rgb / rgba. > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1681 > + var spectrum = new WebInspector.Spectrum(swatchElement, rgba); As I mentioned earlier, passing Color type here would be more straightforward. > Source/WebCore/inspector/front-end/inspector.css:2644 > +.sp-container { Please be more verbose and expand those to .spectrum-* > Source/WebCore/inspector/front-end/inspector.html:84 > + <script type="text/javascript" src="Spectrum.js"></script> You will need to add this file to: WebCore/WebCore.gypi, WebCore/inspector/front-end/WebKit.qrc and WebCore/WebCore.vcproj/WebCore.vjproj (last one uses tabs, not spaces). Sorry for trouble.
Few thoughts on the UI: 1) I don't see clearly the color I am building. Would be nice to have a large preview box to the left from the main spectrum box. 2) It is hard to assess the transparency. I thought I was chess board mentioned somewhere. 3) Close icon is not consistent with the rest of the UI. Could we drop it and see if it works? 4) It is not clear that transparency control is for tuning transparency. I thought we didn't need to mirror values for rgba in the control, but now that I see it embedded, I kinda miss those.
This would look best as a popover pointing to the swatch too.
Copy that, I'll make the suggested changes. Should I revert my initial changelog, and rebase all of the new changes into the initial commit, then generate a new changelog and patch? (In reply to comment #3) > (From update of attachment 115923 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=115923&action=review > > Thanks for putting it together. I made a first pass on the change to highlight some of the WebKit style guidelines and pointed to the UI utilities you could leverage. I'll comment on the UI separately. > > > Source/WebCore/ChangeLog:3 > > + first commit for colorpicker functionality > > This should be below WebInspector: topic, above Reviewed by line. Should also describe what you are doing in 1-2 sentences. > > > Source/WebCore/ChangeLog:10 > > + No new tests. (OOPS!) > > You should remove this line. There are currently no tests for inspector front-end UI components. > > > Source/WebCore/inspector/front-end/Spectrum.js:2 > > +WebInspector.Spectrum = function(swatch, rgb) > > We are using Element suffix for all DOM element variables: swatch -> swatchElement > Passing Color instead of rgb would make more sense: otherwise it is not clear whether this is rgb or rgba. > > > Source/WebCore/inspector/front-end/Spectrum.js:4 > > + this.document = swatch.ownerDocument; > > We don't use iframes at the moment, so you should not need this. > > > Source/WebCore/inspector/front-end/Spectrum.js:5 > > + this.swatch = swatch; > > Here and below: prefix all private variables that are not accessed from outside this file with "_". this._swatchElement in this case. > > > Source/WebCore/inspector/front-end/Spectrum.js:8 > > + this.element.innerHTML = WebInspector.Spectrum.markup; > > We are not using any templating in the front-end, you should create your element using DOM API instead. There is Element.prototype.createChild = function(elementName, className) defined that is likely to make your code compact. > > > Source/WebCore/inspector/front-end/Spectrum.js:16 > > + this.slider = this.element.querySelectorAll(".sp-hue")[0]; > > You won't need these once you create your dom programmatically. > > > Source/WebCore/inspector/front-end/Spectrum.js:31 > > + this.updateUI(); > > Methods should also be private (this._updateUI()) > > > Source/WebCore/inspector/front-end/Spectrum.js:82 > > +WebInspector.Spectrum.hsvToRgb = function(h, s, v, a) { > > Seems like this could belong to Color.js > > > Source/WebCore/inspector/front-end/Spectrum.js:116 > > +WebInspector.Spectrum.rgbToHsv = function(r, g, b, a) { > > ditto > > > Source/WebCore/inspector/front-end/Spectrum.js:149 > > +WebInspector.Spectrum.stopPropagation = function(e) { > > Please try to use existing framework methods where possible or define generic ones in utilities.js. > > > Source/WebCore/inspector/front-end/Spectrum.js:154 > > + if (typeof name === "object") { > > We are preparing the front-end for closure compilation, so flexibility like this harms us. > > > Source/WebCore/inspector/front-end/Spectrum.js:176 > > + return { left: el.totalOffsetLeft(), top: el.totalOffsetTop() }; > > should be defined on Element.prototype in utilities.js > > > Source/WebCore/inspector/front-end/Spectrum.js:179 > > +WebInspector.Spectrum.getScrollOffset = function(el) { > > ditto > > > Source/WebCore/inspector/front-end/Spectrum.js:190 > > +WebInspector.Spectrum.draggable = function(element, onmove, onstart, onstop) { > > There is WebInspector.elementDragStart that has somewhat similar logic. Have you seen it? > > > Source/WebCore/inspector/front-end/Spectrum.js:260 > > +WebInspector.Spectrum.prototype = { > > This class should probably be called WebInspector.SpectrumControl to better reflect the nature of the component. > > > Source/WebCore/inspector/front-end/Spectrum.js:386 > > + addChangeListener: function(listener) { > > Inheriting from Object.js provides you with the generic event mechanism. You would declare WebInspector.Spectrum.Event object with event names and dispatch them from here and from onhide. > > > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1680 > > + var rgba = (color.rgba || color.rgb).slice(0); > > It sounds like Color class itself is broken. It should maintain single data entry for rgb / rgba. > > > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1681 > > + var spectrum = new WebInspector.Spectrum(swatchElement, rgba); > > As I mentioned earlier, passing Color type here would be more straightforward. > > > Source/WebCore/inspector/front-end/inspector.css:2644 > > +.sp-container { > > Please be more verbose and expand those to .spectrum-* > > > Source/WebCore/inspector/front-end/inspector.html:84 > > + <script type="text/javascript" src="Spectrum.js"></script> > > You will need to add this file to: WebCore/WebCore.gypi, WebCore/inspector/front-end/WebKit.qrc and WebCore/WebCore.vcproj/WebCore.vjproj (last one uses tabs, not spaces). Sorry for trouble.
Timothy, Is there a certain css class or JavaScript function I can use to achieve the popover effect? Thanks, Brian (In reply to comment #5) > This would look best as a popover pointing to the swatch too.
There was supposed to be a checkerboard on the original swatch element, but now I see that it isn't updating properly. I could do any of these: * Make the swatch element update to show transparency (I think this is just a background image that isn't being applied) * Put a checkboard behind the actual colorpicker region and apply transparency to that. I am not a huge fan of this, since when you have alpha it is hard to see the actual h/s/v you have chosen. * Show a label next to the transparency slider to make it clear that it is for changing alpha * Show a text representation of currently selected color inside of the picker. Note: this is currently happening in the actual value element that you are modifying, but it may be hard to see. I don't have any problem dropping the close button, it really doesn't look very good - I just used an existing icon from the project. (In reply to comment #4) > Few thoughts on the UI: > > 1) I don't see clearly the color I am building. Would be nice to have a large preview box to the left from the main spectrum box. > 2) It is hard to assess the transparency. I thought I was chess board mentioned somewhere. > 3) Close icon is not consistent with the rest of the UI. Could we drop it and see if it works? > 4) It is not clear that transparency control is for tuning transparency. > > I thought we didn't need to mirror values for rgba in the control, but now that I see it embedded, I kinda miss those.
(In reply to comment #6) > Copy that, I'll make the suggested changes. Should I revert my initial changelog, and rebase all of the new changes into the initial commit, then generate a new changelog and patch? I would re-build ChangeLog, yes. (In reply to comment #7) > Is there a certain css class or JavaScript function I can use to achieve the popover effect? > Take a look at the Popover.js and the PopoverHelper there. It is basically a rich focusable tooltip control. I was going to suggest that you use it, but it hardcodes the yellowish background, so I was not sure it applies. It also disappears upon mouse move out, but that should be configurable. In either case you could at least re-use its utility code for positioning your box next to the anchor element. (In reply to comment #8) > There was supposed to be a checkerboard on the original swatch element, but now I see that it isn't updating properly. I could do any of these: > * Make the swatch element update to show transparency (I think this is just a background image that isn't being applied) That would be nice. > * Put a checkboard behind the actual colorpicker region and apply transparency to that. I am not a huge fan of this, since when you have alpha it is hard to see the actual h/s/v you have chosen. I would not do that as well. > * Show a label next to the transparency slider to make it clear that it is for changing alpha > * Show a text representation of currently selected color inside of the picker. Note: this is currently happening in the actual value element that you are modifying, but it may be hard to see. Yeah, that's what I meant. I now think you should mimic it inside the picker as well. > I don't have any problem dropping the close button, it really doesn't look very good - I just used an existing icon from the project. Lets see if clicking away / pressing Esc or toggling swatch back would be sufficient.
Attachment 115923 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/inspector/front-end/Spectrum.js:336: Line contains tab character. [whitespace/tab] [5] Source/WebCore/inspector/front-end/Spectrum.js:341: Line contains tab character. [whitespace/tab] [5] Source/WebCore/inspector/front-end/Spectrum.js:342: Line contains tab character. [whitespace/tab] [5] Source/WebCore/inspector/front-end/inspector.css:2675: Line contains tab character. [whitespace/tab] [5] Source/WebCore/inspector/front-end/inspector.css:2681: Line contains tab character. [whitespace/tab] [5] Total errors found: 5 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #3) I have finished most of this (and the UI) feedback, but am not sure how to proceed with the Color.js changes. > > Source/WebCore/inspector/front-end/Spectrum.js:82 > > +WebInspector.Spectrum.hsvToRgb = function(h, s, v, a) { > > Seems like this could belong to Color.js > > > Source/WebCore/inspector/front-end/Spectrum.js:116 > > +WebInspector.Spectrum.rgbToHsv = function(r, g, b, a) { > > ditto > I do agree that the Color class should probably have this functionality. Since HSV colors aren't supported by the css parser it might be confusing if it worked in the web developer ui. If someone copied an HSV into the value element, for instance, that could be confusing. > > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1680 > > + var rgba = (color.rgba || color.rgb).slice(0); > > It sounds like Color class itself is broken. It should maintain single data entry for rgb / rgba. > I was a little leery of making changes to the color class since I didn't know of all the places it was used. I didn't want to change the default behavior of return values for this patch to prevent messing up existing code. I don't mind making this change, but I would want some guidance on where to look for problems. Besides that, I have two issues I want to address before resubmitting a patch: 1. The big issue I am still seeing is that when I add a new property: value; string to an element. When I open the colorpicker on that element, it seems like it is reinitializing the picker on a minor change event. This might be because the initialization code is in the updateTitle function. I have been trying to console.trace() or enter into the debugger to figure out what is going on, but haven't quite gotten it yet. Any tips on why this might be happening? 2. Also, I want to attach the picker to the _parentPane instead of the valueElement, so that I can put labels inside of the picker (I can't leave it in the valueElement because other code uses valueElement.textContent to determine what CSS to apply). For some reason, the self._parentPane is sometimes null in updateTitle, so I ended up checking this: if (self._parentPane) { // initialize colorpicker only if there is a parentPane var spectrum = new WebInspector.Spectrum(swatchElement, self._parentPane.bodyElement, rgba); } I'm not sure why this code is being called on StylePropertyTreeElement objects that have a null _parentPane. Again, any tips on why this might be and if what I am doing now should be fine? You
One missing point in the patch is the ability to revert the change (e.g. on clicking outside of the picker, as opposed to the "commit" arrow-button). Another thing that I noticed is that you create as many WebInspector.Spectrum instances (and its DOM replicas) as you have color swatches in the StylesSidebarPane. You should only have one instance of Spectrum (perhaps stored in the StylesSidebarPane instance) which will get attached to the proper swatch/property. I also did not quite get the problem stated in the item 1 about adding a new property. A common rule is that only one field (property name/value) can be edited at any time. Could you give us an exact series of steps to reproduce the issue? Your second problem is (at least partly) due to the fact that the ComputedStylePropertiesSection StylePropertyTreeElements are created with a null parentPane (see WebInspector.ComputedStylePropertiesSection.prototype.onpopulate). In fact, you should not worry about it, since bringing up a color picker for computed property values makes no sense.
(In reply to comment #12) > One missing point in the patch is the ability to revert the change (e.g. on clicking outside of the picker, as opposed to the "commit" arrow-button). > The commit arrow button has been removed based on initial UI feedback. To me it feels like "clicking out" of the picker is a natural way to close *without* reverting. We could either change that default behavior, add a cancel button or 'X' in the top right corner to revert, or maybe make the new color text focused with after exiting with the ability to press escape to revert to the original value. Not sure how hard that last one would be to implement. > I also did not quite get the problem stated in the item 1 about adding a new property. A common rule is that only one field (property name/value) can be edited at any time. Could you give us an exact series of steps to reproduce the issue? > Click on an element on the left pane like the body. When it pulls up all of the styles on the right, you should be able to click on a color and use the picker as intended. Now, double click into element.style and add a property, like "background": "red". If you click on this new swatch that gets created and start dragging the colorpicker around, it disappears after a second (but your changes do get applied). My guess is that it is calling the updateTitle function every time a minor change happens to a new property.
(In reply to comment #13) > (In reply to comment #12) > > One missing point in the patch is the ability to revert the change (e.g. on clicking outside of the picker, as opposed to the "commit" arrow-button). > > > > The commit arrow button has been removed based on initial UI feedback. To me it feels like "clicking out" of the picker is a natural way to close *without* reverting. We could either change that default behavior, add a cancel button or 'X' in the top right corner to revert, or maybe make the new color text focused with after exiting with the ability to press escape to revert to the original value. Not sure how hard that last one would be to implement. Handling keys is not hard but I'll defer the final decision to Pavel who is also the primary UI advisor for Web Inspector. > > I also did not quite get the problem stated in the item 1 about adding a new property. A common rule is that only one field (property name/value) can be edited at any time. Could you give us an exact series of steps to reproduce the issue? > > > > Click on an element on the left pane like the body. When it pulls up all of the styles on the right, you should be able to click on a color and use the picker as intended. Now, double click into element.style and add a property, like "background": "red". If you click on this new swatch that gets created and start dragging the colorpicker around, it disappears after a second (but your changes do get applied). My guess is that it is calling the updateTitle function every time a minor change happens to a new property. Oh, now I get your point. You don't need to edit a color before creating and editing one in element.style. Whenever you edit element.style, the "style" attribute for the inspected element gets invalidated, and all its styles get re-requested, thereby rebuilding the entire Styles pane (which, of course, blows away the color swatch and the spectrum element attached to it). We've got a flag defined on the StylesSidebarPane to avoid updates during style edits. Your related snippets should look similar to this (provided you implement necessary checks for self._parentPane during the setup phase): ... function finishSpectrum(e) { swatchElement.ownerDocument.removeEventListener("click", finishSpectrum, false); spectrum.hide(e); delete self._parentPane._isEditingStyle; } ... if (isShown) { self._parentPane._isEditingStyle = true; swatchElement.ownerDocument.addEventListener("click", finishSpectrum, false); } ...
Created attachment 116361 [details] Screenshot with updated patch running Using popover.js, showing selected color and label
Created attachment 116365 [details] Patch with updates (see comments for details) Updated commit for colorpicker functionality based on feedback from https://bugs.webkit.org/show_bug.cgi?id=71262. Changes include: using only one colorpicker for the whole styles panel, UI improvements, working on element.style, native WebKit.Color within plugin, use webkit style for code, update project references, use popover instead of custom measurements.
Created attachment 116391 [details] Same as https://bug-71262-attachments.webkit.org/attachment.cgi?id=116365, but with latest master Forgot to update master before creating the last patch. This one fixes that.
Attachment 116391 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/inspector/front-end/Spectrum.js:317: Line contains tab character. [whitespace/tab] [5] Source/WebCore/inspector/front-end/Spectrum.js:318: Line contains tab character. [whitespace/tab] [5] Source/WebCore/inspector/front-end/inspector.css:2678: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] Total errors found: 4 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 116391 [details] Same as https://bug-71262-attachments.webkit.org/attachment.cgi?id=116365, but with latest master View in context: https://bugs.webkit.org/attachment.cgi?id=116391&action=review The change looks better with the previous comments addressed. A couple of general notes: - try to make Spectrum a reusable component not directly tied to the Styles sidebar - use "===" rather than "==" wherever possible - don't use blank lines to separate opening and closing braces of method bodies - clean up duplicate blank lines (you use them them very rarely in the code) - if in doubt, use the same style as the surrounding code does, even if it's against the rules :) > Source/WebCore/ChangeLog:7 > + updated commit for colorpicker functionality based on feedback from https://bugs.webkit.org/show_bug.cgi?id=71262. Changes include: using only one colorpicker for the whole styles panel, UI improvements, working on element.style, native WebKit.Color within plugin, use webkit style for code, update project references, use popover instead of custom measurements The message usually starts with a capital letter and has line breaks at reasonable intervals (usually around 100-120 characters). > Source/WebCore/ChangeLog:9 > + Reviewed by NOBODY (OOPS!). This should be aligned with the rest of text. Webkit uses only spaces not tabs. > Source/WebCore/inspector/front-end/Popover.js:102 > + Inadvertent change > Source/WebCore/inspector/front-end/Popover.js:176 > + ditto > Source/WebCore/inspector/front-end/Spectrum.js:22 > + alphaLabel.textContent = "alpha: "; This should be i18n-ized using WebInspector.UIString() / localizedStrings.js > Source/WebCore/inspector/front-end/Spectrum.js:30 > + extra blank line > Source/WebCore/inspector/front-end/Spectrum.js:31 > + var swatchClone = WebInspector.Spectrum.getSwatchElement(); In general, I'd be wary of this ad-hoc solution, as we'd like to make Spectrum a reusable component. What if it should be invoked on a button click and modify the contents of an edit box on change? Spectrum might inherit Object and dispatch events when changed and closed, and clients would add/remove listeners for these events. > Source/WebCore/inspector/front-end/Spectrum.js:46 > + //container.appendChild(this._containerElement); stray commented line > Source/WebCore/inspector/front-end/Spectrum.js:53 > + this._onchange(); can _onchange() incorporate a this._updateUI() call? > Source/WebCore/inspector/front-end/Spectrum.js:84 > +WebInspector.Spectrum.hsvToRgb = function(h, s, v, a) { This should be "hsvToRGB" according to the webkit naming guidelines > Source/WebCore/inspector/front-end/Spectrum.js:118 > +WebInspector.Spectrum.rgbToHsv = function(r, g, b, a) { Ditto > Source/WebCore/inspector/front-end/Spectrum.js:128 > + s = max == 0 ? 0 : d / max; Prefer "===" over "==" everywhere > Source/WebCore/inspector/front-end/Spectrum.js:153 > + onmove = onmove || function() { }; Pavel feels strongly against nested anonymous functions, you can define a named one above. > Source/WebCore/inspector/front-end/Spectrum.js:158 > + var dragging = false; You don't need to initialize these 4 locals here (dragging will be undefined which is equivalent of false in the inspector code most of the time). > Source/WebCore/inspector/front-end/Spectrum.js:171 > + e.returnValue = false; We don't normally rely on non-standard properties, this must be a leftover from IE-compatible code. You also don't need the method presence checks above. > Source/WebCore/inspector/front-end/Spectrum.js:177 > + var dragX = Math.max(0, Math.min(e.pageX - offset.left + scrollOffset.left, maxWidth)); Where does scrollOffset come from? It should be at least defined in the closure scope, or else it gets defined on the window object. > Source/WebCore/inspector/front-end/Spectrum.js:185 > + var rightclick = (e.which) ? (e.which == 3) : (e.button == 2); Webkit tends to use camelcased identifiers ("rightClick") > Source/WebCore/inspector/front-end/Spectrum.js:188 > + if (onstart.apply(element, arguments) !== false) { Try to avoid explicit boolean comparisons (see the WebKit coding guidelines) > Source/WebCore/inspector/front-end/Spectrum.js:226 > + if (rgba.length < 4) { One-line blocks do not have braces. > Source/WebCore/inspector/front-end/Spectrum.js:237 > + if (rgb[3] == 1) We use "===" wherever possible. > Source/WebCore/inspector/front-end/Spectrum.js:243 > + get colorNoSatVal() Webkit mostly avoids abbreviations ("colorWithFullSaturation"?) > Source/WebCore/inspector/front-end/Spectrum.js:251 > + extra blank line? > Source/WebCore/inspector/front-end/Spectrum.js:257 > + _isShown: false, No need to explicitly define this. Historically, we either compute such properties in getters or create them when they turn "true" and delete (using the "delete" operator) when they turn "false". > Source/WebCore/inspector/front-end/Spectrum.js:262 > + extra blank line? > Source/WebCore/inspector/front-end/Spectrum.js:265 > + Web Inspector does not separate opening and closing braces for a function with blank lines. > Source/WebCore/inspector/front-end/Spectrum.js:285 > + // Where to show the bar that displays your current selected hue Period at the end. > Source/WebCore/inspector/front-end/Spectrum.js:289 > + extra blank line? > Source/WebCore/inspector/front-end/Spectrum.js:298 > + if (rgb.length < 4) can it be, for example, 2? If applicable, check for "=== 3". > Source/WebCore/inspector/front-end/Spectrum.js:301 > + var rgbNoSatVal = this.colorNoSatVal.rgb; Please avoid abbreviations if possible > Source/WebCore/inspector/front-end/Spectrum.js:318 > + return this._isShown; odd indent > Source/WebCore/inspector/front-end/Spectrum.js:340 > + extra blank line? > Source/WebCore/inspector/front-end/Spectrum.js:353 > + extra blank line? > Source/WebCore/inspector/front-end/Spectrum.js:355 > + // existing event listeners Period at the end. > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1668 > + extra blank lines > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1672 > + // Alt + click toggles color formats Comments should be full sentences followed by periods. > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1673 > + // Click opens colorpicker, only if the element is not in computed styles section) A closing parenthesis instead of a period. > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1677 > + } "else if" should be found on this line > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1690 > + ditto > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1758 > + inadvertent change? > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1774 > + ditto > Source/WebCore/inspector/front-end/inspector.css:2669 > + position:absolute; The CSS property format everywhere is: property: value; > Source/WebCore/inspector/front-end/inspector.css:2680 > +.spectrum-top { separate all rules with blank lines > Source/WebCore/inspector/front-end/inspector.css:2681 > + position:relative; odd indentation > Source/WebCore/inspector/front-end/inspector.css:2686 > + position:absolute; top:0; left:0; bottom:0; right:0; odd indentation. Also, please don't use more than one property per line. > Source/WebCore/inspector/front-end/inspector.css:2690 > + top:0;left:0;bottom:0;right:20%; One property per line > Source/WebCore/inspector/front-end/inspector.css:2694 > + top:0;right:0;bottom:0;left:83%; ditto > Source/WebCore/inspector/front-end/inspector.css:2697 > + margin-top: 80%; /* Same as spectrum-color width */ you can have a common class for the elements that should have margin-top: 80% > Source/WebCore/inspector/front-end/utilities.js:286 > + var totalLeft = totalTop = 0; One declaration per line. > Source/WebCore/inspector/front-end/utilities.js:296 > + var curleft = curtop = 0; Webkit uses one declaration per line, and camelcased variable identifiers (also, |left| and |top| would do just fine here). > Source/WebCore/inspector/front-end/utilities.js:298 > + if (el.offsetParent) { Is this really consistent? I.e., the loop does not run if |this| has no offsetParent but will run for some of |this|'s offsetParent-ancestors having no offsetParent. Or do you mean that BODY has no offsetParent but can have scrollLeft/Top? If so, please add a comment to clarify this, otherwise the following snippet could be reused: while (el.offsetParent) { curLeft += el.scrollLeft; curTop += el.scrollTop; el = el.offsetParent; } > Source/WebCore/inspector/front-end/utilities.js:325 > + Odd change > Source/WebCore/inspector/front-end/utilities.js:907 > + extra blank line
Created attachment 116686 [details] Updating whitespace / webkit style This handles most of the feedback from my latest patch. I realize there isn't a changelog entry here. I am having a hard time generating a changelog from multiple git commits, and am not able to easily rebase now that I have merged from master. Any tips for handling this? I basically want to generate a changelog based on 3 or so commits, separated by multiple merged commits if that makes sense. Here is the output when I try to generate the changelog (my last commit was reverting the latest changelog): Tools/Scripts/prepare-ChangeLog --git-commit HEAD --bug 71262 Running status to find changed, added, or removed files. No changes found.
(In reply to comment #20) > Created an attachment (id=116686) [details] > Updating whitespace / webkit style > > This handles most of the feedback from my latest patch. I realize there isn't a changelog entry here. I am having a hard time generating a changelog from multiple git commits, and am not able to easily rebase now that I have merged from master. Any tips for handling this? I basically want to generate a changelog based on 3 or so commits, separated by multiple merged commits if that makes sense. Here is the output when I try to generate the changelog (my last commit was reverting the latest changelog): > > Tools/Scripts/prepare-ChangeLog --git-commit HEAD --bug 71262 > Running status to find changed, added, or removed files. > No changes found. Well, I usually do "git rebase master" instead of "git merge master", since rebasing is what you most likely want to do, that is, apply your current changes on top of the current master state, instead of the reverse (applying your latest master on top of you changes). Actually, "git merge" is intended for the opposite task, according to "git help merge": apply your development branch changes to the master branch (since it modifies the commits from the branch merged). In situations like yours (branch goofed up), I usually run "git diff master > work.patch", branch off of master (e.g. git checkout -b topic master) and apply work.patch against the "topic" working copy: patch -p1 -F3 < work.patch (-F3 handles ChangeLog merges if you are having problems with them, but may ruin your source code merges if the code you have changed also got changed in "master" - use with care.) Then commit and "git show > colorpicker.patch" (you have the only commit after "master" in your "topic" branch now.)
Comment on attachment 116686 [details] Updating whitespace / webkit style View in context: https://bugs.webkit.org/attachment.cgi?id=116686&action=review Thanks for addressing the comments, this is getting much closer to the webkit-ish code! Since I'm not a reviewer, we'll need some of those (pfeldman@chromium.org and yurys@chromium.org are the closest ones) to run the final check some time soon. > Source/WebCore/inspector/front-end/Color.js:46 > +WebInspector.Color.fromRGB = function(r, g, b) blank line above > Source/WebCore/inspector/front-end/Popover.js:52 > + if (hideOverflow) { No braces necessary for one-line if-blocks > Source/WebCore/inspector/front-end/Popover.js:-163 > - Inadvertent change > Source/WebCore/inspector/front-end/Spectrum.js:29 > +WebInspector.Spectrum = function(container) Annotating your new object for the Closure compiler is a good thing to do once you have fixed other issues (see other files in the "front-end" directory and http://code.google.com/closure/compiler/docs/js-for-compiler.html) > Source/WebCore/inspector/front-end/Spectrum.js:59 > + swatchElement.createChild("span", "swatch-inner"); Please combine the two lines into one (querySelectorAll is generally expensive and is unnecessary here): this._swatchInnerElement = swatchElement.createChild("span", "swatch-inner"); > Source/WebCore/inspector/front-end/Spectrum.js:142 > + s = max === 0 ? 0 : d / max; Is the case of "-0" possible here? Also, a better version is "s = max ? d / max : 0" since comparisons to NULL, false, and 0 are disapproved and allowed only to resolve ambiguities (http://www.webkit.org/coding/coding-style.html, "Null, false and 0"). > Source/WebCore/inspector/front-end/Spectrum.js:190 > + onmove.apply(element, [dragX, dragY]); It is not a good idea to pass an arbitrary "this" to a client callback, since it may be a bound function having its own "this". Sorry for overlooking this. A better option is just "onmove(element, [dragX, dragY]);". > Source/WebCore/inspector/front-end/Spectrum.js:196 > + var rightClick = (e.which) ? (e.which === 3) : (e.button === 2); The parentheses around e.which are not required. > Source/WebCore/inspector/front-end/Spectrum.js:201 > + onstart.apply(element, arguments) A better option is just "onstart(element, e);". > Source/WebCore/inspector/front-end/Spectrum.js:219 > + function stop() You should declare the Event ("e") argument... > Source/WebCore/inspector/front-end/Spectrum.js:228 > + onstop.apply(element, arguments); ...and use it as "onstop(element, e);". > Source/WebCore/inspector/front-end/Spectrum.js:251 > + var round = [Math.round(rgb[0]), Math.round(rgb[1]), Math.round(rgb[2]), rgb[3]]; Should hsvToRGB handle the rounding as well? I can't think of an RGB color using floats as its components. > Source/WebCore/inspector/front-end/Spectrum.js:254 > + return WebInspector.Color.fromRGB.apply(this, round); Why do you need "apply" if this is a "static" method? Just "return WebInspector.Color.fromRGB(round);" would do here. > Source/WebCore/inspector/front-end/Spectrum.js:256 > + return WebInspector.Color.fromRGBA.apply(this, round); ditto > Source/WebCore/inspector/front-end/Spectrum.js:264 > + return WebInspector.Color.fromRGBA.apply(this, round); ditto > Source/WebCore/inspector/front-end/Spectrum.js:302 > + extra blank line > Source/WebCore/inspector/front-end/Spectrum.js:352 > + extra blank line > Source/WebCore/inspector/front-end/Spectrum.js:364 > + ditto > Source/WebCore/inspector/front-end/StylesSidebarPane.js:-1584 > - Inadvertent change > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1650 > + swatchInnerElement.style.setProperty("background-color", text); Web Inspector typically uses ...style.backgroundColor = text; > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1660 > + swatchInnerElement.style.setProperty("background-color", colorString); ditto > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1671 > + extra blank line > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1677 > + if (e.altKey) { No braces necessary for if (e.altKey). We also tend to check for other modifiers not being active. > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1760 > + Inadvertent change > Source/WebCore/inspector/front-end/inspector.css:2708 > + top:0; spaces after colons > Source/WebCore/inspector/front-end/inspector.css:2753 > + border: solid black 3px; a typical format in the inspector CSS is "3px solid #000" (historically, there were only rgb's, so it would have been ...rgb (0, 0, 0))
(In reply to comment #22) > > Source/WebCore/inspector/front-end/inspector.css:2753 > > + border: solid black 3px; > > a typical format in the inspector CSS is "3px solid #000" (historically, there were only rgb's, so it would have been ...rgb (0, 0, 0)) We prefer using keywords for black/white.
Thanks for replying to this. I am already on a topic branch, not sure if that was clear or not. I understand what you are saying about making a new branch and applying a patch to get a fresh state - I'll do that along with the other changes for the next patch. Being new to this project, I want to make sure I am doing this right. I have been looking through all the docs I can find on contributing, but still have a couple questions about this (maybe I am just missing something): 1) Why can't I generate the changelog from multiple commits? It seems silly that you have to squash all your commits into one for a changelog, since you don't have to do that for a patch. I like to do small commits so I can see related changes together, and rebasing them together seems like counterproductive work to me. Is there a way to just generate one from a patch? 2) When you do a branch like this, do you rebase into master every time you want to generate a patch? What if you need to make changes to the patch based on feedback - do you revert the rebased commit, then rebase again, then generate the ChangeLog again, then do a patch? What if I have updated from origin/master in the meantime and had to resolve conflicts - do you have to do that every time also? Maybe I am just misunderstanding what you meant by this. (In reply to comment #21) > (In reply to comment #20) > > Created an attachment (id=116686) [details] [details] > > Updating whitespace / webkit style > > > > This handles most of the feedback from my latest patch. I realize there isn't a changelog entry here. I am having a hard time generating a changelog from multiple git commits, and am not able to easily rebase now that I have merged from master. Any tips for handling this? I basically want to generate a changelog based on 3 or so commits, separated by multiple merged commits if that makes sense. Here is the output when I try to generate the changelog (my last commit was reverting the latest changelog): > > > > Tools/Scripts/prepare-ChangeLog --git-commit HEAD --bug 71262 > > Running status to find changed, added, or removed files. > > No changes found. > > Well, I usually do "git rebase master" instead of "git merge master", since rebasing is what you most likely want to do, that is, apply your current changes on top of the current master state, instead of the reverse (applying your latest master on top of you changes). Actually, "git merge" is intended for the opposite task, according to "git help merge": apply your development branch changes to the master branch (since it modifies the commits from the branch merged). > > In situations like yours (branch goofed up), I usually run "git diff master > work.patch", branch off of master (e.g. git checkout -b topic master) and apply work.patch against the "topic" working copy: patch -p1 -F3 < work.patch (-F3 handles ChangeLog merges if you are having problems with them, but may ruin your source code merges if the code you have changed also got changed in "master" - use with care.) Then commit and "git show > colorpicker.patch" (you have the only commit after "master" in your "topic" branch now.)
(In reply to comment #23) > (In reply to comment #22) > > > Source/WebCore/inspector/front-end/inspector.css:2753 > > > + border: solid black 3px; > > > > a typical format in the inspector CSS is "3px solid #000" (historically, there were only rgb's, so it would have been ...rgb (0, 0, 0)) > > We prefer using keywords for black/white. Alright, I will use black/white for now unless if I hear otherwise.
(In reply to comment #24) > Thanks for replying to this. I am already on a topic branch, not sure if that was clear or not. I understand what you are saying about making a new branch and applying a patch to get a fresh state - I'll do that along with the other changes for the next patch. > > Being new to this project, I want to make sure I am doing this right. I have been looking through all the docs I can find on contributing, but still have a couple questions about this (maybe I am just missing something): > > 1) Why can't I generate the changelog from multiple commits? It seems silly that you have to squash all your commits into one for a changelog, since you don't have to do that for a patch. I like to do small commits so I can see related changes together, and rebasing them together seems like counterproductive work to me. Is there a way to just generate one from a patch? I believe it's a limitation of prepare-ChangeLog - it was originally written for svn (which is the VCS for webkit). Eric Seidel is the webkit tool master who usually handles this kind of issues. > 2) When you do a branch like this, do you rebase into master every time you want to generate a patch? What if you need to make changes to the patch based on feedback - do you revert the rebased commit, then rebase again, then generate the ChangeLog again, then do a patch? What if I have updated from origin/master in the meantime and had to resolve conflicts - do you have to do that every time also? Maybe I am just misunderstanding what you meant by this. My workflow is close to the following: Pull in the latest changes from the repo: [master] git pull git svn rebase git checkout topic [topic] git rebase master ...edit... git commit / git commit --amend (I keep no more than one commit in the topic branch for the sake of prepare-ChangeLog) (optionally goto begin) Upload a patch: Tools/Scripts/prepare-ChangeLog -g HEAD --bug=<bug_number> ...edit ChangeLogs... git commit -a --amend Tools/Scripts/webkit-patch upload Hope this helps. And sorry for the confusion about the colors: I don't remember when I used keywordish colors last :) > (In reply to comment #21) > > (In reply to comment #20) > > > Created an attachment (id=116686) [details] [details] [details] > > > Updating whitespace / webkit style > > > > > > This handles most of the feedback from my latest patch. I realize there isn't a changelog entry here. I am having a hard time generating a changelog from multiple git commits, and am not able to easily rebase now that I have merged from master. Any tips for handling this? I basically want to generate a changelog based on 3 or so commits, separated by multiple merged commits if that makes sense. Here is the output when I try to generate the changelog (my last commit was reverting the latest changelog): > > > > > > Tools/Scripts/prepare-ChangeLog --git-commit HEAD --bug 71262 > > > Running status to find changed, added, or removed files. > > > No changes found. > > > > Well, I usually do "git rebase master" instead of "git merge master", since rebasing is what you most likely want to do, that is, apply your current changes on top of the current master state, instead of the reverse (applying your latest master on top of you changes). Actually, "git merge" is intended for the opposite task, according to "git help merge": apply your development branch changes to the master branch (since it modifies the commits from the branch merged). > > > > In situations like yours (branch goofed up), I usually run "git diff master > work.patch", branch off of master (e.g. git checkout -b topic master) and apply work.patch against the "topic" working copy: patch -p1 -F3 < work.patch (-F3 handles ChangeLog merges if you are having problems with them, but may ruin your source code merges if the code you have changed also got changed in "master" - use with care.) Then commit and "git show > colorpicker.patch" (you have the only commit after "master" in your "topic" branch now.)
> > My workflow is close to the following: > > Pull in the latest changes from the repo: > [master] > git pull > git svn rebase > git checkout topic > > [topic] > git rebase master > ...edit... > git commit / git commit --amend (I keep no more than one commit in the topic branch for the sake of prepare-ChangeLog) > (optionally goto begin) > > Upload a patch: > Tools/Scripts/prepare-ChangeLog -g HEAD --bug=<bug_number> > ...edit ChangeLogs... > git commit -a --amend > Tools/Scripts/webkit-patch upload > > Hope this helps. Thanks! This workflow makes a lot more sense. Would love to see this documented somewhere, since it would have saved some time (and errant patches) getting into the project. Anyway, I have a patch ready with *most* of the issues you have brought up taken care of. I can work on adding the closure compiler annotations, etc while making any changes requested by a reviewer.
Created attachment 117092 [details] Latest patch cleaned up based on feedback Web Inspector: Add colorpicker functionality to color swatches in Styles Sidebar This makes it easier to pick styles, and opens up some possibilities for neat color based features in the future (for example: selecting from a 'pallet' of colors found in the CSS). https://bugs.webkit.org/show_bug.cgi?id=71262
Comment on attachment 117092 [details] Latest patch cleaned up based on feedback View in context: https://bugs.webkit.org/attachment.cgi?id=117092&action=review Overall the code looks good, I'll defer it to a real reviewer to comment on more subtle issues. > Source/WebCore/inspector/front-end/Spectrum.js:49 > + alphaLabel.textContent = WebInspector.UIString("alpha") + ": "; add to English.lproj/localizedStrings.js > Source/WebCore/inspector/front-end/Spectrum.js:64 > + colorLabel.textContent = WebInspector.UIString("color") + ": "; add to English.lproj/localizedStrings.js > Source/WebCore/inspector/front-end/Spectrum.js:359 > + extra blank line > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1644 > + swatchElement.title = WebInspector.UIString("Click to open a colorpicker"); add to English.lproj/localizedStrings.js > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1677 > + if (e.altKey) { Remove braces for the one-line block. > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1695 > + var format; optional: While we are here, you can remove this var altogether and simply return the required value from every alternative (that way you'll have to remove "else" in "else if", so that they will look like if (.....) return cf.something; if (.....) return cf.someOtherThing; ... > Source/WebCore/inspector/front-end/inspector.css:2681 > + position: relative; bad indentation for the 3 properties. Make sure you always use 4 spaces, not [a mixture of spaces and] tabs > Source/WebCore/inspector/front-end/inspector.css:2750 > + border-radius: 5px; bad indentation for all properties in the rule
Created attachment 119233 [details] Patch to provide colorpicker functionality inside of the styles sidebar for the web inspector frontend Latest patch, ready for review.
Comment on attachment 119233 [details] Patch to provide colorpicker functionality inside of the styles sidebar for the web inspector frontend View in context: https://bugs.webkit.org/attachment.cgi?id=119233&action=review Thanks for working on this. It looks like we are almost ready to get it landed. I'd suggest that we do it in steps: 1) land utility changes 2) land the Spectrum component 3) start using it behind the flag Steps (1) and (3) need separate bugs filed. > Source/WebCore/inspector/front-end/Popover.js:35 > +WebInspector.Popover = function(popoverHelper, appendElement, hideOverflow) appendElement does not looks like a good name to me and I don't think it belongs to the popover constructor. I'd call it parentElement and would pass it into show method. More importantly, you should land this as a separate change since it seems universal. > Source/WebCore/inspector/front-end/Spectrum.js:95 > + this.hideProxy = this.hide.bind(this); This seems to be private to this js file -> rename to this._hideProxy. > Source/WebCore/inspector/front-end/Spectrum.js:98 > +WebInspector.Spectrum.hsvToRGB = function(h, s, v, a) { { should be on the new line > Source/WebCore/inspector/front-end/Spectrum.js:131 > +WebInspector.Spectrum.rgbToHSV = function(r, g, b, a) { ditto > Source/WebCore/inspector/front-end/Spectrum.js:146 > + else { should be } else { > Source/WebCore/inspector/front-end/Spectrum.js:163 > +WebInspector.Spectrum.draggable = function(element, onmove, onstart, onstop) { I think I was suggesting to use WebInspector.elementDragStart earlier. I think I missed your comment on that, did it prove not to be applicable to your case? > Source/WebCore/inspector/front-end/Spectrum.js:276 > + _updateHelperLocations: function() { { on the new line > Source/WebCore/inspector/front-end/Spectrum.js:288 > + ); We don't fomat closing ) like this, I'd do: dragX = Math.max(-this._dragHelperElementHeight, Math.min(this.dragWidth - this._dragHelperElementHeight, dragX - this._dragHelperElementHeight)); > Source/WebCore/inspector/front-end/Spectrum.js:303 > + _updateUI: function() { { on the new line > Source/WebCore/inspector/front-end/Spectrum.js:322 > + toggle: function(element, color) { ditto > Source/WebCore/inspector/front-end/Spectrum.js:331 > + show: function(element, color) { ditto > Source/WebCore/inspector/front-end/Spectrum.js:351 > + hide: function() { ditto > Source/WebCore/inspector/front-end/StylesSidebarPane.js:92 > + this._spectrum = new WebInspector.Spectrum(this.bodyElement); I think you should land the spectrum first and then enable it in a separate change. You could also hide its use behind the Preferences.useSpectrum flag first (see Settings.js). > Source/WebCore/inspector/front-end/inspector.css:2668 > +.spectrum-container { fyi, no need to fix now: we might want to load this CSS lazily via using the View infrastructure. > Source/WebCore/inspector/front-end/utilities.js:284 > +Element.prototype.totalOffset = function(parent) Changes to the Color, Element prototype could also land as a separate change. Do you mind filing separate bugs and extracting the patches for them? I would allow us landing the spectrum faster. > Source/WebCore/inspector/front-end/utilities.js:902 > +function stopPropagation(e) { ditto, should be landed along with the element prototype changes.
(In reply to comment #31) > Thanks for working on this. It looks like we are almost ready to get it landed. I'd suggest that we do it in steps: > 1) land utility changes > 2) land the Spectrum component > 3) start using it behind the flag > Steps (1) and (3) need separate bugs filed. I have opened a bug for item (1) here: https://bugs.webkit.org/show_bug.cgi?id=75454. I will attach a patch for (2 / 3) in this case, with a flag in Settings.js. > I think I was suggesting to use WebInspector.elementDragStart earlier. I think I missed your comment on that, did it prove not to be applicable to your case? I tried to make it work and didn't have much success right away. I'm sure it could be made to work, but since the current implementation seemed to work fine I just left it as is.
Created attachment 120904 [details] Add colorpicker functionality to color swatches in Styles Sidebar. Depends on: https://bugs.webkit.org/show_bug.cgi?id=75454
Comment on attachment 120904 [details] Add colorpicker functionality to color swatches in Styles Sidebar. View in context: https://bugs.webkit.org/attachment.cgi?id=120904&action=review This looks mostly Ok. Let me apply it to see if it behaves consistently with the rest of the app. > Source/WebCore/inspector/front-end/Spectrum.js:31 > + this._popover = new WebInspector.Popover(null, true); First parameter is marked as optional, not nullable, so you should pass "undefined" instead. > Source/WebCore/inspector/front-end/Spectrum.js:99 > +WebInspector.Spectrum.hsvToRGB = function(h, s, v, a) WebInspector.Spectrum.hsvaToRGBA ? > Source/WebCore/inspector/front-end/Spectrum.js:133 > +WebInspector.Spectrum.rgbToHSV = function(r, g, b, a) ditto > Source/WebCore/inspector/front-end/Spectrum.js:165 > +WebInspector.Spectrum.draggable = function(element, onmove, onstart, onstop) { I still think it is worth extra effort to make it work with WebInspector.elementDragStart. I am fine with landing this method for now since you change is up for review for so long. Please add //FIXME: migrate to WebInspector.elementDragStart above this line. > Source/WebCore/inspector/front-end/Spectrum.js:254 > + return WebInspector.Color.fromRGB(round[0], round[1], round[2], round[3]); WebInspector.Color.fromRGB receives only 3 arguments. > Source/WebCore/inspector/front-end/Spectrum.js:261 > + var rgb = WebInspector.Spectrum.hsvToRGB(this.hsv[0], 1, 1); WebInspector.Spectrum.hsvToRGB expects 4 parameters. > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1609 > + var spectrum = hasColorpicker && self._parentPane._spectrum; Prefer hasColorpicker ? self._parentPane._spectrum : null
Couple of things I noticed at runtime: 1. Popover positioning is broken: - Console is drawn above the popover - When there is insufficient amount of space, popover is still drawn either above or below the anchor, while it should be drawn to the right (or even over) the anchor to prevent the box clipping. 2. Color picker does not respect the chosen color format: when user chooses hsla, changing the value in the picker resets it back to rgb.
Brian, we haven't heard from you for a while on this issue. Do you have an intention to continue your work on the feature or should we fix the problems in your patch and land it ourselves? In the latter case, would you give your permission for us to do so, naturally sharing the credit with you in the ChangeLog?
I actually got a chance to work on it more this week after being off of it for a couple. I changed the popover positioning (which actually means there are no changes to Popover.js anymore). In fact, I almost resubmitted the patch this weekend but did because of these two main issues left: 1. Now that the popover can overflow, when you scroll down to the bottom then try to drag around the picker, the dragging doesn't work (the 'y' value is off because of the scroll). 2. "Color picker does not respect the chosen color format: when user chooses hsla, changing the value in the picker resets it back to rgb". This issue is actually pretty tricky to fix because of the way WebInspector.Color and the sidebar pane function together. I am still in the debugging stage of this problem. I can work out at least item 1 and resubmit it. Maybe one of you would have better luck with 2? I have tried a few different things as far as passing in the original format to WebInspector.Color.toString() but nothing seems to work just right. (In reply to comment #36) > Brian, we haven't heard from you for a while on this issue. Do you have an intention to continue your work on the feature or should we fix the problems in your patch and land it ourselves? In the latter case, would you give your permission for us to do so, naturally sharing the credit with you in the ChangeLog?
Created attachment 125058 [details] Add colorpicker functionality to color swatches in Styles Sidebar - Using native popover and keeping original color format Reverted popover changes, incorporating other feedback from comments. Should now be keeping the initial color format when possible. NOTE: I know when you alt+click to switch formats it works while dragging but reverts back to the old format when finished. I can't figure out exactly why this is happening (something to do with a major applyStyleText). I went ahead and sent it through even with this issue b/c it is relatively minor and just to keep the dialog going (since there seems to be UI feedback still coming through).
Comment on attachment 125058 [details] Add colorpicker functionality to color swatches in Styles Sidebar - Using native popover and keeping original color format View in context: https://bugs.webkit.org/attachment.cgi?id=125058&action=review Make sure all strings you pass into WebInspector.UIString(..) are present in Source/WebCore/English.lproj/localizedStrings.js (it is in UTF-16). > Source/WebCore/ChangeLog:1 > +2012-02-01 bgrins <briangrinstead@gmail.com> A real name will look nice here (http://trac.webkit.org/wiki/UsingGitWithWebKit#WebKitScriptsupportforGit has a good description of the git setup process for WebKit development) > Source/WebCore/inspector/front-end/Spectrum.js:98 > +WebInspector.Spectrum.hsvToRGBA = function(h, s, v, a) hsvaToRGBA? > Source/WebCore/inspector/front-end/Spectrum.js:132 > +WebInspector.Spectrum.rgbToHSVA = function(r, g, b, a) rgbaToHSVA? > Source/WebCore/inspector/front-end/Spectrum.js:138 > + var max = Math.max(r, g, b), min = Math.min(r, g, b); One variable per line > Source/WebCore/inspector/front-end/Spectrum.js:254 > + if (rgb[3] === 1) { Please remove braces around single-line blocks > Source/WebCore/inspector/front-end/Spectrum.js:257 > + else { ditto > Source/WebCore/inspector/front-end/Spectrum.js:291 > + var round = [Math.round(rgb[0]), Math.round(rgb[1]), Math.round(rgb[2]), rgb[3]]; Rounding of the RGB values can be extracted into a separate method, since you do exactly the same in the "get color()" above. > Source/WebCore/inspector/front-end/Spectrum.js:306 > + a lonely extra blank line :) > Source/WebCore/inspector/front-end/Spectrum.js:398 > + this.dispatchEventToListeners("hide", this.color); Typically we define something along the lines of WebInspector.Spectrum.Events = { ColorChanged: "ColorChanged", Hidden: "Hidden" } (using the participle instead of infinitive) - see CSSStyleModel.js for an example. Also, the Hidden event doesn't seem to need the color as the payload, since the updated color has been dispatched earlier in ColorChanged (and it's a good idea to dispatch the default initial color when the picker has just been opened.) > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1644 > + self.applyStyleText(nameElement.textContent + ": " + valueElement.textContent, true, true, false); The user seems to be unable to cancel colorpicking (e.g. using Esc)? > Source/WebCore/inspector/front-end/inspector.css:2690 > +.spectrum-sat, .spectrum-val, .spectrum-top-inner { Instead of these separate classes, you can use the .fill class on respective elements, defined at the top of inspector.css. This will at least slightly help the size of our giant CSS.
(In reply to comment #39) > (From update of attachment 125058 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125058&action=review > > Make sure all strings you pass into WebInspector.UIString(..) are present in Source/WebCore/English.lproj/localizedStrings.js (it is in UTF-16). Done > > Source/WebCore/ChangeLog:1 > > +2012-02-01 bgrins <briangrinstead@gmail.com> > > A real name will look nice here (http://trac.webkit.org/wiki/UsingGitWithWebKit#WebKitScriptsupportforGit has a good description of the git setup process for WebKit development) Done > > Source/WebCore/inspector/front-end/Spectrum.js:98 > > +WebInspector.Spectrum.hsvToRGBA = function(h, s, v, a) > > hsvaToRGBA? > > > Source/WebCore/inspector/front-end/Spectrum.js:132 > > +WebInspector.Spectrum.rgbToHSVA = function(r, g, b, a) > > rgbaToHSVA? > Done > > Source/WebCore/inspector/front-end/Spectrum.js:138 > > + var max = Math.max(r, g, b), min = Math.min(r, g, b); > > One variable per line > > > Source/WebCore/inspector/front-end/Spectrum.js:254 > > + if (rgb[3] === 1) { > Done > Please remove braces around single-line blocks > > > Source/WebCore/inspector/front-end/Spectrum.js:257 > > + else { > > ditto > Done > > Source/WebCore/inspector/front-end/Spectrum.js:291 > > + var round = [Math.round(rgb[0]), Math.round(rgb[1]), Math.round(rgb[2]), rgb[3]]; > > Rounding of the RGB values can be extracted into a separate method, since you do exactly the same in the "get color()" above. > Moved to Spectrum.hsvaToRGBA > > Source/WebCore/inspector/front-end/Spectrum.js:306 > > + > > a lonely extra blank line :) > Ugh... finally fixed the setting on my text editor that was causing all of those problems with indenting new lines. This lonely one was just from user error. > > Source/WebCore/inspector/front-end/Spectrum.js:398 > > + this.dispatchEventToListeners("hide", this.color); > > Typically we define something along the lines of > > WebInspector.Spectrum.Events = { > ColorChanged: "ColorChanged", > Hidden: "Hidden" > } > > (using the participle instead of infinitive) - see CSSStyleModel.js for an example. > > Also, the Hidden event doesn't seem to need the color as the payload, since the updated color has been dispatched earlier in ColorChanged (and it's a good idea to dispatch the default initial color when the picker has just been opened.) > Done > > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1644 > > + self.applyStyleText(nameElement.textContent + ": " + valueElement.textContent, true, true, false); > > The user seems to be unable to cancel colorpicking (e.g. using Esc)? > Should 'escape' close the picker in the current state or revert back to the original color from when it opened? > > Source/WebCore/inspector/front-end/inspector.css:2690 > > +.spectrum-sat, .spectrum-val, .spectrum-top-inner { > > Instead of these separate classes, you can use the .fill class on respective elements, defined at the top of inspector.css. This will at least slightly help the size of our giant CSS. Ah, good catch... fixed that too.
Created attachment 126438 [details] Patch to provide colorpicker functionality inside of the styles sidebar for the web inspector frontend Patch based on feedback from Alexander Pavlov
Comment on attachment 126438 [details] Patch to provide colorpicker functionality inside of the styles sidebar for the web inspector frontend View in context: https://bugs.webkit.org/attachment.cgi?id=126438&action=review The patch looks good in general. My only concern is that you cannot cancel the colorpicker (without applying the selected color to the current property.) Please fix the nits and let us play around with it to see if there are any critical issues that should definitely be fixed before landing. > Source/WebCore/inspector/front-end/Spectrum.js:282 > + // Everything except HSL(A) should returned as RGBA if transparency is involved. typo: ...should be returned... > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1667 > + spectrum.addEventListener(WebInspector.Spectrum.Events.ColorChanged, spectrumChange); Shouldn't we remove the listeners when the spectrum gets hidden?
Comment on attachment 126438 [details] Patch to provide colorpicker functionality inside of the styles sidebar for the web inspector frontend View in context: https://bugs.webkit.org/attachment.cgi?id=126438&action=review We'd really appreciate it if you could remove a number of trailing spaces in inspector.css, Spectrum.js, and StylesSidebarPane.js (perhaps using one of the tools available out there or just by a regex replacement in Eclipse.) > Source/WebCore/WebCore.vcproj/WebCore.vcproj:72076 > + RelativePath="..\inspector\front-end\Spectrum.js" Please also rebase your patch on the current tip of tree, this merged poorly with "patch -F3" for me, so this won't land properly with the commit-queue.
Runtime thoughts :) First and foremost: the picker will be a magnitude more usable if clicking (mousedown) in the two colored areas will move the dragger/slider to the click point, like the alpha slider (<input type="range">) does. This style can make the dragger look a lot better in dark areas of the spectrum: .spectrum-dragger { border-radius: 5px; height: 5px; width: 5px; border: 1px solid white; cursor: pointer; position: absolute; top: 0; left: 0; background: black; } Also "-webkit-user-select: none" would be useful on "spectrum-container", since click-dragging the alpha slider may inadvertently result in selecting the "alpha: " label (or some other text nearby) if you miss the slider active area (which was the case with me.) The strings "alpha: ", "color: ", and "Click to open a colorpicker" are not present in localizedStrings.js (this file is considered binary by the VCS's, so you will need to manually re-add them every time it changes in the HEAD). It would be awesome if there were a way to cancel the colorpicking. Also, Pavel might have some concerns regarding the compatibility with his recent Undo feature (at the very least, we can deal with it in a subsequent patch.)
(In reply to comment #44) > Runtime thoughts :) > > First and foremost: the picker will be a magnitude more usable if clicking (mousedown) in the two colored areas will move the dragger/slider to the click point, like the alpha slider (<input type="range">) does. > Good point, added this in. > This style can make the dragger look a lot better in dark areas of the spectrum: > .spectrum-dragger { > border-radius: 5px; > height: 5px; > width: 5px; > border: 1px solid white; > cursor: pointer; > position: absolute; > top: 0; > left: 0; > background: black; > } > Done > Also "-webkit-user-select: none" would be useful on "spectrum-container", since click-dragging the alpha slider may inadvertently result in selecting the "alpha: " label (or some other text nearby) if you miss the slider active area (which was the case with me.) > Done > The strings "alpha: ", "color: ", and "Click to open a colorpicker" are not present in localizedStrings.js (this file is considered binary by the VCS's, so you will need to manually re-add them every time it changes in the HEAD). > When I change this file, I just see 'Binary files differ' in the patch... So I'm not sure how to include these changes in the patch. How do you typically get these changes landed? > It would be awesome if there were a way to cancel the colorpicking. Also, Pavel might have some concerns regarding the compatibility with his recent Undo feature (at the very least, we can deal with it in a subsequent patch.) I wonder if the 'cancel' functionality should be put in place with this? Not sure if we would have to change to code to make it compatible with undo. I don't have a problem adding in a cancel button or 'escape' support, but I want to make sure it would be accepted before putting the time into it.
(In reply to comment #43) > (From update of attachment 126438 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=126438&action=review > > We'd really appreciate it if you could remove a number of trailing spaces in inspector.css, Spectrum.js, and StylesSidebarPane.js (perhaps using one of the tools available out there or just by a regex replacement in Eclipse.) > Done > > Source/WebCore/WebCore.vcproj/WebCore.vcproj:72076 > > + RelativePath="..\inspector\front-end\Spectrum.js" > > Please also rebase your patch on the current tip of tree, this merged poorly with "patch -F3" for me, so this won't land properly with the commit-queue. Done
(In reply to comment #42) > (From update of attachment 126438 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=126438&action=review > > The patch looks good in general. My only concern is that you cannot cancel the colorpicker (without applying the selected color to the current property.) Please fix the nits and let us play around with it to see if there are any critical issues that should definitely be fixed before landing. > > > Source/WebCore/inspector/front-end/Spectrum.js:282 > > + // Everything except HSL(A) should returned as RGBA if transparency is involved. > > typo: ...should be returned... > Is it just me or are these the same string? returned returned There could be a typo in there, maybe I just have a blind eye to it :) > > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1667 > > + spectrum.addEventListener(WebInspector.Spectrum.Events.ColorChanged, spectrumChange); > > Shouldn't we remove the listeners when the spectrum gets hidden? Good point, done.
> > > Source/WebCore/inspector/front-end/Spectrum.js:282 > > > + // Everything except HSL(A) should returned as RGBA if transparency is involved. > > > > typo: ...should be returned... > > > > Is it just me or are these the same string? Haha, I finally see what you are talking about. Wow, that took a while for me to see for some reason.
Brian, we believe you have addressed the comments, but we cannot see the patch! :-)
Created attachment 126769 [details] Patch
Ah, I used webkit-patch upload and that fixed my issue with the binary diff on localizedStrings.js. Hopefully everything else went through correctly using this method.
Committed r107804: <http://trac.webkit.org/changeset/107804>
(In reply to comment #52) > Committed r107804: <http://trac.webkit.org/changeset/107804> Great, is there anything else I should do for this? Cancel, undo, and keeping in sync with alt+click changed formats have all been mentioned. I think cancel and undo are going to be pretty closely tied together, and I don't yet know anything about the undo feature but I'm willing to follow this patch up with whatever changes are needed.
(In reply to comment #53) > (In reply to comment #52) > > Committed r107804: <http://trac.webkit.org/changeset/107804> > > Great, is there anything else I should do for this? Cancel, undo, and keeping in sync with alt+click changed formats have all been mentioned. I think cancel and undo are going to be pretty closely tied together, and I don't yet know anything about the undo feature but I'm willing to follow this patch up with whatever changes are needed. I just landed a followup http://trac.webkit.org/changeset/107810 that deals with the compilability and our best practices, but canceling the color selection is a most welcome feature! Pavel also wanted that hundred-line third-level spectrum-related chunk inside StylesSidebarPane#processColor() to be extracted to the top level.