Bug 118896 – Web Inspector: support click-and-drag editing of CSS numeric values

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Antoine Quint]

We should support click-and-drag editing of CSS numeric values which Adobe apps support and Bret Victor's Tangle library supports too (https://github.com/worrydream/Tangle).

[Antoine Quint]

<rdar://problem/13993923>

[Antoine Quint]

Made very good progress on this, I should have a fix ready for review soon.

However, there is an issue with the handling of the Command key since jump-to-definition also acts on Cmd+click to highlight tokens and jump to the definition on click. Should we just override the whole jump-to-definition behaviour when the token is a number in favour of click-and-drag editing? Or we should use an alternate key modifier system? Or is there a better way yet?

[Timothy Hatcher]

I think it is possible for command-click on a number to take you somewhere different compared to command clicking the property or even another adjacent value of the same property when complex SASS is involved. Right Joe?

It could be the option key, since that is the key used in conjunction with the arrow keys.

[Antoine Quint]

(In reply to comment #3)
> It could be the option key, since that is the key used in conjunction with the arrow keys.

Option key also has the role of incrementing by .1 rather 1, and shift by 10. We could use the control key?

[Antoine Quint]

I think we may also need a way to change the cursor outside of the inspector window as well so that while you're dragging to adjust the value, the cursor remains "col-resize".

[Timothy Hatcher]

(In reply to comment #4)
> (In reply to comment #3)
> > It could be the option key, since that is the key used in conjunction with the arrow keys.
> 
> Option key also has the role of incrementing by .1 rather 1, and shift by 10. We could use the control key?

The shortcuts should match the modifiers used by the arrow keys. Right now option-arrow is increment by one (except when near zero). So that shouldn't change to 0.1. Option-shift-arrow is 10. If you want 0.1 separate, then that would be a better candidate for control-arrow or option-control-arrow (thick conflicts with a system wide zoom accessibility shortcut).

[Timothy Hatcher]

(In reply to comment #5)
> I think we may also need a way to change the cursor outside of the inspector window as well so that while you're dragging to adjust the value, the cursor remains "col-resize".

WebInspector.elementDragStart handles this by adding a a top level element that covers the whole inspector. We can break that out into a reusable piece (and rename it to something other than glass pane…).

[Joseph Pecoraro]

Possible idea: make different dragging adjustment rates matching the keyboard arrow adjustments

  Option Drag       = default behavior, change value by 1 per pixel, and maybe also 0.1 when near -1..1
  Shift+Option Drag = change value by 10 per pixel, for wide adjustments (you can toggle shift on/off while dragging. Too crazy?)
  ???+Option Drag   = change value by 0.1 per pixel, for fine grained adjustment (maybe not Ctrl, since that could trigger a right click on OS X)

[Timothy Hatcher]

(In reply to comment #8)
> Possible idea: make different dragging adjustment rates matching the keyboard arrow adjustments
> 
>   Option Drag       = default behavior, change value by 1 per pixel, and maybe also 0.1 when near -1..1
>   Shift+Option Drag = change value by 10 per pixel, for wide adjustments (you can toggle shift on/off while dragging. Too crazy?)
>   ???+Option Drag   = change value by 0.1 per pixel, for fine grained adjustment (maybe not Ctrl, since that could trigger a right click on OS X)

That is what I was getting at. Make the modifiers match what we use for the arrow keys, except dragging instead of arrow keys. And add a new modifier for explicit 0.1.

I think allowing you to change modifiers mid-drag makes sense and should be supported.

[Antoine Quint]

(In reply to comment #7)
> (In reply to comment #5)
> > I think we may also need a way to change the cursor outside of the inspector window as well so that while you're dragging to adjust the value, the cursor remains "col-resize".
> 
> WebInspector.elementDragStart handles this by adding a a top level element that covers the whole inspector. We can break that out into a reusable piece (and rename it to something other than glass pane…).

Hmm, would that set the cursor outside of the window as well?

[Antoine Quint]

(In reply to comment #9)
> (In reply to comment #8)
> > Possible idea: make different dragging adjustment rates matching the keyboard arrow adjustments
> > 
> >   Option Drag       = default behavior, change value by 1 per pixel, and maybe also 0.1 when near -1..1
> >   Shift+Option Drag = change value by 10 per pixel, for wide adjustments (you can toggle shift on/off while dragging. Too crazy?)
> >   ???+Option Drag   = change value by 0.1 per pixel, for fine grained adjustment (maybe not Ctrl, since that could trigger a right click on OS X)
> 
> That is what I was getting at. Make the modifiers match what we use for the arrow keys, except dragging instead of arrow keys. And add a new modifier for explicit 0.1.
> 
> I think allowing you to change modifiers mid-drag makes sense and should be supported.

Yes, that's what I have working now, it's not a problem. Maybe Cmd+Option+Drag for the 0.1 change?

[Timothy Hatcher]

(In reply to comment #10)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > I think we may also need a way to change the cursor outside of the inspector window as well so that while you're dragging to adjust the value, the cursor remains "col-resize".
> > 
> > WebInspector.elementDragStart handles this by adding a a top level element that covers the whole inspector. We can break that out into a reusable piece (and rename it to something other than glass pane…).
> 
> Hmm, would that set the cursor outside of the window as well?

Oh, I didn't catch the "outside the window part". No, it wouldn't. We would need to add InspectorFrontendHost or WebCore API for that.

[Antoine Quint]

(In reply to comment #12)
> (In reply to comment #10)
> > (In reply to comment #7)
> > > (In reply to comment #5)
> > > > I think we may also need a way to change the cursor outside of the inspector window as well so that while you're dragging to adjust the value, the cursor remains "col-resize".
> > > 
> > > WebInspector.elementDragStart handles this by adding a a top level element that covers the whole inspector. We can break that out into a reusable piece (and rename it to something other than glass pane…).
> > 
> > Hmm, would that set the cursor outside of the window as well?
> 
> Oh, I didn't catch the "outside the window part". No, it wouldn't. We would need to add InspectorFrontendHost or WebCore API for that.

We can deal with that as a followup.

[Antoine Quint]

Created attachment 207392 [details]
Patch

[Antoine Quint]

> > Oh, I didn't catch the "outside the window part". No, it wouldn't. We would need to add InspectorFrontendHost or WebCore API for that.
> 
> We can deal with that as a followup.

Follow up is https://bugs.webkit.org/show_bug.cgi?id=119045.

[Timothy Hatcher]

Comment on attachment 207392 [details]
Patch

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

r- for now. Looking good!

> Source/WebInspectorUI/UserInterface/CodeMirrorAdditions.js:289
> +        var foundNumber = WebInspector.alterNumberInRange(amount, codeMirror, startPosition, endPosition, true);

This is a layering violation, since this file is meant to be standalone from the Inspector (if anyone else ever wanted to use it or merge things back to CodeMirror.)

You should keep alterNumberInRange in this file and use CodeMirror.defineExtension to add it to CodeMirror instances. Then the codeMirror parameter would be removed, and you would use 'this' for the CM instance.

> Source/WebInspectorUI/UserInterface/CodeMirrorAdditions.js:340
> +    CodeMirror.defineOption("dragToAdjustNumbers", false, function(codeMirror, value, oldValue) {
> +        if (!codeMirror.dragToAdjustController)
> +            codeMirror.dragToAdjustController = new WebInspector.DragToAdjustController(codeMirror);
> +        codeMirror.dragToAdjustController.enabled = value;
> +    });

This should live in DragToAdjustController.js like the other add-on options, they all self-define. You could also define it as default true, so you don't need to touch all instances and we won't forget it if we add more later.

> Source/WebInspectorUI/UserInterface/CodeMirrorAdditions.js:355
> +WebInspector.alterNumberInRange = function(amount, codeMirror, startPosition, endPosition, affectsSelection)

See previous comment.

> Source/WebInspectorUI/UserInterface/CodeMirrorOverrides.css:87
> +.CodeMirror.drag-to-adjust .CodeMirror-lines {
> +    cursor: col-resize;
> +}

This should be in a DragToAdjustController.css file to be better associated with the class. CodeMirrorOverrides.css is only meant to correct/override some of the default CodeMirror styles to better match our platform.

> Source/WebInspectorUI/UserInterface/ConsolePrompt.js:42
> +        dragToAdjustNumbers: true

See comment about making this the default so you don't need to do this everywhere.

> Source/WebInspectorUI/UserInterface/DragToAdjustController.js:26
> +WebInspector.DragToAdjustController = function(codeMirror)

I feel the name DragToAdjustController is too generic. We also try to put CodeMirror in the name for these, since they are deeply integrated with CodeMirror and not a higher level.

I would suggest CodeMirrorDragToAlterNumberController or CodeMirrorAlterNumberController if you want to move the rest of the alter number code into this one file, and all under the option.

> Source/WebInspectorUI/UserInterface/DragToAdjustController.js:40
> +    set active(active)

I am torn about having just setters that look like they are public (no underscore). I would prefer them to be underscored, but then that conflicts with the internal property names. Could these just be private methods since there is no getter (even if it makes the call sites uglier)? Or switch the internal properties to double underscore prefixes and have the setters have a single underscore?

You should add a // Private comment too to separate the methods.

> Source/WebInspectorUI/UserInterface/DragToAdjustController.js:49
> +        }
> +        else {

One line.

> Source/WebInspectorUI/UserInterface/DragToAdjustController.js:67
> +            WebInspector.elementDragEnd(window.event);

You should assert window.event somewhere. Or add event as a parameter if this becomes a private method.

> Source/WebInspectorUI/UserInterface/DragToAdjustController.js:84
> +            this._element.addEventListener("mouseenter", this, false);
> +            this._element.addEventListener("mouseleave", this, false);
> +        } else {
> +            this._element.removeEventListener("mouseenter", this, false);
> +            this._element.removeEventListener("mouseleave", this, false);

We don't include the optional capture parameter if it is false.

> Source/WebInspectorUI/UserInterface/DragToAdjustController.js:110
> +    handleEvent: function(event)

I would put this above the private methods in a "protected section" labeled with a // Protected comment.

> Source/WebInspectorUI/UserInterface/DragToAdjustController.js:152
> +        if (this._hoveredTokenInfo &&
> +            this._hoveredTokenInfo.line === position.line &&
> +            this._hoveredTokenInfo.token.start === token.start &&
> +            this._hoveredTokenInfo.token.end === token.end)
> +            return;

Excessive wrapping. Could be two lines:

if (this._hoveredTokenInfo && this._hoveredTokenInfo.line === position.line &&
    this._hoveredTokenInfo.token.start === token.start && this._hoveredTokenInfo.token.end === token.end)

> Source/WebInspectorUI/UserInterface/DragToAdjustController.js:154
> +        var containsNumber = token.type.indexOf("number") !== -1;

This works, but it also differs from the arrow key alter number logic. This way makes more sense, but it still has the issue of <rdar://problem/13877085> where up/down (and now drag) over the unit does not work. We should fix and consolidate the logic for both when we fix <rdar://problem/13877085>.

> Source/WebInspectorUI/UserInterface/DragToAdjustController.js:192
> +        if (event.metaKey)

I think control would be better than command, it is closer in the key cluster of option, shift and control. Command also seems more sacred… *waves hands*

[Antoine Quint]

Created attachment 207394 [details]
Patch

[Antoine Quint]

New patch deals with all of Tim's comments.

[WebKit Commit Bot]

Comment on attachment 207394 [details]
Patch

Clearing flags on attachment: 207394

Committed r153087: <http://trac.webkit.org/changeset/153087>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Antoine Quint]

Followup bug: https://bugs.webkit.org/show_bug.cgi?id=119048.

[Joseph Pecoraro]

Comment on attachment 207394 [details]
Patch

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

Nice!

> Source/WebInspectorUI/UserInterface/CodeMirrorDragToAlterNumberController.js:95
> +        if (active) {
> +            WebInspector.notifications.addEventListener(WebInspector.Notification.GlobalModifierKeysDidChange, this._modifiersDidChange, this);
> +            this._element.addEventListener("mousemove", this);

By only starting to listen to mousemove and  modifier changes on "mouseenter" you'll miss any modifiers / hovered token the initial moment the mouse enters. I see currently the code updates this on mousemove, but right on "mouseenter" if you don't move the mouse the state could be wrong. Maybe this could affect a number right on the edge of an editor area. Maybe that is too obscure or doesn't even repro.

> Source/WebInspectorUI/UserInterface/Main.html:143
> +    <script src="CodeMirrorDragToAlterNumberController.js"></script>

Why not put this down with the other CodeMirror*Controller files?