Summary: | Web Inspector: Bezier curve visual editor | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Farler <dfarler> | ||||||||||||||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | agomez, commit-queue, graouts, hi, joepeck, timothy, webkit-bug-importer | ||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||
OS: | All | ||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||
Bug Blocks: | 147681 | ||||||||||||||||||||
Attachments: |
|
Description
David Farler
2014-07-01 11:28:47 PDT
As an example: https://atom.io/packages/bezier-curve-editor Created attachment 257569 [details]
Patch
Created attachment 257571 [details]
After Patch is applied (Sidebar)
Created attachment 257572 [details]
After Patch is applied (Resources)
Comment on attachment 257569 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257569&action=review Neat! Just a lot of nits in review. > Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorBezierEditingController.js:2 > + * Copyright (C) 2013 Apple Inc. All rights reserved. Nit: 2015 > Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorBezierEditingController.js:54 > + this._bezierEditor.bezier = this._value; Nit: _value is managed by the super class, so this sub-class should use the public method: this.value. > Source/WebInspectorUI/UserInterface/Images/CubicBezier.svg:4 > + <!-- <path xmlns="http://www.w3.org/2000/svg" d="M0,14 C13,13 3,3 16,2" fill="none" stroke="white" style="stroke-width: 3;"/> --> This is commented out. Just remove it? > Source/WebInspectorUI/UserInterface/Images/CubicBezier.svg:5 > + <path xmlns="http://www.w3.org/2000/svg" d="M2,15.5 C2.5,1 13.5,15 14,0.5" fill="none" stroke="white" style="stroke-width: 3;"/> Our normal style for paths is always space separated (no commas). So: d="M 2 15.5 C 2.5 1 13.5 ..." See another svg in the Images folder for a reference. > Source/WebInspectorUI/UserInterface/Models/Geometry.js:335 > + static fromPoints(points) "fromPoints" is misleading. These aren't WebInspector.Point objects. Maybe fromCoordinates, fromNumbers, fromFloats? > Source/WebInspectorUI/UserInterface/Models/Geometry.js:340 > + return new WebInspector.CubicBezier(points[0], points[1], points[2], points[3]); I think you could use the spread operator here: return new WebInspector.CubicBezier(...points); > Source/WebInspectorUI/UserInterface/Models/Geometry.js:355 > + var matches = trimmedText.match(/^cubic-bezier\(([-\d.]+),([-\d.]+),([-\d.]+),([-\d.]+)\)$/); In general I worry about having CSS specific knowledge in Geometry.js. Scientific notation is allowed in these numbers (1e2) though highly unexpected here, note you already toLowerCased the string so you won't see (1E2). https://developer.mozilla.org/en-US/docs/Web/CSS/timing-function https://developer.mozilla.org/en-US/docs/Web/CSS/number > Source/WebInspectorUI/UserInterface/Models/Geometry.js:359 > + matches.splice(0, 1); This should probably go through and convert these strings to numbers, otherwise you are relying on implicit coercions for later actions. Note: parseFloat(...) could work, but it is not as strict as Number(...), so I think Number would work here: parseFloat("1e2") // 100 Number("1e2") // 100 parseFloat("1e2e") // 100 Number("1e2e") // NaN > Source/WebInspectorUI/UserInterface/Models/Geometry.js:380 > + toString() Ditto regarding CSS specific knowledge. > Source/WebInspectorUI/UserInterface/Models/Geometry.js:387 > + if (!Object.shallowEqual(WebInspector.CubicBezier.keywordValues[key], values)) > + continue; > + > + return key; Style: Flip these and the function gets a lot shorter. > Source/WebInspectorUI/UserInterface/Views/BezierEditor.css:32 > + width: -webkit-calc(100% - 10px); Nit: Drop the prefix, we support "calc(...)" now. > Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:45 > + bezierPreviewContainer.title = WebInspector.UIString("Click to restart the animation."); Tooltips do not normally end in periods. > Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:74 > + this._controls = []; > + for (var i = 0; i < 2; ++i) { Instead of two loops, this might be clearer as a function: function createControl(x1, y1) { ... return {point:null, line, handle} } this._controls[0] = createControl.call(this, 0, this._bezierHeight); this._controls[1] = createControl.call(this, this._bezierWidth, 0); Likewise the controls don't need to be in an array, it could be this._inControl and this._outControl. > Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:108 > + this._bezier = bezier; Lets console.assert(bezier instanceof WebInspector.CubicBezier) > Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:142 > + _handleMousedown(event) > + { > + window.addEventListener("mousemove", this, true); Ignore right click? See other mousedown handles in Web Inspector: // Only handle left mouse clicks. if (event.button !== 0) return; > Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:157 > + this._controls[this._selectedControlIndex].handle.setAttribute("r", this._controlHandleRadius - 2); > + this._triggerPreviewAnimation(); Lets clear the selectedControl as well. > Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:178 > + this._selectedControlIndex = i; Seems it would be simpler to store the selectedControl instead of the selected control index. this._selectedControl = ...; > Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:217 > + var startPoint = this._controlHandleRadius + "," + (this._bezierHeight + this._controlHandleRadius); > + > + var curvePoints = ""; > + for (var control of this._controls) { > + curvePoints += (control.point.x + this._controlHandleRadius) + "," + (control.point.y + this._controlHandleRadius) + " "; > + } > + > + var endPoint = (this._bezierWidth + this._controlHandleRadius) + "," + this._controlHandleRadius; > + > + this._bezierCurve.setAttribute("d", "M" + startPoint + " C" + curvePoints + endPoint); This is hard to read with all the string logic. How about building an array and joining with spaces at the end, something like: var path = []; path.concat(["M", this._controlHandleRadius, this._bezierHeight + this._controlHandleRadius]); path.push("C"); for (var control of this._controls) { path.push(control.point.x + this._controlHandleRadius); path.push(control.point.y + this._controlHandleRadius); } path.push(this._bezierWidth + this._controlHandleRadius); path.push(this._controlHandleRadius); path.join(" "); > Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:242 > + return document.createElementNS("http://www.w3.org/2000/svg", tagName); We should make this a general utility function instead of just in here. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1417 > + if (this._cubicBezierEditorPopover) > + return; Ignore right click? Do color markers ignore right click? > Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:662 > + // We're not interested if the color value is not inside a keyword. Nit: "color value" => "bezier value". I believe this is trying to exclude comments, so maybe that is a better comment. Very cool! One small comment, I think the include curve icon in the style sidebar should have similar margins and size to the color swatches. Created attachment 257636 [details]
Patch
Created attachment 257804 [details]
Patch
Comment on attachment 257804 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257804&action=review > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:897 > + // Look for color strings and add swatches in front of them. Look for cubic-bezier and timing functions? > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1263 > // Update the line for any color swatches that got removed. Add cubic-bezier. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1303 > // Update the line for any color swatches that got removed. Add cubic-bezier. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1414 > + _cubicBezierMarkerClicked(event) We should find a better way to make this generic enough to share with the color and gradient code. > Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:633 > + CodeMirror.defineExtension("createCubicBezierMarkers", function(range, callback) { This and the ones for color and gradients should move to another file. The others should have never been added here, since they use WebInspector namespaces objects internally, it is a layering violation. Maybe a CodeMirrorTextMarkers.js file with global functions instead of extending CodeMirror. View in context: https://bugs.webkit.org/attachment.cgi?id=257804&action=review > Source/WebInspectorUI/UserInterface/Models/Geometry.js:338 > + static fromCoordinates(coordinates) > + { > + if (!coordinates || coordinates.length < 4) > + return; It seems we expect to return a value here, lets return null. > Source/WebInspectorUI/UserInterface/Models/Geometry.js:340 > + return new WebInspector.CubicBezier(Number(coordinates[0]), Number(coordinates[1]), Number(coordinates[2]), Number(coordinates[3])); Might be good to verify none of these are NaN. That would cause confusion later. coordinates = coordinates.map((x) => Number(x)); if (coordinates.includes(NaN)) return null; > Source/WebInspectorUI/UserInterface/Models/Geometry.js:346 > + static fromString(text) > + { > + if (!text || !text.length) > + return; Same with these, lets return null. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:900 > + bezierMarker.title = WebInspector.UIString("Click to open a cubic-bezier editor."); Not sure if we should end this tooltip in a period. Created attachment 257861 [details]
Patch
Comment on attachment 257804 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257804&action=review I can't comment on the CodeMirror/TextEditor portions, but the rest looks pretty good. I would like to play with the UI, can you please post a patch that applies to TOT? > Source/WebInspectorUI/UserInterface/Models/Geometry.js:325 > +WebInspector.CubicBezier = class CubicBezier When you get a chance, maybe file a follow-up bug to consolidate this with the existing bezier curve solver code. > Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:35 > + var width = 184; editorWidth? > Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:75 > + function createControl(x1, y1) should this take WebInspector.Point? > Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:81 > + svgGroup.appendChild(line); I would explicitly set x2 and y2 as well, I have had problems in the past in some engines that do not assume x1 = x2. > Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:137 > + handleEvent(event) This is weird, we usually just attach individual listeners since some elements may already have listeners. > Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:175 > + this._selectedControl.handle.setAttribute("r", this._controlHandleRadius - 2); This seems awkwardly low level, can you fiddle with a "selected" class instead? You can set the 'r' property using CSS. > Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:186 > + point.x = clamp(0, point.x - this._controlHandleRadius, this._bezierWidth); Where did 'clamp' come from? > Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:223 > + path.push("M", this._controlHandleRadius, this._bezierHeight + this._controlHandleRadius); You should use a template string for this, since it doesn't use a loop. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:394 > + delete this._cubicBezierEditorPopover; I think we have changed to never use delete, so do: this._cubicBezierEditorPopover = undefined; Comment on attachment 257861 [details]
Patch
Marking patch r-; does not apply to TOT.
Comment on attachment 257804 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257804&action=review >> Source/WebInspectorUI/UserInterface/Models/Geometry.js:325 >> +WebInspector.CubicBezier = class CubicBezier > > When you get a chance, maybe file a follow-up bug to consolidate this with the existing bezier curve solver code. Good idea. >> Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:75 >> + function createControl(x1, y1) > > should this take WebInspector.Point? I think that that just adds another level of complexity that isn't really necessary here. >> Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:137 >> + handleEvent(event) > > This is weird, we usually just attach individual listeners since some elements may already have listeners. I was emulating how the ColorPicker and Slider are written, since they are also popover elements. >> Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:186 >> + point.x = clamp(0, point.x - this._controlHandleRadius, this._bezierWidth); > > Where did 'clamp' come from? It's a function in Utilities.js >> Source/WebInspectorUI/UserInterface/Views/BezierEditor.js:223 >> + path.push("M", this._controlHandleRadius, this._bezierHeight + this._controlHandleRadius); > > You should use a template string for this, since it doesn't use a loop. Could you add a bit more info please? I haven't dealt with template strings much. Created attachment 258334 [details]
Patch
Comment on attachment 258334 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258334&action=review > Source/WebInspectorUI/UserInterface/Views/BezierEditor.css:91 > + cursor: pointer; Pointer is for links, not UI. Created attachment 258347 [details]
Patch
Comment on attachment 258347 [details] Patch Clearing flags on attachment: 258347 Committed r188028: <http://trac.webkit.org/changeset/188028> All reviewed patches have been landed. Closing bug. |