Bug 93581 – Web Inspector: Feature Request - Adding mouse gesture for editing attribute values in elements/css panel

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

[sam]

Right now we have a support of using up/down arrow keys for updating attribute values (for ex updating font size in  font-size: 12px). 
This is also coupled with meta keys for adjusting increment/decrement steps.

It would be more intuitive to enhance this with a mouse based gestures such as mouse scroll (up/down events).

[sam]

I would like to know suggestions/opinions before starting to work on this patch.

[Pavel Feldman]

I like the idea, I was even hacking a patch for it on some holiday. I think once you started using the wheel or Up/Down buttons, or press and hold the Ctrl/Cmd key we should show a range control over the value:
http://www.youtube.com/watch?v=PUv66718DII&t=4m10s

[sam]

Created attachment 157428 [details]
Patch

[sam]

(In reply to comment #2)
> I like the idea, I was even hacking a patch for it on some holiday. I think once you started using the wheel or Up/Down buttons, or press and hold the Ctrl/Cmd key we should show a range control over the value:
> http://www.youtube.com/watch?v=PUv66718DII&t=4m10s

Thank you Pavel for your suggestion.

That's great and interesting way of handling the value modifications. I initially intended to have the mouse scroll as another way of input for value modifications in addition to current keyboard events.

I think your suggestion is again interesting representation. So shall we handle this in the same bug or we should handle in a separate one?

[Pavel Feldman]

Comment on attachment 157428 [details]
Patch

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

r- for small nits.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:2530
>              if (this._handleNameOrValueUpDown(event)) {

I would leave switch as is.

> Source/WebCore/inspector/front-end/TextPrompt.js:115
> +        this._element.addEventListener("mousewheel", this._boundOnKeyDown, false);

You should bind _handleNameOrValueUpDown explicitly (otherwise calling boundOnKeyDown upon mouse event is misleading) and call event.consume(true) from within _handleNameOrValueUpDown instead of doing preventDefault.

> Source/WebCore/inspector/front-end/UIUtils.js:315
> +WebInspector._modifiedHexValue = function(hexString, event, direction)

You should either derive direction and scale from the event or not pass event into this method. Otherwise, you do half work in one place and another half in another!

> Source/WebCore/inspector/front-end/UIUtils.js:350
> +WebInspector._modifiedFloatNumber = function(number, event, direction)

ditto

[sam]

Created attachment 157686 [details]
Patch

[sam]

(In reply to comment #5)
> (From update of attachment 157428 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157428&action=review
> 
Thank You Pavel for your review.
> r- for small nits.
> 
> > Source/WebCore/inspector/front-end/StylesSidebarPane.js:2530
> >              if (this._handleNameOrValueUpDown(event)) {
> 
> I would leave switch as is.
> 
> > Source/WebCore/inspector/front-end/TextPrompt.js:115
> > +        this._element.addEventListener("mousewheel", this._boundOnKeyDown, false);
> 
> You should bind _handleNameOrValueUpDown explicitly (otherwise calling boundOnKeyDown upon mouse event is misleading) and call event.consume(true) from within _handleNameOrValueUpDown instead of doing preventDefault.
I have made some changes in TextPrompt as it is a parent object for CSSPropertyPrompt. I have added a generic mouse wheel event listener within text prompt. This can be overridden by CSSPropertyPrompt for handling mouse wheel events for changing attribute values.

Please let me know if this sounds logical.

[Alexander Pavlov (apavlov)]

Comment on attachment 157428 [details]
Patch

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

> Source/WebCore/inspector/front-end/UIUtils.js:389
> +            if (event.wheelDelta == 120)

Does this imply you want both horizontal and vertical wheels behave exactly the same?

[Alexander Pavlov (apavlov)]

Comment on attachment 157686 [details]
Patch

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

> Source/WebCore/inspector/front-end/TextPrompt.js:116
> +        this._element.addEventListener("mousewheel", this._boundOnMouseWheel, false);

You never remove this listener (see the removal code for this._boundOnKeyDown in detach()). We had a major glitch in one of Chrome releases due to a similar bug (hint: that is the reason why we store the bound listeners as fields). r- for this.

> Source/WebCore/inspector/front-end/UIUtils.js:318
> +    if (event.type && event.type === "mousewheel") {

The first check (event.type) is not necessary here.

> Source/WebCore/inspector/front-end/UIUtils.js:319
> +        if (event.wheelDelta == 120)

This check can fail in certain cases, since 120 is a single tick multiplier, and it is possible for a wheelDelta to contain several ticks. Also, 120 is based on the Windows port, and this may break in the future/for other ports. E.g. chrome/src/ui/views/events/event.cc:

#if defined(OS_WIN)
// This value matches windows WHEEL_DELTA.
// static
const int MouseWheelEvent::kWheelDelta = 120;
#else
// This value matches GTK+ wheel scroll amount.
const int MouseWheelEvent::kWheelDelta = 53;
#endif

> Source/WebCore/inspector/front-end/UIUtils.js:410
> +    var mouseWheelScroll = (event.type && event.type === "mousewheel")

The "event.type" check is extraneous.
Also, a trailing semicolon is missing.

[sam]

Created attachment 157703 [details]
Patch

[sam]

Created attachment 157707 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 157707 [details]
Patch

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

> Source/WebCore/inspector/front-end/TextPrompt.js:228
> +        return;

Such "return"s usually do not have rich semantics in JS :)

> Source/WebCore/inspector/front-end/UIUtils.js:314
> +  */

add "@return {string}" here

> Source/WebCore/inspector/front-end/UIUtils.js:315
> +WebInspector._valueModificationDirection = function(event)

This needs to be fixed to account for the zero event.wheelDeltaY value (as we discussed in chat, you don't want to handle the horizontal scrolling). See comments below.

> Source/WebCore/inspector/front-end/UIUtils.js:317
> +    var direction = false;

Please use "null" instead of "false" to be type-consistent

> Source/WebCore/inspector/front-end/UIUtils.js:319
> +        if (Math.abs(event.wheelDeltaY) == event.wheelDeltaY)

Please use simpler checks:

if (event.wheelDeltaY > 0)
    direction = "Up";
else if (event.wheelDeltaY < 0)
    direction = "Down";

> Source/WebCore/inspector/front-end/UIUtils.js:326
> +        else if(event.keyIdentifier === "Down" || event.keyIdentifier === "PageDown")

missing whitespace after "if"

> Source/WebCore/inspector/front-end/UIUtils.js:339
>      var number = parseInt(hexString, 16);

Add

if (!direction)
    return hexString;

before this line. You don't want to handle horizontal mouse scrolling.

> Source/WebCore/inspector/front-end/UIUtils.js:375
> +    var arrowKeyOrMouseWheelEvent = (event.keyIdentifier === "Up" || event.keyIdentifier === "Down" || event.type === "mousewheel");

Add

if (!direction)
    return number;

before this line. You don't want to handle horizontal mouse scrolling.

> Source/WebCore/inspector/front-end/UIUtils.js:410
> +    var mouseWheelScroll = (event.type === "mousewheel");

You don't need this var.

> Source/WebCore/inspector/front-end/UIUtils.js:411
> +    if (!arrowKeyPressed && !pageKeyPressed && !mouseWheelScroll)

if (!arrowKeyOrMouseWheelEvent && !pageKeyPressed)

[sam]

Created attachment 157921 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 157921 [details]
Patch

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

Great work! Please fix the two nits, and we are good to land.

> Source/WebCore/inspector/front-end/TextPrompt.js:228
> +        // Default.  

Comments of this kind should be omitted, since WebKit discourages comments that add no value to the code (and sometimes it even discourages the comments that do :)). If you feel strongly about having a comment here, it should be something more useful, like "// Subclasses can implement." Even though I would go with no comment at all (since the point is clear to the code readers.)

> Source/WebCore/inspector/front-end/UIUtils.js:314
> +  * @return {string}

Apologies, that was my fault: this line should actually be "@return {?string}" (add the question-mark), since the returned value is nullable.

[sam]

Created attachment 157927 [details]
Patch

[sam]

(In reply to comment #14)
> (From update of attachment 157921 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157921&action=review
> 
> Great work! Please fix the two nits, and we are good to land.
> 
> > Source/WebCore/inspector/front-end/TextPrompt.js:228
> > +        // Default.  
> 
> Comments of this kind should be omitted, since WebKit discourages comments that add no value to the code (and sometimes it even discourages the comments that do :)). If you feel strongly about having a comment here, it should be something more useful, like "// Subclasses can implement." Even though I would go with no comment at all (since the point is clear to the code readers.)
> 
> > Source/WebCore/inspector/front-end/UIUtils.js:314
> > +  * @return {string}
> 
> Apologies, that was my fault: this line should actually be "@return {?string}" (add the question-mark), since the returned value is nullable.

Thanks for the review Alexander. Have incorporated the review comments in modified patch.

[Alexander Pavlov (apavlov)]

Comment on attachment 157927 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 157927 [details]
Patch

Clearing flags on attachment: 157927

Committed r125402: <http://trac.webkit.org/changeset/125402>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.