Bug 89586 – Web Inspector: Add support for keyboard increment / decrement on numbers in attributes in Elements Panel

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Brian Grinstead]

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.

[Brian Grinstead]

Created attachment 148612 [details]
Screenshot of number highlighted in Elements Panel

[Vivek Galatage]

Created attachment 150760 [details]
Patch

[Pavel Feldman]

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?

[Vivek Galatage]

Created attachment 150882 [details]
Patch

[Pavel Feldman]

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.

[Vivek Galatage]

(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.

[Alexander Pavlov (apavlov)]

(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)...

[Vivek Galatage]

Created attachment 150915 [details]
Patch

[Vivek Galatage]

(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.

[Alexander Pavlov (apavlov)]

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 (;)

[Pavel Feldman]

Comment on attachment 150915 [details]
Patch

Looks good. Please fix according to Alexander's comments.

[Vivek Galatage]

Created attachment 150930 [details]
Patch

[Vivek Galatage]

(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.

[WebKit Review Bot]

Comment on attachment 150930 [details]
Patch

Clearing flags on attachment: 150930

Committed r121902: <http://trac.webkit.org/changeset/121902>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Brian Grinstead]

(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!