[webkit-reviews] review denied: [Bug 191037] Web Inspector: Styles: implement copying and deletion of multiple properties : [Attachment 353307] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has denied Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 191037: Web Inspector: Styles: implement copying and deletion of multiple
properties
https://bugs.webkit.org/show_bug.cgi?id=191037

Attachment 353307: Patch

https://bugs.webkit.org/attachment.cgi?id=353307&action=review




--- Comment #4 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 353307
  --> https://bugs.webkit.org/attachment.cgi?id=353307
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 at 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