Bug 40522 – Web Inspector: up/down keys are not treating hex numbers properly while editing styles.

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

[Pavel Feldman]

What steps will reproduce the problem?
1. Open any page
2. Find a colored element (such as a link)
3. Hit "inspect element"
4. Click the hex css color field and use arrow up/arrow down. 

What is the expected result?
Good question, but something different.

What happens instead?
A value of 00C for example on crbug.com will increment to 0.1C ... 100C and over. It can also go 
negative with -0.1C ... -100C and so on. 
Either way, it shouldnt give invalid hex values.

[Alexander Pavlov (apavlov)]

Created attachment 87845 [details]
[PATCH] Suggested solution

The color editing works as follows:
- Up/Down -> change the first (least significant) digit
- PageUp/PageDown -> change the second digit

Shift results in the change of the second or the third digit, respectively (which completely covers the #RGB case).

[Yury Semikhatsky]

Comment on attachment 87845 [details]
[PATCH] Suggested solution

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

> LayoutTests/inspector/styles/up-down-numerics-and-colors.html:6
> +    color: #FF2;

Can you use #FAF or something that will overflow on increment?

> LayoutTests/inspector/styles/up-down-numerics-and-colors.html:25
> +            section = WebInspector.panels.elements.sidebarPanes.styles.sections[0][2];

Missing "var" before section declaration. Also could we get the section in a more reliable way?

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1650
> +        matches = /(.*?#)([\da-fA-F]+)(.*)/.exec(wordString);

.*?# - > .*#

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1658
> +            matches = /(.*?)(-?(?:\d+(?:\.\d+)?|\.\d+))(.*)/.exec(wordString);

.*? - > .*

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1740
> +        var arrowKeyPressed = (event.keyIdentifier === "Up" || event.keyIdentifier === "Down");

These shortcuts look overcomplicated too me, could we use a simpler scheme?

[Alexander Pavlov (apavlov)]

(In reply to comment #2)
> (From update of attachment 87845 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87845&action=review
> 
> > LayoutTests/inspector/styles/up-down-numerics-and-colors.html:6
> > +    color: #FF2;
> 
> Can you use #FAF or something that will overflow on increment?

Done. Tested for the most significant 'F' increment.

> > LayoutTests/inspector/styles/up-down-numerics-and-colors.html:25
> > +            section = WebInspector.panels.elements.sidebarPanes.styles.sections[0][2];
> 
> Missing "var" before section declaration. Also could we get the section in a more reliable way?

Fixed "var". I also don't know a better way to get sections, and we use this approach elsewhere.

> > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1650
> > +        matches = /(.*?#)([\da-fA-F]+)(.*)/.exec(wordString);
> 
> .*?# - > .*#

Fixed.

> > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1658
> > +            matches = /(.*?)(-?(?:\d+(?:\.\d+)?|\.\d+))(.*)/.exec(wordString);
> 
> .*? - > .*

Alas, this results in a greedy match (i.e. 188 will be parsed as (18)(8)).

> > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1740
> > +        var arrowKeyPressed = (event.keyIdentifier === "Up" || event.keyIdentifier === "Down");
> 
> These shortcuts look overcomplicated too me, could we use a simpler scheme?

We've agreed that this is consistent with the current numeric property value inc/dec's.

[Alexander Pavlov (apavlov)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
        M       LayoutTests/ChangeLog
        M       LayoutTests/http/tests/inspector/inspector-test.js
        A       LayoutTests/inspector/styles/up-down-numerics-and-colors-expected.txt
        A       LayoutTests/inspector/styles/up-down-numerics-and-colors.html
        M       Source/WebCore/ChangeLog
        M       Source/WebCore/inspector/front-end/StylesSidebarPane.js
Committed r82674