When I press up or down on a number in the Styles sidebar, it causes the number to move up or down by one. Sometimes I want to do the same thing with an attribute (either an inline style, or just a number like `colspan`). The up/down shortcuts are not hooked up in this case. See the attached image for example.
Created attachment 148612 [details] Screenshot of number highlighted in Elements Panel
Created attachment 150760 [details] Patch
Comment on attachment 150760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150760&action=review > Source/WebCore/inspector/front-end/ElementsTreeOutline.js:1193 > + function handleKeyDownEvent(event) This looks like a lot of copy-paste. Could you extract this functionality from the styles sidebar and make it re-usable instead?
Created attachment 150882 [details] Patch
Comment on attachment 150882 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150882&action=review > Source/WebCore/inspector/front-end/UIUtils.js:279 > +WebInspector.alteredFloatNumber = function(number, event) Please annotate with jsdoc and compile using compile-front-end.py. This method is private (rename to _alteredFloatNumber) > Source/WebCore/inspector/front-end/UIUtils.js:299 > + if (!String(result).match(WebInspector.StylesSidebarPane.CSSNumberRegex)) This looks like a bad dependency. Utility class should not depend on the styles panel. You should move this check to the call site. > Source/WebCore/inspector/front-end/UIUtils.js:305 > +WebInspector.handleUpOrDownValue = function(event, element, suggestionHanlder, finishHandler) Please annotate. > Source/WebCore/inspector/front-end/UIUtils.js:334 > + number = WebInspector.StylesSidebarPane.alteredHexNumber(matches[2], event); Should also be moved into this class as a private member.
(In reply to comment #5) > (From update of attachment 150882 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=150882&action=review > > > Source/WebCore/inspector/front-end/UIUtils.js:279 > > +WebInspector.alteredFloatNumber = function(number, event) > > Please annotate with jsdoc and compile using compile-front-end.py. > This method is private (rename to _alteredFloatNumber) > Will do. > > Source/WebCore/inspector/front-end/UIUtils.js:299 > > + if (!String(result).match(WebInspector.StylesSidebarPane.CSSNumberRegex)) > > This looks like a bad dependency. Utility class should not depend on the styles panel. You should move this check to the call site. > Ahh.. Thats really bad. Missed these migrations. Sure will update them as well. > > Source/WebCore/inspector/front-end/UIUtils.js:305 > > +WebInspector.handleUpOrDownValue = function(event, element, suggestionHanlder, finishHandler) > > Please annotate. > Sure, will annotate it. > > Source/WebCore/inspector/front-end/UIUtils.js:334 > > + number = WebInspector.StylesSidebarPane.alteredHexNumber(matches[2], event); > > Should also be moved into this class as a private member. Will do. Thank you for the comments. Will update all of them and re-upload the patch.
(In reply to comment #5) > (From update of attachment 150882 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=150882&action=review > > > Source/WebCore/inspector/front-end/UIUtils.js:279 > > +WebInspector.alteredFloatNumber = function(number, event) > > > Source/WebCore/inspector/front-end/UIUtils.js:305 > > +WebInspector.handleUpOrDownValue = function(event, element, suggestionHanlder, finishHandler) Vivek, could I ask you to give these methods more specific names now that you are moving them inside WebInspector? The existing names were comprehensible while inside StylesSidebarPane, but in UIUtils, it is hard to understand what they. Do. Maybe handleUpOrDownFloatValue[Change]() (and something similar for alteredFloatNumber)...
Created attachment 150915 [details] Patch
(In reply to comment #7) > (In reply to comment #5) > > (From update of attachment 150882 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=150882&action=review > > > > > Source/WebCore/inspector/front-end/UIUtils.js:279 > > > +WebInspector.alteredFloatNumber = function(number, event) > > > > > Source/WebCore/inspector/front-end/UIUtils.js:305 > > > +WebInspector.handleUpOrDownValue = function(event, element, suggestionHanlder, finishHandler) > > Vivek, could I ask you to give these methods more specific names now that you are moving them inside WebInspector? The existing names were comprehensible while inside StylesSidebarPane, but in UIUtils, it is hard to understand what they. Do. Maybe handleUpOrDownFloatValue[Change]() (and something similar for alteredFloatNumber)... Thank you Alexander, I have renamed the method to handleElementValueModifications and other methods as: _modifiedHexValue & _modifiedFloatNumber.
Comment on attachment 150915 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150915&action=review The patch works for me. Please fix the nit before landing (guess Pavel will r+ cq+ an updated patch automatically :)) Thanks for handling this feature. > Source/WebCore/inspector/front-end/MetricsSidebarPane.js:317 > + WebInspector.handleElementValueModifications(event, element, finishHandler.bind(this), undefined, customNumberHandler.bind(this)) You don't need to bind customNumberHandler, since it does not address "this" or any of its properties. This line is also missing a trailing semicolon (;)
Comment on attachment 150915 [details] Patch Looks good. Please fix according to Alexander's comments.
Created attachment 150930 [details] Patch
(In reply to comment #11) > (From update of attachment 150915 [details]) > Looks good. Please fix according to Alexander's comments. Thank you Pavel and Alexander for your quick reviews. Uploaded the new patch.
Comment on attachment 150930 [details] Patch Clearing flags on attachment: 150930 Committed r121902: <http://trac.webkit.org/changeset/121902>
All reviewed patches have been landed. Closing bug.
(In reply to comment #13) > (In reply to comment #11) > > (From update of attachment 150915 [details] [details]) > > Looks good. Please fix according to Alexander's comments. > > Thank you Pavel and Alexander for your quick reviews. Uploaded the new patch. Vivek, thank you so much for taking care of this - can't wait to check it out!