Bug 59789 – Web Inspector: CSS editing breaks when entering "color: rgb(1"

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

[Pavel Feldman]

It throws error in the devtools console and does not work.

[Pavel Feldman]

Created attachment 91656 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 91656 [details]
Patch

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

> LayoutTests/inspector/styles/styles-add-invalid-property.html:31
> +        treeElement.valueElement.textContent = "rgb(";

This should be "rgb(1", as per the failing case in the reported issue.

[Pavel Feldman]

Created attachment 91677 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 91677 [details]
Patch

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

These are the primary issues I noticed.

> LayoutTests/inspector/styles/styles-commit-editing.html:25
> +        treeElement.valueElement.textContent = "rgb(1";

doesn't the following sequence work?
treeElement.valueElement.textContent = "rgb(";
treeElement.valueElement.dispatchEvent(InspectorTest.createKeyEvent("U+0031"));

This could save us a fortune (and a ForTest method).

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1644
> +            } else if (event.keyCode === WebInspector.KeyboardShortcut.Keys.Esc.code || event.keyIdentifier === "Escape")

A better approach is to fix this to check a single event.keyIdentifier === "U+001B". This way we won't have to add two hacks.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1941
> +        return typeof this.originalPropertyText === "string";

If you don't use any other type than "string", why is the original condition changed?

> Source/WebCore/inspector/front-end/inspector.js:1555
> +        

a new blank line might not be necessary

[Pavel Feldman]

Created attachment 91684 [details]
Patch

[Pavel Feldman]

Created attachment 91685 [details]
Patch

[Pavel Feldman]

(In reply to comment #4)
> (From update of attachment 91677 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91677&action=review
> 
> These are the primary issues I noticed.
> 
> > LayoutTests/inspector/styles/styles-commit-editing.html:25
> > +        treeElement.valueElement.textContent = "rgb(1";
> 
> doesn't the following sequence work?
> treeElement.valueElement.textContent = "rgb(";
> treeElement.valueElement.dispatchEvent(InspectorTest.createKeyEvent("U+0031"));
> 

I need this to be processed synchronously, while key event handler only schedules a timer.

> This could save us a fortune (and a ForTest method).
> 
> > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1644
> > +            } else if (event.keyCode === WebInspector.KeyboardShortcut.Keys.Esc.code || event.keyIdentifier === "Escape")
> 
> A better approach is to fix this to check a single event.keyIdentifier === "U+001B". This way we won't have to add two hacks.
> 

Done.

> > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1941
> > +        return typeof this.originalPropertyText === "string";
> 
> If you don't use any other type than "string", why is the original condition changed?
> 

I like searching for this.originalPropertyText occurrences more + it is compiler friendly.

> > Source/WebCore/inspector/front-end/inspector.js:1555
> > +        
> 
> a new blank line might not be necessary

Done.

[Pavel Feldman]

Committed r85324: <http://trac.webkit.org/changeset/85324>