Bug 147317 – Web Inspector: Option+Up/Down arrow keys should increment/decrement numbers in HTML and SVG attributes

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Created attachment 257548 [details]
[Animated GIF] Option+Up/Down keys don't increment/decrement :(

In Styles panel it's possible to increment/decrement numbers by pressing Option+Up/Down arrow keys. It would be very useful for editing SVG too.

The attached animated GIF was recorded on this page http://codepen.io/sol0mka/full/jpecs/.

[Radar WebKit Bug Importer]

<rdar://problem/22005531>

[Devin Rousso]

Created attachment 258017 [details]
Patch

[Nikita Vasilyev]

Created attachment 258131 [details]
[Animated GIF] Editing SVG path by pressing Alt + Up/Down

The patch works. It feels great to edit SVG paths.

Undo doesn't work.

[Nikita Vasilyev]

Comment on attachment 258017 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/EditingSupport.js:216
> +        } else if (result && result.startsWith("modify-")) {

What code is responsible for Option Up/Down behavior in the styles panel and CSS resources? We should try to abstract it away.

> Source/WebInspectorUI/UserInterface/Views/EditingSupport.js:229
> +            var wordRange = range.startContainer.rangeOfWord(range.startOffset, " \xA0\t\n\"':;,/()", element);

Already used in WebInspector.BoxModelDetailsSectionRow.StyleValueDelimiters

> Source/WebInspectorUI/UserInterface/Views/EditingSupport.js:230
> +            var matches = /(.*?)(-?(?:\d+(?:\.\d+)?|\.\d+))(.*)/.exec(wordRange.toString());

WebInspector.BoxModelDetailsSectionRow.CSSNumberRegex

[Nikita Vasilyev]

Comment on attachment 258017 [details]
Patch

Another bug: pressing down without holding Option modifies the number, pressing up doesn't.

[Devin Rousso]

Comment on attachment 258017 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/EditingSupport.js:216
>> +        } else if (result && result.startsWith("modify-")) {
> 
> What code is responsible for Option Up/Down behavior in the styles panel and CSS resources? We should try to abstract it away.

The code responsible for this behavior in the Styles and Resource panels is a CodeMirror addition.

CodeMirror.defineExtension("alterNumberInRange", ...)

I agree that it would be nice to combine the two, but the CodeMirror version makes extensive use of the CodeMirror APIs and I'd rather not mess with it too much.  I can try to merge the two in a later patch.

[Devin Rousso]

Created attachment 258156 [details]
Patch

[Nikita Vasilyev]

Undo with Cmd+Z still doesn't work.

(In reply to comment #5)
> Another bug: pressing down without holding Option modifies the number,
> pressing up doesn't.

This one is fixed.

Another bug:
"42A3|6" ("|" is a text cursor)
Pressing Option Up/Down should change the second number (36), not the first one.

[Devin Rousso]

(In reply to comment #8)
> Another bug:
> "42A3|6" ("|" is a text cursor)
> Pressing Option Up/Down should change the second number (36), not the first
> one.

Good catch!  Completely forgot that you could do this.

[Devin Rousso]

(In reply to comment #8)
> Undo with Cmd+Z still doesn't work.

Can you post a video of this or provide some steps to reproduce please?  I am not seeing any issues with undo.

[Devin Rousso]

Created attachment 258161 [details]
Patch

[Nikita Vasilyev]

Created attachment 258162 [details]
[Video] Cmd+Z isn't working

[Nikita Vasilyev]

(In reply to comment #12)
> Created attachment 258162 [details]
> [Video] Cmd+Z isn't working

Hm, this video doesn't work in Safari for me — it gets stuck on "Loading". It should work if you download it.

[Timothy Hatcher]

Making Undo/Redo work would be nice, but I am not sure we have the editing hooks for do that. These are content editable fields, not CodeMirror.

[Devin Rousso]

Created attachment 258225 [details]
Patch

[Devin Rousso]

With the most recent patch applied, I do see that the undo works once the editable field is blurred.  It's not the best but it works.  Also, this should now also work with inline styles and any other instance of a number in an attribute field.

[Timothy Hatcher]

(In reply to comment #16)
> With the most recent patch applied, I do see that the undo works once the
> editable field is blurred.  It's not the best but it works.  Also, this
> should now also work with inline styles and any other instance of a number
> in an attribute field.

Undo when blurred is using the DOM editing undo stack we talked about yesterday.

[Timothy Hatcher]

Comment on attachment 258225 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/EditingSupport.js:219
> +            var modifyValue = result.substring(7) === "up" ? 1 : -1;
> +            if (event.shiftKey)
> +                modifyValue *= 10;

What about the other modifiers we support in CodeMirror? They should be the same.

        "Alt-Up": alterNumber.bind(null, 1),
        "Ctrl-Alt-Up": alterNumber.bind(null, 0.1),
        "Shift-Alt-Up": alterNumber.bind(null, 10),
        "Alt-PageUp": alterNumber.bind(null, 10),
        "Shift-Alt-PageUp": alterNumber.bind(null, 100),
        "Alt-Down": alterNumber.bind(null, -1),
        "Ctrl-Alt-Down": alterNumber.bind(null, -0.1),
        "Shift-Alt-Down": alterNumber.bind(null, -10),
        "Alt-PageDown": alterNumber.bind(null, -10),
        "Shift-Alt-PageDown": alterNumber.bind(null, -100),

> Source/WebInspectorUI/UserInterface/Views/EditingSupport.js:253
> +            wordRange.deleteContents();
> +            wordRange.insertNode(replacementTextNode);

You should be able to use document.execCommand("insertText", false, wordPrefix + replacement + wordSuffix); that would in theory make Undo/Redo work.

https://developer.mozilla.org/en-US/docs/Web/API/Document/execCommand

[Devin Rousso]

Created attachment 258457 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 258457 [details]
Patch

Clearing flags on attachment: 258457

Committed r188138: <http://trac.webkit.org/changeset/188138>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.