Bug 193305 - Web Inspector: Styles: easier way to select a single line
Summary: Web Inspector: Styles: easier way to select a single line
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
: 191126 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-01-09 15:25 PST by Devin Rousso
Modified: 2019-02-08 17:48 PST (History)
5 users (show)

See Also:


Attachments
[Animated GIF] Select single property (14.08 KB, image/gif)
2019-01-10 13:14 PST, Nikita Vasilyev
no flags Details
Patch (3.44 KB, patch)
2019-02-08 15:30 PST, Nikita Vasilyev
hi: review+
hi: commit-queue-
Details | Formatted Diff | Diff
[Video] With patch applied (1.08 MB, video/quicktime)
2019-02-08 15:34 PST, Nikita Vasilyev
no flags Details
Patch for landing (3.70 KB, patch)
2019-02-08 17:25 PST, Nikita Vasilyev
nvasilyev: commit-queue+
Details | Formatted Diff | Diff
Patch for landing (3.69 KB, patch)
2019-02-08 17:26 PST, 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 Devin Rousso 2019-01-09 15:25:18 PST
If I have a single property within a rule, there's no way for me to highlight it so I can copy/paste it :(
Comment 1 Nikita Vasilyev 2019-01-10 13:14:37 PST
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
Comment 2 Nikita Vasilyev 2019-01-10 13:16:02 PST
*** Bug 191126 has been marked as a duplicate of this bug. ***
Comment 3 Devin Rousso 2019-01-10 13:19:38 PST
(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 :)
Comment 4 Nikita Vasilyev 2019-02-08 15:30:36 PST
Created attachment 361542 [details]
Patch
Comment 5 Nikita Vasilyev 2019-02-08 15:34:24 PST
Created attachment 361544 [details]
[Video] With patch applied
Comment 6 Devin Rousso 2019-02-08 16:58:00 PST
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.
Comment 7 Nikita Vasilyev 2019-02-08 17:24:24 PST
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.
Comment 8 Nikita Vasilyev 2019-02-08 17:25:36 PST Comment hidden (obsolete)
Comment 9 Nikita Vasilyev 2019-02-08 17:26:41 PST
Created attachment 361571 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2019-02-08 17:47:14 PST
Comment on attachment 361571 [details]
Patch for landing

Clearing flags on attachment: 361571

Committed r241226: <https://trac.webkit.org/changeset/241226>
Comment 11 WebKit Commit Bot 2019-02-08 17:47:16 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2019-02-08 17:48:29 PST
<rdar://problem/47936525>