Bug 193305 – Web Inspector: Styles: easier way to select a single line

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

If I have a single property within a rule, there's no way for me to highlight it so I can copy/paste it :(

[Nikita Vasilyev]

Created attachment 358823 [details]
[Animated GIF] Select single property

It's possible, but it's cumbersome.

1. Mousedown on the property
2. Move mouse cursor away from the property
3. Hover the property again

[Nikita Vasilyev]

*** Bug 191126 has been marked as a duplicate of this bug. ***

[Devin Rousso]

(In reply to Nikita Vasilyev from comment #1)
> It's possible, but it's cumbersome.
Oh wow, yeah that's not fun :(

Glad to know that there is some way though :)

[Nikita Vasilyev]

Created attachment 361542 [details]
Patch

[Nikita Vasilyev]

Created attachment 361544 [details]
[Video] With patch applied

[Devin Rousso]

Comment on attachment 361542 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361542&action=review

r=me, nice!

> Source/WebInspectorUI/ChangeLog:8
> +        Start property selection if mouse cursor moved more than 8px after mousedown.

Why 8px?  I'm guessing it's related to the size of a single character, but perhaps we should be a bit more "exact" about it.  Some explanation would be useful.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:49
> +        this._boundHandleWindowMouseMove = this._handleWindowMouseMove.bind(this);

Please lazy initialize this when it's first needed (although keep it as null in the constructor).

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:411
> +            this._mouseDownPoint = WI.Point.fromEvent(event);

We should also define `_mouseDownPoint` as a `null` value in the constructor.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:412
> +            window.addEventListener("mousemove", this._boundHandleWindowMouseMove, false);

You can drop the `false`.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:433
> +        console.assert(this._mouseDownPoint);

Style: add a newline after.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:434
> +        if (WI.Point.fromEvent(event).distance(this._mouseDownPoint) > 8) {

NIT: I'd rather you put the "known" value first and compare it to the current value, like `this._mouseDownPoint.distance(WI.Point.fromEvent(event))`.  This way, it's a little less "chaining" and a bit easier to read.

NIT: we should invert this and make it an early return.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:435
> +            if (!this._propertiesEditor.hasSelectedProperties())

Is it actually possible for `hasSelectedProperties()` to be true?  We clear any selection when we "mousedown" so it should be cleared before are able to move the mouse (and therefore fire this listener).

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:436
> +                this._propertiesEditor.selectProperties(this._mouseDownIndex, this._mouseDownIndex);

Should we check that `_mouseDownIndex` isn't `NaN`?  We only set it to a value when there's a property underneath the mouse in "mousedown".  I know we set `display: none` when there are no properties, but maybe we should assert just in case.

[Nikita Vasilyev]

Comment on attachment 361542 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361542&action=review

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:435
>> +            if (!this._propertiesEditor.hasSelectedProperties())
> 
> Is it actually possible for `hasSelectedProperties()` to be true?  We clear any selection when we "mousedown" so it should be cleared before are able to move the mouse (and therefore fire this listener).

Yes, it's possible if we hover another property before moving 8px.

[Nikita Vasilyev]

Created attachment 361570 [details]
Patch for landing

[Nikita Vasilyev]

Created attachment 361571 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 361571 [details]
Patch for landing

Clearing flags on attachment: 361571

Committed r241226: <https://trac.webkit.org/changeset/241226>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47936525>