Bug 191037 – Web Inspector: Styles: implement copying and deletion of multiple properties

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

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

[Nikita Vasilyev]

Created attachment 353307 [details]
Patch

[Nikita Vasilyev]

Created attachment 353309 [details]
[Screen recording] With patch applied

[Radar WebKit Bug Importer]

<rdar://problem/45650078>

[Devin Rousso]

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
> [phone]  Nikita Vasilyev  <[e-mail]>

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

[Nikita Vasilyev]

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!

[Nikita Vasilyev]

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

[Blaze Burg]

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.

[Blaze Burg]

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

[Devin Rousso]

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.

[Nikita Vasilyev]

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.

[WebKit Commit Bot]

Comment on attachment 353537 [details]
Patch

Clearing flags on attachment: 353537

Committed r237659: <https://trac.webkit.org/changeset/237659>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.