It should be possible to select multiple properties on the style editor. Once selected, - Pressing Command-C should copy the selected properties - Pressing Delete should remove the properties
Created attachment 353307 [details] Patch
Created attachment 353309 [details] [Screen recording] With patch applied
<rdar://problem/45650078>
Comment on attachment 353307 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353307&action=review r-, as there is some funky stuff happening with `tabindex` that needs some explanation Also, I've encountered a few "bugs": - starting a selection on the first property of a style and dragging up (outside of that style) will add a property to the end after mouseup - I am able to continue "selecting" past the last property of a section, even going so far as to select "User Agent Stylesheet" - the selection background doesn't appear until I drag to another property, meaning that for rules with one style, I'm unable to select it by dragging sideways - tabbing doesn't work after selecting more than one property > Source/WebInspectorUI/ChangeLog:11 > + Clicking on a property (1) and moving the mouse cursor to another property (2) should select properties 1, 2, and NIT: "click" implies both mousedown and mouseup. I think you mean to say "mousedown on 1 and mouseup on 2". > Source/WebInspectorUI/ChangeLog:14 > + Once selected, NIT: ":" instead of "," > Source/WebInspectorUI/ChangeLog:39 > + Property selection defined as two numbers: anchorIndex and focusIndex. NIT: "selection is defined" > Source/WebInspectorUI/ChangeLog:55 > + Implement copying the same way it's done for DataGrid: by adding copyHandler property to the focused element. NIT: is it critical to mention `DataGrid`, or is that just where you got the idea? > Source/WebInspectorUI/ChangeLog:62 > +2018-09-27 Nikita Vasilyev <nvasilyev@apple.com> Double ChangeLog > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:41 > + border-left: 1px solid transparent; Does this work in RTL? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:42 > + outline: none; Should these styles be behind a `.multiple-properties-selection` class? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:137 > + background-color: var(--background-color-selected); Ditto (42) > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:141 > + border-left-color: var(--border-color-selected); Ditto (41, 42) > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:319 > + console.assert(anchorIndex < this._propertyViews.length, `anchorIndex (${anchorIndex}) is greater than the last property index (${this._propertyViews.length})`); > + console.assert(focusIndex < this._propertyViews.length, `focusIndex (${focusIndex}) is greater than the last property index (${this._propertyViews.length})`); NIT: you should also assert that they're `> 0` (just for completeness' sake) > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:333 > + for (let index = 0; index < this._propertyViews.length; index++) { NIT: just use `i` NIT: `++i` > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:340 > + property.element.tabIndex = 0; Why is this needed? Please add a comment here or in the ChangeLog. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:463 > + for (let index = startIndex; index <= endIndex; index++) { Ditto (333) > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:465 > + formattedProperties.push(propertyView._property.formattedText); Add a getter for `_property` if you want to access it > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:506 > + focusIndex = Math.min(focusIndex, this._propertyViews.length - 1); > + focusIndex = Math.max(focusIndex, 0); Use `Number.constrain` > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:508 > + // Blur event causes to deselect all properties. NIT: "event deselects all" > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:519 > + let startIndex = Math.min(this._anchorIndex, this._focusIndex); > + let endIndex = Math.max(this._anchorIndex, this._focusIndex); Considering how often you do this, maybe just make a simple private method that returns `{startIndex, endIndex}` so the logic isn't repeated? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:523 > + if (!isLastPropertySelected) Just inline `isLastPropertySelected` instead of creating a variable > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:526 > + if (startIndex > 0) Merge this with the `else` > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:532 > + for (let index = endIndex; index >= startIndex; index--) Ditto (333) > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:533 > + this._propertyViews[index]._remove(); Don't access private functions of other classes > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:52 > + this._isMousePressed = true; I think this should start as false? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:479 > + window.addEventListener("mouseup", this._handleMouseUp.bind(this), {capture: true, once: true}); NIT: since this is a listener for a different object, I'd prefer if you indicated that in the function name (e.g. `_handleWindowMouseUp`) > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:38 > + this._elementContent = document.createElement("div"); This is unused? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:86 > + get selected() { return this._selected; } Style: when we have non-simple setters, we tend to expand the getter as well
Comment on attachment 353307 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353307&action=review >> Source/WebInspectorUI/ChangeLog:55 >> + Implement copying the same way it's done for DataGrid: by adding copyHandler property to the focused element. > > NIT: is it critical to mention `DataGrid`, or is that just where you got the idea? Yes, this is where I got the idea. Having a `copyHandler` property on an element is an unusual way of handling copied text. We only do this for DataGrid. Usually, we define `handleCopyEvent` property on a contentView. Since the style editor is in the details sidebar, the usual way didn't work (I tried taking this path but it was more error-prone). >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:41 >> + border-left: 1px solid transparent; > > Does this work in RTL? The style editor is currently always LTR because CSS is strictly LTR. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:319 >> + console.assert(focusIndex < this._propertyViews.length, `focusIndex (${focusIndex}) is greater than the last property index (${this._propertyViews.length})`); > > NIT: you should also assert that they're `> 0` (just for completeness' sake) `>= 0`, 0 is a valid index. I don't think so. There's a real possibility that this._propertyViews changes. Negative indexes, however, are very unlikely. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:340 >> + property.element.tabIndex = 0; > > Why is this needed? Please add a comment here or in the ChangeLog. This actually isn't needed! >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:506 >> + focusIndex = Math.max(focusIndex, 0); > > Use `Number.constrain` I was close to implementing Math.constrain 🤦♂️ >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:52 >> + this._isMousePressed = true; > > I think this should start as false? Yes! >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:38 >> + this._elementContent = document.createElement("div"); > > This is unused? Whoops!
Created attachment 353451 [details] Patch (In reply to Devin Rousso from comment #4) > Comment on attachment 353307 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=353307&action=review > > r-, as there is some funky stuff happening with `tabindex` that needs some > explanation tabIndex is always -1 now. > Also, I've encountered a few "bugs": > - starting a selection on the first property of a style and dragging up > (outside of that style) will add a property to the end after mouseup > - I am able to continue "selecting" past the last property of a section, > even going so far as to select "User Agent Stylesheet" I fixed this one. I plan to fix the rest as a follow-up. > - the selection background doesn't appear until I drag to another property, > meaning that for rules with one style, I'm unable to select it by dragging > sideways > - tabbing doesn't work after selecting more than one property
Comment on attachment 353451 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353451&action=review r=me. This is very clean, good work! > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:56 > +.multiple-properties-selection .spreadsheet-style-declaration-editor :matches(.name, .value):not(.editing) { Cool. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:322 > + console.assert(false, `Nothing to select. anchorIndex (${anchorIndex}) and focusIndex (${focusIndex}) must be numbers.`); console.error? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:124 > + remove(replacement = "") If this argument not going to be used as a string when it's the default value, then I think the default value should be `null` so that this is unambiguous.
(In reply to Nikita Vasilyev from comment #6) > Created attachment 353451 [details] > Patch > > (In reply to Devin Rousso from comment #4) > > Comment on attachment 353307 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=353307&action=review > > > > r-, as there is some funky stuff happening with `tabindex` that needs some > > explanation > > tabIndex is always -1 now. > > > Also, I've encountered a few "bugs": > > - starting a selection on the first property of a style and dragging up > > (outside of that style) will add a property to the end after mouseup > > - I am able to continue "selecting" past the last property of a section, > > even going so far as to select "User Agent Stylesheet" > > I fixed this one. I plan to fix the rest as a follow-up. Please post bug links here when you have filed them for followups.
Comment on attachment 353451 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353451&action=review > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:340 > + property.element.tabIndex = -1; I still don't understand why this is necessary. It deserves a comment here or in the ChangeLog. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:353 > + focusedProperty.element.tabIndex = -1; Ditto (340) > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:220 > + property.element.tabIndex = -1; I feel like this could easily be forgotten if changes are made in the future to the `tabindex` value. Consider making these types of changes into methods on the holder of the `element`. Something like "resetTabindex". Also, is this actually needed anymore? Everywhere seems to set the tabIndex to -1, so I don't see why this is necessary.
Created attachment 353537 [details] Patch Bug 191125 - Web Inspector: Styles: when selecting multiple properties don't add a new property on mouseup Bug 191126 - Web Inspector: Styles: improve property selection heuristic Bug 191135 - Web Inspector: Styles: When property is selected pressing Tab should select property name (In reply to Devin Rousso from comment #9) > Also, is this actually needed anymore? Everywhere seems to set the tabIndex > to -1, so I don't see why this is necessary. No, it isn't needed! I updated the patch. I only need to set tabIndex to -1 once when the property element is created. Without `tabIndex = -1` I wouldn't be able to focus on the property element.
Comment on attachment 353537 [details] Patch Clearing flags on attachment: 353537 Committed r237659: <https://trac.webkit.org/changeset/237659>
All reviewed patches have been landed. Closing bug.