Bug 170779 – Web Inspector: Modify CSS number values with up key and down key

• Component: Web Inspector
• Status: RESOLVED FIXED

[Chris Chiera]

Background:
Selecting, editing and manipulating text in Chrome inspector has always been a joy, but Safari/Webkit has lacked some of those refinements.

One issue I brought up previously was in Chrome selecting a css attribute in Chrome inspector automatically selects that text rather then putting the cursor there unless your double click, which as a developer seems the better use case rather than the reverse of having a single click put cursor and double click highlight. While that idea was shot down previously here, I see in the latest Webkit it now selects text the Chrome way by default and the user can disable that in settings. Awesome!

Issue:
However, in Chrome if I select an attribute such as "3em" with a single click I can then up arrow to increase that to say 4em, 5em, etc, or down arrow to 2em, 1em, -1em etc. which is incredible helpful in easily seeing how a layout changes with a single key but currently does not work in Webkit. With the new default selection, hopefully the up/down for number based items.

Happy to provide additional details as needed and loving all the great progress in Webkit Inspector.

[Nikita Vasilyev]

In Web Inspector you can do the same by holding Option key. It also works for CSS files in Resources.

https://webkit.org/blog/2518/state-of-web-inspector/#styles

[Chris Chiera]

Thanks Nikita, though this requests is for allowing adjusting with solely the up/down keys without having to hold additional keys as the extra keys would seem to be an unnecessary more complication process than Chrome's method.

Unless Safari requires option because they have future plans for up/down on numbers without a additional keys to carry out a different function?

[Matt Baker]

(In reply to Chris Chiera from comment #2)
> Thanks Nikita, though this requests is for allowing adjusting with solely
> the up/down keys without having to hold additional keys as the extra keys
> would seem to be an unnecessary more complication process than Chrome's
> method.
> 
> Unless Safari requires option because they have future plans for up/down on
> numbers without a additional keys to carry out a different function?

It would be great to simplify this. I don't use this feature that often, so when I do it takes me a few tries to figure out the right modifier.

[Radar WebKit Bug Importer]

<rdar://problem/33170633>

[Nikita Vasilyev]

Created attachment 323845 [details]
Patch

Now the modifier keys match Chrome:
- Option modifies the value by 0.1
- Shift modifies the value by 10
- Command modifies the value by 100

[Nikita Vasilyev]

Created attachment 323846 [details]
[Animated GIF] With patch applied

Up/down keys behavior outside of the styles sidebar was unchanged (up/down doesn't modify numeric values, requires holding Option key).

[Matt Baker]

Comment on attachment 323845 [details]
Patch

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

r-, only because this needs a test for the new EditingSupport function.

> Source/WebInspectorUI/UserInterface/Views/EditingSupport.js:244
> +            } else

I think this would be cleaner as:

if (!modified)
    return;

event.preventDefault();
...

Actually, preventDefault gets called on line 250 below, so you should only need the early return.

> Source/WebInspectorUI/UserInterface/Views/EditingSupport.js:280
> +WI.modifyNumericValue = function(element, modifyValue)

This was existing code, but it would still be good to add a layout test now that it's included in EditingSupport.

I think it could use a better name too. I didn't really know what `modifyValue` was, or what this function did to `element` until I got to the new code in SpreadsheetTextField later. What do you think about:

WI.incrementElementValue = function(element, amount)
{
    ....
}

I think "numeric" is implied, but it could be `incrementNumericElementValue` if you think that's more descriptive. Kind of a long name though.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:236
> +                modifyValue /= 10;

Instead of multiplying and dividing, you could do:

let amount;
if (event.metaKey)
    amount = 100;
else if (event.shiftKey)
    amount = 10;
else if (event.altKey)
    amount = 0.1;
else
    amount = 1;

if (event.key === "ArrowDown")
    amount = -amount;

[Joseph Pecoraro]

Comment on attachment 323845 [details]
Patch

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

At this point matt reviewed so I'll just leave my comments.

>> Source/WebInspectorUI/UserInterface/Views/EditingSupport.js:244
>> +            if (modified) {
>> +                event.preventDefault();
>> +            } else
> 
> I think this would be cleaner as:
> 
> if (!modified)
>     return;
> 
> event.preventDefault();
> ...
> 
> Actually, preventDefault gets called on line 250 below, so you should only need the early return.

Style: No braces.

Better yet, you could reformat this as:

    let modified = WI.modifyNumericValue(element, modifyValue);
    if (!modified)
        return;

    event.preventDefault();
    ...

>> Source/WebInspectorUI/UserInterface/Views/EditingSupport.js:280
>> +WI.modifyNumericValue = function(element, modifyValue)
> 
> This was existing code, but it would still be good to add a layout test now that it's included in EditingSupport.
> 
> I think it could use a better name too. I didn't really know what `modifyValue` was, or what this function did to `element` until I got to the new code in SpreadsheetTextField later. What do you think about:
> 
> WI.incrementElementValue = function(element, amount)
> {
>     ....
> }
> 
> I think "numeric" is implied, but it could be `incrementNumericElementValue` if you think that's more descriptive. Kind of a long name though.

I think `modifyDelta` or just `delta` would be clearer than `modifyValue`. At first I thought `modifyValue` was the value that was going to be modified. What do you think?

You could probably write tests for this. Even though its in Views, its essentially just a Utility function given an element.

[Nikita Vasilyev]

Created attachment 323865 [details]
Patch

[Matt Baker]

Comment on attachment 323865 [details]
Patch

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

r=me, with a minor nit.

> Source/WebInspectorUI/UserInterface/Views/EditingSupport.js:240
> +            let modified = WI.incrementElementValue(element, modifyValue);

Nit: change `modifyValue` to `delta` to be consistent with other caller, and function signature.

[Nikita Vasilyev]

Created attachment 323874 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 323874 [details]
Patch

Clearing flags on attachment: 323874

Committed r223336: <https://trac.webkit.org/changeset/223336>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.