Summary: | Web Inspector: Modify CSS number values with up key and down key | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Chiera <chris> | ||||||||||
Component: | Web Inspector | Assignee: | Nikita Vasilyev <nvasilyev> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Enhancement | CC: | commit-queue, inspector-bugzilla-changes, joepeck, mattbaker, nvasilyev, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Mac | ||||||||||||
OS: | macOS 10.12 | ||||||||||||
Bug Depends on: | 145982 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Chris Chiera
2017-04-12 10:47:29 PDT
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 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? (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. 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
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).
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; 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. Created attachment 323865 [details]
Patch
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. Created attachment 323874 [details]
Patch
Comment on attachment 323874 [details] Patch Clearing flags on attachment: 323874 Committed r223336: <https://trac.webkit.org/changeset/223336> All reviewed patches have been landed. Closing bug. |