The styles editor has had copying of properties for a while but no cutting. Pressing Command-X should cut (copy to clipboard and remove) selected properties. If the keyboard shortcut is non-standard (e.g. not Command-X), it should also work.
Created attachment 373170 [details] Patch
Created attachment 373172 [details] [Animated GIF] With patch applied
Comment on attachment 373170 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373170&action=review r-, due to naming issue and the order of operations when not editable. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:550 > + let removed = this.removeSelectedProperties(); I think you mean `_removeSelectedProperties`. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:588 > + _handleCut(event) The "cut" event should only have an effect if the style is editable. If not, it should cause a beep. So ... (>612) > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:604 > + for (let i = startIndex; i <= endIndex; ++i) { > + let propertyView = this._propertyViews[i]; > + formattedProperties.push(propertyView.property.formattedText); > + } You could rewrite/simplify this as: ``` let formattedProperties = this._propertyViews.slice(startIndex, endIndex + 1).map((propertyView) => propertyView.property.formattedText); ``` > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:612 > + if (!this.style.editable) (>588) ... we should check this earlier (and assert here). > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:493 > + // Without this, "copy" and "cut" events wouldn't fire on SpreadsheetCSSStyleDeclarationEditor. This comment doesn't help me understand _why_ this is needed. Please explain.
Created attachment 373548 [details] Patch
Comment on attachment 373548 [details] Patch Attachment 373548 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12671875 New failing tests: webgl/2.0.0/conformance/context/context-release-upon-reload.html
Created attachment 373555 [details] Archive of layout-test-results from ews112 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 373548 [details] Patch Unrelated test failures.
Comment on attachment 373548 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373548&action=review > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:553 > + let removed = this._removeSelectedProperties(); This should also check for `this._style.editable` and `InspectorFrontendHost.beep()` if not. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:585 > + if (!this.hasSelectedProperties()) Should we also `InspectorFrontendHost.beep()` if they try to copy when no properties are selected. Is that even possible? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:593 > + if (!this.hasSelectedProperties()) Ditto (>585). > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:495 > + this._propertiesEditor.placeTextCaretInFocusedProperty(); Does this affect keyboard navigation, like left/right or tab? NIT: I'd also include an explanation of this in the ChangeLog.
Comment on attachment 373548 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373548&action=review >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:585 >> + if (!this.hasSelectedProperties()) > > Should we also `InspectorFrontendHost.beep()` if they try to copy when no properties are selected. Is that even possible? Yes, it is possible — when editing CSS property name/value. We shouldn't beep for this case — copying selected text shoud work just fine there. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:495 >> + this._propertiesEditor.placeTextCaretInFocusedProperty(); > > Does this affect keyboard navigation, like left/right or tab? > > NIT: I'd also include an explanation of this in the ChangeLog. It doesn't affect keyboard navigation.
Comment on attachment 373548 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373548&action=review >>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:495 >>> + this._propertiesEditor.placeTextCaretInFocusedProperty(); >> >> Does this affect keyboard navigation, like left/right or tab? >> >> NIT: I'd also include an explanation of this in the ChangeLog. > > It doesn't affect keyboard navigation. Do you want me to add duplicate the comment in the Changelog?
Comment on attachment 373548 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373548&action=review > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:551 > return; We should `InspectorFrontendHost.beep()` here. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:556 > Style: extra newline. >>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:585 >>> + if (!this.hasSelectedProperties()) >> >> Should we also `InspectorFrontendHost.beep()` if they try to copy when no properties are selected. Is that even possible? > > Yes, it is possible — when editing CSS property name/value. We shouldn't beep for this case — copying selected text shoud work just fine there. In that case, we should check that there's some text selected and `InspectorFrontendHost.beep()` if not. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:598 > + if (!this.style.editable) { > + InspectorFrontendHost.beep(); > + return; I'd move this above the other `if`, as we should always `InspectorFrontendHost.beep()` when trying to "cut", even if there's nothing selected. >>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:495 >>>> + this._propertiesEditor.placeTextCaretInFocusedProperty(); >>> >>> Does this affect keyboard navigation, like left/right or tab? >>> >>> NIT: I'd also include an explanation of this in the ChangeLog. >> >> It doesn't affect keyboard navigation. > > Do you want me to add duplicate the comment in the Changelog? It's probably fine without it It's very specific to this use, so I don't think it's "general" enough.
Comment on attachment 373548 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373548&action=review >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:551 >> return; > > We should `InspectorFrontendHost.beep()` here. Makes sense. >>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:585 >>>> + if (!this.hasSelectedProperties()) >>> >>> Should we also `InspectorFrontendHost.beep()` if they try to copy when no properties are selected. Is that even possible? >> >> Yes, it is possible — when editing CSS property name/value. We shouldn't beep for this case — copying selected text shoud work just fine there. > > In that case, we should check that there's some text selected and `InspectorFrontendHost.beep()` if not. The beep already happens — it's a native behavior for contentEditable. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:593 >> + if (!this.hasSelectedProperties()) > > Ditto (>585). The beep already happens — it's a native behavior for contentEditable.
Created attachment 374532 [details] Patch
Comment on attachment 374532 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374532&action=review > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:556 > + if (removed) It looks like `_removeSelectedProperties` always returns `true`, so this check is unnecessary. Should `_removeSelectedProperties` have another `return`, or can you remove the `if`?
Comment on attachment 373548 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373548&action=review >>>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:585 >>>>> + if (!this.hasSelectedProperties()) >>>> >>>> Should we also `InspectorFrontendHost.beep()` if they try to copy when no properties are selected. Is that even possible? >>> >>> Yes, it is possible — when editing CSS property name/value. We shouldn't beep for this case — copying selected text shoud work just fine there. >> >> In that case, we should check that there's some text selected and `InspectorFrontendHost.beep()` if not. > > The beep already happens — it's a native behavior for contentEditable. Oh neat! I didn't know that! =D
Created attachment 374710 [details] Patch
Comment on attachment 374710 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374710&action=review r=me, nice work! :) > Source/WebInspectorUI/ChangeLog:22 > + Introduce this method to use when copying and cutting CSS properties. This kind of ChangeLog message doesn't really add anything useful. I'd either explain _what_ happens inside `_copySelectedProperties` or remove it. > Source/WebInspectorUI/ChangeLog:25 > + Introduce this method to use when cutting and removing CSS properties (by pressing Delete). Ditto (>22).
Created attachment 374732 [details] Patch
Comment on attachment 374732 [details] Patch Clearing flags on attachment: 374732 Committed r247760: <https://trac.webkit.org/changeset/247760>
All reviewed patches have been landed. Closing bug.
<rdar://problem/53472934>