Bug 199341 – Web Inspector: Styles: Command-X should cut selected properties

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

Created attachment 373170 [details]
Patch

[Nikita Vasilyev]

Created attachment 373172 [details]
[Animated GIF] With patch applied

[Devin Rousso]

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.

[Nikita Vasilyev]

Created attachment 373548 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Nikita Vasilyev]

Comment on attachment 373548 [details]
Patch

Unrelated test failures.

[Devin Rousso]

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.

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

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?

[Devin Rousso]

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.

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

Created attachment 374532 [details]
Patch

[Devin Rousso]

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`?

[Devin Rousso]

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

[Nikita Vasilyev]

Created attachment 374710 [details]
Patch

[Devin Rousso]

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).

[Nikita Vasilyev]

Created attachment 374732 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 374732 [details]
Patch

Clearing flags on attachment: 374732

Committed r247760: <https://trac.webkit.org/changeset/247760>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53472934>