Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2230183004:
DevTools: Add shadow-editor swatch/icon before box-shadows and text-shadows (Closed)
|
Created:
4 years, 4 months ago by flandy Modified:
4 years, 3 months ago CC:
chromium-reviews, caseq+blink_chromium.org, blink-reviews-style_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Add shadow-editor swatch/icon before box-shadows and text-shadows
This is the first step for the shadow editor and is hidden under an experiment.
BUG=639095
Committed: https://crrev.com/ea253f5b0f251a0942a6cd181559a1ce0683fc75
Cr-Commit-Position: refs/heads/master@{#413927}
Patch Set 1 #
Total comments: 16
Patch Set 2 : New image swatches.png #
Total comments: 4
Patch Set 3 : Use smallIcons and shadow model #
Total comments: 18
Patch Set 4 : Update model #
Total comments: 22
Patch Set 5 : Move model to another patch #
Total comments: 9
Patch Set 6 : Address comments #Patch Set 7 : Add TODO #Messages
Total messages: 24 (6 generated)
flandy@google.com changed reviewers: + dgozman@chromium.org, lushnikov@chromium.org
Please take an initial look. Still need to update the icons so that the "s" shows through.
https://codereview.chromium.org/2230183004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js (right): https://codereview.chromium.org/2230183004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:2037: var results = WebInspector.TextUtils.splitStringByRegexes(propertyValue, [WebInspector.Color.Regex, /,/g]); we should put this logic somewhere else - we might want to reuse it in sources panel https://codereview.chromium.org/2230183004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:2224: propertyRenderer.setColorHandler(this._processColorUneditable.bind(this)); ideally, we should show bezier and shadow swatches for read-only rules as well; they just should not be editable. This is good thing to address in a follow-up https://codereview.chromium.org/2230183004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:3068: valueElement.appendChild(this._shadowHandler(this._propertyValue, this._propertyName)); does this mean there could be only one shadow swatch in value? https://codereview.chromium.org/2230183004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/colorSwatch.css (right): https://codereview.chromium.org/2230183004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/colorSwatch.css:39: .shadow-swatch { colorSwatch.css should be only about ColorSwatch.js web component; let's not add unrelated things here
https://codereview.chromium.org/2230183004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/elements/ColorSwatchPopoverIcon.js (right): https://codereview.chromium.org/2230183004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/ColorSwatchPopoverIcon.js:45: path.setAttribute("d", "M2,8 C2,3 8,7 8,2"); We can do similar svg for shadows. https://codereview.chromium.org/2230183004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/ColorSwatchPopoverIcon.js:258: WebInspector.ShadowPopoverIcon = function(treeElement, swatchPopoverHelper, shadowText, shadowType) Do you plan to use the same editor for box shadow and text shadow? https://codereview.chromium.org/2230183004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/ColorSwatchPopoverIcon.js:298: this._colorSwatchPopoverIcon = new WebInspector.ColorSwatchPopoverIcon(this._treeElement, this._swatchPopoverHelper, result.value); I thought this will be handled outside of shadow-related code. Andrey? https://codereview.chromium.org/2230183004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/colorSwatch.css (right): https://codereview.chromium.org/2230183004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/colorSwatch.css:40: -webkit-mask-image: url(Images/toolbarButtonGlyphs.png); Let's not place random images in toolbarButtonGlyphs. Make a separate one.
Please take another look. Chris and I have added a new image asset for the swatches, and the "s" should now show through. https://codereview.chromium.org/2230183004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/elements/ColorSwatchPopoverIcon.js (right): https://codereview.chromium.org/2230183004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/ColorSwatchPopoverIcon.js:45: path.setAttribute("d", "M2,8 C2,3 8,7 8,2"); On 2016/08/10 22:03:45, dgozman wrote: > We can do similar svg for shadows. Let's use png instead. https://codereview.chromium.org/2230183004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/ColorSwatchPopoverIcon.js:258: WebInspector.ShadowPopoverIcon = function(treeElement, swatchPopoverHelper, shadowText, shadowType) On 2016/08/10 22:03:45, dgozman wrote: > Do you plan to use the same editor for box shadow and text shadow? Yes, as much as possible. https://codereview.chromium.org/2230183004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/ColorSwatchPopoverIcon.js:298: this._colorSwatchPopoverIcon = new WebInspector.ColorSwatchPopoverIcon(this._treeElement, this._swatchPopoverHelper, result.value); On 2016/08/10 22:03:44, dgozman wrote: > I thought this will be handled outside of shadow-related code. Andrey? It is usually handled in StylesSidebarPropertyRenderer.renderValue but I skip that and do separate handling for box/text shadows. The shadow handler should also know about the color, in case we want to add opacity controls to the shadow editor, for example. https://codereview.chromium.org/2230183004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js (right): https://codereview.chromium.org/2230183004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:2037: var results = WebInspector.TextUtils.splitStringByRegexes(propertyValue, [WebInspector.Color.Regex, /,/g]); On 2016/08/10 19:29:02, lushnikov wrote: > we should put this logic somewhere else - we might want to reuse it in sources > panel Done. https://codereview.chromium.org/2230183004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:2224: propertyRenderer.setColorHandler(this._processColorUneditable.bind(this)); On 2016/08/10 19:29:02, lushnikov wrote: > ideally, we should show bezier and shadow swatches for read-only rules as well; > they > just should not be editable. > > This is good thing to address in a follow-up Okay let's hold off on this for now. https://codereview.chromium.org/2230183004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:3068: valueElement.appendChild(this._shadowHandler(this._propertyValue, this._propertyName)); On 2016/08/10 19:29:02, lushnikov wrote: > does this mean there could be only one shadow swatch in value? No, value could have multiple shadow swatches if _propertyValue has multiple shadows separated by commas. Should these be plural, _shadowsHandler? _processShadows? https://codereview.chromium.org/2230183004/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/colorSwatch.css (right): https://codereview.chromium.org/2230183004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/colorSwatch.css:39: .shadow-swatch { On 2016/08/10 19:29:02, lushnikov wrote: > colorSwatch.css should be only about ColorSwatch.js web component; let's not add > unrelated things here Done. https://codereview.chromium.org/2230183004/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/colorSwatch.css:40: -webkit-mask-image: url(Images/toolbarButtonGlyphs.png); On 2016/08/10 22:03:45, dgozman wrote: > Let's not place random images in toolbarButtonGlyphs. Make a separate one. Done.
https://codereview.chromium.org/2230183004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/Images/src/swatches.svg (right): https://codereview.chromium.org/2230183004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/Images/src/swatches.svg:1: <?xml version="1.0" encoding="UTF-8" standalone="no"?> let's make a "smallIcons.svg" and put all the small icons in there (we should also extract them out from toolbarButtonGlyphs.svg, see smallIcons.css for the reference). https://codereview.chromium.org/2230183004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js (right): https://codereview.chromium.org/2230183004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:19: WebInspector.ShadowEditor.Shadow.splitShadows = function(text) I don't like this - it seems to be a utility method which will be utilized only in StylesSidebarPane. The WI.ShadowEditor should have a ShadowModel which it edits (see WI.Geometry.CubicBezier / WI.Color). Then you can introduce methods like WI.ShadowModel.parse(text) -> which returns an array of WI.ShadowModel objects. Once edited, every WI.ShadowModel has asCSSText() method which returns appropriate css text.
Please take another look. https://codereview.chromium.org/2230183004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/Images/src/swatches.svg (right): https://codereview.chromium.org/2230183004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/Images/src/swatches.svg:1: <?xml version="1.0" encoding="UTF-8" standalone="no"?> On 2016/08/11 01:40:07, lushnikov wrote: > let's make a "smallIcons.svg" and put all the small icons in there (we should > also extract them out from > toolbarButtonGlyphs.svg, see smallIcons.css for the reference). Done. https://codereview.chromium.org/2230183004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js (right): https://codereview.chromium.org/2230183004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:19: WebInspector.ShadowEditor.Shadow.splitShadows = function(text) On 2016/08/11 01:40:07, lushnikov wrote: > I don't like this - it seems to be a utility method which will be utilized only > in StylesSidebarPane. > > The WI.ShadowEditor should have a ShadowModel which it edits (see > WI.Geometry.CubicBezier / WI.Color). > > Then you can introduce methods like WI.ShadowModel.parse(text) -> which returns > an array of WI.ShadowModel objects. > > Once edited, every WI.ShadowModel has asCSSText() method which returns > appropriate css text. > Yes I was planning on adding much to the Shadow Model just in a later patch. I've gone ahead and introduced more to the shadow model now, which simplifies the ShadowPopoverIcon. I think it makes sense to return the css text in parts/chunks, so that we can add the color swatch in front of the color.
Also, as far as we add a parsing logic, let's cover it with a test! https://codereview.chromium.org/2230183004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ColorSwatchPopoverIcon.js (right): https://codereview.chromium.org/2230183004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ColorSwatchPopoverIcon.js:276: this._element.removeChildren(); let's not do this. Instead, we should create a real shadowSwatch custom component, like the WI.ColorSwatch: WI.CSSShadowSwatch.prototype = { colorSwatch: function() { .. } setCSSShadow: function(cssShadowModel) ... } With the colorSwatch exposed, you would be able to bind clicks. https://codereview.chromium.org/2230183004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js (right): https://codereview.chromium.org/2230183004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:172: WebInspector.ShadowSwatch.create = function() Let's rename to be consistent with our future model name: WI.CSSShadowSwatch = https://codereview.chromium.org/2230183004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js (right): https://codereview.chromium.org/2230183004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:13: WebInspector.ShadowEditor.Shadow = function(type) Let's make this a real css shadow model: WebInspector.CSSShadowModel = function(inset, x, y, blurRadius, spreadRadius, color) { ... } WebInspector.CSSShadowModel.prototype = { setInset: ... setOffset: function(x, y) ... setBlueRadius: ... setSpreadRadius: ... setColor: ... setFormat: function(format) ... // format toCSSBoxShadow: function() ... toCSSTextShadow: function() ... } Where format is one of the permutations of letters "L", "I", "C": L - lengths I - inset C - color https://codereview.chromium.org/2230183004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:33: WebInspector.ShadowEditor.Shadow.parse = function(text, type) WI.CSSShadowModel.parseBoxShadow = function(text) ... https://codereview.chromium.org/2230183004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:98: addLength: function(length) this method should not belong to model; it's a part of model parsing process and should be inside parsing function https://codereview.chromium.org/2230183004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:159: WebInspector.ShadowEditor.Length = function(amount, unit) WI.CSSLength = function(...) https://codereview.chromium.org/2230183004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:166: WebInspector.ShadowEditor.Length.Regex = /([0-9]+)([a-zA-Z]{2,4})|0/g; could it be negative/fractional? https://codereview.chromium.org/2230183004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:169: WebInspector.ShadowEditor.Length._units = new Set([ WI.CSSLengthUnit = { Ch: "ch", Ex: "ex", ... None: "" } https://codereview.chromium.org/2230183004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:195: toString: function() let's do toCSSText()
Description was changed from ========== DevTools: Add shadow-editor swatch/icon before box-shadows and text-shadows This is the first step for the shadow editor and is hidden under an experiment. ========== to ========== DevTools: Add shadow-editor swatch/icon before box-shadows and text-shadows This is the first step for the shadow editor and is hidden under an experiment. BUG=639095 ==========
Please take another look :) https://codereview.chromium.org/2230183004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ColorSwatchPopoverIcon.js (right): https://codereview.chromium.org/2230183004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ColorSwatchPopoverIcon.js:276: this._element.removeChildren(); On 2016/08/16 22:53:19, lushnikov wrote: > let's not do this. Instead, we should create a real shadowSwatch custom > component, like the > WI.ColorSwatch: > > WI.CSSShadowSwatch.prototype = { > colorSwatch: function() { .. } > setCSSShadow: function(cssShadowModel) ... > } > > With the colorSwatch exposed, you would be able to bind clicks. Done. https://codereview.chromium.org/2230183004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js (right): https://codereview.chromium.org/2230183004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:172: WebInspector.ShadowSwatch.create = function() On 2016/08/16 22:53:20, lushnikov wrote: > Let's rename to be consistent with our future model name: > > WI.CSSShadowSwatch = Done. https://codereview.chromium.org/2230183004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js (right): https://codereview.chromium.org/2230183004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:13: WebInspector.ShadowEditor.Shadow = function(type) On 2016/08/16 22:53:20, lushnikov wrote: > Let's make this a real css shadow model: > > WebInspector.CSSShadowModel = function(inset, x, y, blurRadius, spreadRadius, > color) > { > ... > } > > WebInspector.CSSShadowModel.prototype = { > setInset: ... > setOffset: function(x, y) ... > setBlueRadius: ... > setSpreadRadius: ... > setColor: ... > setFormat: function(format) ... // format > toCSSBoxShadow: function() ... > toCSSTextShadow: function() ... > } > > Where format is one of the permutations of letters "L", "I", "C": > L - lengths > I - inset > C - color Done. I've opted to use more options for the format with "I", "X", "Y", "B", "S", and "C", and put these in an enum. https://codereview.chromium.org/2230183004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:33: WebInspector.ShadowEditor.Shadow.parse = function(text, type) On 2016/08/16 22:53:20, lushnikov wrote: > WI.CSSShadowModel.parseBoxShadow = function(text) ... Done. https://codereview.chromium.org/2230183004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:98: addLength: function(length) On 2016/08/16 22:53:20, lushnikov wrote: > this method should not belong to model; it's a part of model parsing process and > should be inside parsing function Done. https://codereview.chromium.org/2230183004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:159: WebInspector.ShadowEditor.Length = function(amount, unit) On 2016/08/16 22:53:20, lushnikov wrote: > WI.CSSLength = function(...) Done. https://codereview.chromium.org/2230183004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:166: WebInspector.ShadowEditor.Length.Regex = /([0-9]+)([a-zA-Z]{2,4})|0/g; On 2016/08/16 22:53:20, lushnikov wrote: > could it be negative/fractional? Yes, and also scientific notation. Fixed. https://codereview.chromium.org/2230183004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:169: WebInspector.ShadowEditor.Length._units = new Set([ On 2016/08/16 22:53:20, lushnikov wrote: > WI.CSSLengthUnit = { > Ch: "ch", > Ex: "ex", > ... > None: "" > } Done. https://codereview.chromium.org/2230183004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:195: toString: function() On 2016/08/16 22:53:20, lushnikov wrote: > let's do toCSSText() Renamed to asCSSText() to keep consistent with WI.Geometry.CubicBezier.
https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector/components/css-shadow-model-expected.txt (right): https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/components/css-shadow-model-expected.txt:4: 10px I like this test, good coverage! I'd suggest enhancing it a bit. Let's output: 1. source text which we attempt to parse 2. status if we failed or succeeded parsing 3. in case of success, asCSSText() https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector/components/css-shadow-model.html (right): https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/components/css-shadow-model.html:106: function _dumpShadow(shadows) only private class methods start with "_" in this case, let's just name this dumpShadow https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js (right): https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:12: * @param {boolean=} inset let's make non-optional https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:19: WebInspector.CSSShadowModel = function(inset, x, y, blurRadius, spreadRadius, color) This model is a good patch by itself. Let's extract it under common/CSSShadowModel.js and introduce it in a separate patch, together with covering test. https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:22: this._x = x || new WebInspector.CSSLength(0, WebInspector.CSSLengthUnit.None); let's introduce a convenience method: WebInspector.CSSLength.zero() https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:23: this._y = y || new WebInspector.CSSLength(0, WebInspector.CSSLengthUnit.None); let's name these and all related local variables as offsetX and offsetY to align with spec naming https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:27: this._format = ""; there should be some default format - otherwise, the asCSSText() would not work on just-created instance https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:44: * @return {?Array<!WebInspector.CSSShadowModel>} let's return !Array<..>, which would be empty in case of failure https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:254: * @param {string} unit WebInspector.CSSLengthUnit https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:263: WebInspector.CSSLength.Regex = /([+-]?(?:[0-9]*[.])?[0-9]+(?:[eE][+-]?[0-9]+)?)([a-zA-Z]{2,4})|[+-]?(?:0*[.])?0+(?:[eE][+-]?[0-9]+)?/g; can we build this complex regex out of composing parts? e.g. we can extract a number regex, and probably a CSSLengthUnit regex? Otherwise it's hard to follow what are your matching groups Also, CSSLengthUnit regex could be probably generated out of WI.CSSLengthUnit enum https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:306: function validUnit(unit) this can go away if the CSSLengthUnit regex would be generated out of CSSLengthUnit enum
All done. https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector/components/css-shadow-model-expected.txt (right): https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/components/css-shadow-model-expected.txt:4: 10px On 2016/08/19 16:24:34, lushnikov wrote: > I like this test, good coverage! > > I'd suggest enhancing it a bit. > Let's output: > 1. source text which we attempt to parse > 2. status if we failed or succeeded parsing > 3. in case of success, asCSSText() Done. https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector/components/css-shadow-model.html (right): https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/components/css-shadow-model.html:106: function _dumpShadow(shadows) On 2016/08/19 16:24:35, lushnikov wrote: > only private class methods start with "_" > > in this case, let's just name this dumpShadow Done. https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js (right): https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:12: * @param {boolean=} inset On 2016/08/19 16:24:35, lushnikov wrote: > let's make non-optional Done. https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:19: WebInspector.CSSShadowModel = function(inset, x, y, blurRadius, spreadRadius, color) On 2016/08/19 16:24:35, lushnikov wrote: > This model is a good patch by itself. > > Let's extract it under common/CSSShadowModel.js and introduce it in a separate > patch, together with covering test. Done. https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:22: this._x = x || new WebInspector.CSSLength(0, WebInspector.CSSLengthUnit.None); On 2016/08/19 16:24:35, lushnikov wrote: > let's introduce a convenience method: WebInspector.CSSLength.zero() Done. https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:23: this._y = y || new WebInspector.CSSLength(0, WebInspector.CSSLengthUnit.None); On 2016/08/19 16:24:35, lushnikov wrote: > let's name these and all related local variables as offsetX and offsetY to align > with spec naming Done. https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:27: this._format = ""; On 2016/08/19 16:24:35, lushnikov wrote: > there should be some default format - otherwise, the asCSSText() would not work > on just-created instance Done. https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:44: * @return {?Array<!WebInspector.CSSShadowModel>} On 2016/08/19 16:24:35, lushnikov wrote: > let's return !Array<..>, which would be empty in case of failure Done. https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:254: * @param {string} unit On 2016/08/19 16:24:35, lushnikov wrote: > WebInspector.CSSLengthUnit Done. https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:263: WebInspector.CSSLength.Regex = /([+-]?(?:[0-9]*[.])?[0-9]+(?:[eE][+-]?[0-9]+)?)([a-zA-Z]{2,4})|[+-]?(?:0*[.])?0+(?:[eE][+-]?[0-9]+)?/g; On 2016/08/19 16:24:35, lushnikov wrote: > can we build this complex regex out of composing parts? e.g. we can extract a > number regex, and probably a CSSLengthUnit regex? > > Otherwise it's hard to follow what are your matching groups > > Also, CSSLengthUnit regex could be probably generated out of WI.CSSLengthUnit > enum Done. https://codereview.chromium.org/2230183004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ShadowEditor.js:306: function validUnit(unit) On 2016/08/19 16:24:35, lushnikov wrote: > this can go away if the CSSLengthUnit regex would be generated out of > CSSLengthUnit enum Done.
Ping
lgtm https://codereview.chromium.org/2230183004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js (right): https://codereview.chromium.org/2230183004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:2033: if (!shadows.length || !this._editable()) Let's not even parse if value is not editable. https://codereview.chromium.org/2230183004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:2039: container.appendChild(createTextNode(", ")); // Add back commas and spaces between each shadow. This removes original formatting. Do we do this everywhere? https://codereview.chromium.org/2230183004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:3051: if (this._shadowHandler && (this._propertyName === "box-shadow" || this._propertyName === "text-shadow") Are there any -webkit aliases we should support as well? https://codereview.chromium.org/2230183004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js (right): https://codereview.chromium.org/2230183004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:225: hideText: function(hide) setTextHidden
https://codereview.chromium.org/2230183004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js (right): https://codereview.chromium.org/2230183004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:2033: if (!shadows.length || !this._editable()) On 2016/08/23 17:40:56, dgozman wrote: > Let's not even parse if value is not editable. Done. https://codereview.chromium.org/2230183004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:2039: container.appendChild(createTextNode(", ")); // Add back commas and spaces between each shadow. On 2016/08/23 17:40:57, dgozman wrote: > This removes original formatting. Do we do this everywhere? Andrey says it is okay right now. https://codereview.chromium.org/2230183004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:3051: if (this._shadowHandler && (this._propertyName === "box-shadow" || this._propertyName === "text-shadow") On 2016/08/23 17:40:57, dgozman wrote: > Are there any -webkit aliases we should support as well? Yes, thanks. There is -webkit-box-shadow but not -webkit-text-shadow. https://codereview.chromium.org/2230183004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js (right): https://codereview.chromium.org/2230183004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:225: hideText: function(hide) On 2016/08/23 17:40:57, dgozman wrote: > setTextHidden Done.
https://codereview.chromium.org/2230183004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js (right): https://codereview.chromium.org/2230183004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:2039: container.appendChild(createTextNode(", ")); // Add back commas and spaces between each shadow. On 2016/08/23 19:27:15, flandy wrote: > On 2016/08/23 17:40:57, dgozman wrote: > > This removes original formatting. Do we do this everywhere? > > Andrey says it is okay right now. Let's have a TODO then.
On 2016/08/23 19:29:47, dgozman wrote: > https://codereview.chromium.org/2230183004/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js > (right): > > https://codereview.chromium.org/2230183004/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/elements/StylesSidebarPane.js:2039: > container.appendChild(createTextNode(", ")); // Add back commas and spaces > between each shadow. > On 2016/08/23 19:27:15, flandy wrote: > > On 2016/08/23 17:40:57, dgozman wrote: > > > This removes original formatting. Do we do this everywhere? > > > > Andrey says it is okay right now. > > Let's have a TODO then. Done.
The CQ bit was checked by flandy@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2230183004/#ps120001 (title: "Add TODO")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== DevTools: Add shadow-editor swatch/icon before box-shadows and text-shadows This is the first step for the shadow editor and is hidden under an experiment. BUG=639095 ========== to ========== DevTools: Add shadow-editor swatch/icon before box-shadows and text-shadows This is the first step for the shadow editor and is hidden under an experiment. BUG=639095 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== DevTools: Add shadow-editor swatch/icon before box-shadows and text-shadows This is the first step for the shadow editor and is hidden under an experiment. BUG=639095 ========== to ========== DevTools: Add shadow-editor swatch/icon before box-shadows and text-shadows This is the first step for the shadow editor and is hidden under an experiment. BUG=639095 Committed: https://crrev.com/ea253f5b0f251a0942a6cd181559a1ce0683fc75 Cr-Commit-Position: refs/heads/master@{#413927} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/ea253f5b0f251a0942a6cd181559a1ce0683fc75 Cr-Commit-Position: refs/heads/master@{#413927} |
