Bug 199341 - Web Inspector: Styles: Command-X should cut selected properties
Summary: Web Inspector: Styles: Command-X should cut selected properties
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-28 17:44 PDT by Nikita Vasilyev
Modified: 2019-07-23 17:21 PDT (History)
5 users (show)

See Also:


Attachments
Patch (8.56 KB, patch)
2019-06-28 18:15 PDT, Nikita Vasilyev
hi: review-
Details | Formatted Diff | Diff
[Animated GIF] With patch applied (184.00 KB, image/gif)
2019-06-28 18:17 PDT, Nikita Vasilyev
no flags Details
Patch (8.60 KB, patch)
2019-07-05 15:51 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-highsierra (3.29 MB, application/zip)
2019-07-05 17:20 PDT, EWS Watchlist
no flags Details
Patch (9.14 KB, patch)
2019-07-19 17:58 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch (9.05 KB, patch)
2019-07-23 14:26 PDT, Nikita Vasilyev
hi: review+
Details | Formatted Diff | Diff
Patch (8.87 KB, patch)
2019-07-23 16:28 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2019-06-28 17:44:39 PDT
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.
Comment 1 Nikita Vasilyev 2019-06-28 18:15:17 PDT
Created attachment 373170 [details]
Patch
Comment 2 Nikita Vasilyev 2019-06-28 18:17:17 PDT
Created attachment 373172 [details]
[Animated GIF] With patch applied
Comment 3 Devin Rousso 2019-07-05 12:10:02 PDT
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.
Comment 4 Nikita Vasilyev 2019-07-05 15:51:53 PDT
Created attachment 373548 [details]
Patch
Comment 5 EWS Watchlist 2019-07-05 17:20:30 PDT Comment hidden (obsolete)
Comment 6 EWS Watchlist 2019-07-05 17:20:32 PDT Comment hidden (obsolete)
Comment 7 Nikita Vasilyev 2019-07-05 17:26:00 PDT
Comment on attachment 373548 [details]
Patch

Unrelated test failures.
Comment 8 Devin Rousso 2019-07-05 17:45:29 PDT
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 9 Nikita Vasilyev 2019-07-06 18:19:44 PDT
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 10 Nikita Vasilyev 2019-07-12 16:10:39 PDT
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 11 Devin Rousso 2019-07-19 00:02:28 PDT
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 12 Nikita Vasilyev 2019-07-19 17:46:06 PDT
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.
Comment 13 Nikita Vasilyev 2019-07-19 17:58:57 PDT
Created attachment 374532 [details]
Patch
Comment 14 Devin Rousso 2019-07-20 12:59:06 PDT
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 15 Devin Rousso 2019-07-20 12:59:12 PDT
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
Comment 16 Nikita Vasilyev 2019-07-23 14:26:58 PDT
Created attachment 374710 [details]
Patch
Comment 17 Devin Rousso 2019-07-23 15:45:37 PDT
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).
Comment 18 Nikita Vasilyev 2019-07-23 16:28:36 PDT
Created attachment 374732 [details]
Patch
Comment 19 WebKit Commit Bot 2019-07-23 17:20:45 PDT
Comment on attachment 374732 [details]
Patch

Clearing flags on attachment: 374732

Committed r247760: <https://trac.webkit.org/changeset/247760>
Comment 20 WebKit Commit Bot 2019-07-23 17:20:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Radar WebKit Bug Importer 2019-07-23 17:21:19 PDT
<rdar://problem/53472934>