Bug 147317 - Web Inspector: Option+Up/Down arrow keys should increment/decrement numbers in HTML and SVG attributes
Summary: Web Inspector: Option+Up/Down arrow keys should increment/decrement numbers i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-07-26 23:23 PDT by Nikita Vasilyev
Modified: 2015-08-07 10:59 PDT (History)
9 users (show)

See Also:


Attachments
[Animated GIF] Option+Up/Down keys don't increment/decrement :( (391.79 KB, image/gif)
2015-07-26 23:23 PDT, Nikita Vasilyev
no flags Details
Patch (5.65 KB, patch)
2015-08-01 13:32 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Animated GIF] Editing SVG path by pressing Alt + Up/Down (824.00 KB, image/gif)
2015-08-03 16:18 PDT, Nikita Vasilyev
no flags Details
Patch (8.37 KB, patch)
2015-08-03 23:04 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (9.62 KB, patch)
2015-08-04 00:35 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Video] Cmd+Z isn't working (3.45 MB, video/mp4)
2015-08-04 00:44 PDT, Nikita Vasilyev
no flags Details
Patch (9.66 KB, patch)
2015-08-04 16:14 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (10.19 KB, patch)
2015-08-06 22:48 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2015-07-26 23:23:03 PDT
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/.
Comment 1 Radar WebKit Bug Importer 2015-07-26 23:34:41 PDT
<rdar://problem/22005531>
Comment 2 Devin Rousso 2015-08-01 13:32:24 PDT
Created attachment 258017 [details]
Patch
Comment 3 Nikita Vasilyev 2015-08-03 16:18:44 PDT
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.
Comment 4 Nikita Vasilyev 2015-08-03 16:35:21 PDT
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
Comment 5 Nikita Vasilyev 2015-08-03 16:43:14 PDT
Comment on attachment 258017 [details]
Patch

Another bug: pressing down without holding Option modifies the number, pressing up doesn't.
Comment 6 Devin Rousso 2015-08-03 23:02:21 PDT
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.
Comment 7 Devin Rousso 2015-08-03 23:04:09 PDT
Created attachment 258156 [details]
Patch
Comment 8 Nikita Vasilyev 2015-08-03 23:37:10 PDT
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.
Comment 9 Devin Rousso 2015-08-04 00:29:51 PDT
(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.
Comment 10 Devin Rousso 2015-08-04 00:34:50 PDT
(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.
Comment 11 Devin Rousso 2015-08-04 00:35:19 PDT
Created attachment 258161 [details]
Patch
Comment 12 Nikita Vasilyev 2015-08-04 00:44:11 PDT
Created attachment 258162 [details]
[Video] Cmd+Z isn't working
Comment 13 Nikita Vasilyev 2015-08-04 00:46:21 PDT
(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.
Comment 14 Timothy Hatcher 2015-08-04 15:27:46 PDT
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.
Comment 15 Devin Rousso 2015-08-04 16:14:59 PDT
Created attachment 258225 [details]
Patch
Comment 16 Devin Rousso 2015-08-04 16:17:11 PDT
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.
Comment 17 Timothy Hatcher 2015-08-04 16:20:52 PDT
(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.
Comment 18 Timothy Hatcher 2015-08-04 16:27:32 PDT
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
Comment 19 Devin Rousso 2015-08-06 22:48:53 PDT
Created attachment 258457 [details]
Patch
Comment 20 WebKit Commit Bot 2015-08-07 10:59:01 PDT
Comment on attachment 258457 [details]
Patch

Clearing flags on attachment: 258457

Committed r188138: <http://trac.webkit.org/changeset/188138>
Comment 21 WebKit Commit Bot 2015-08-07 10:59:05 PDT
All reviewed patches have been landed.  Closing bug.