Bug 59789

Summary: Web Inspector: CSS editing breaks when entering "color: rgb(1"
Product: WebKit Reporter: Pavel Feldman <pfeldman>
Component: Web Inspector (Deprecated)Assignee: Pavel Feldman <pfeldman>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch yurys: review+

Description Pavel Feldman 2011-04-29 02:34:56 PDT
It throws error in the devtools console and does not work.
Comment 1 Pavel Feldman 2011-04-29 02:51:22 PDT
Created attachment 91656 [details]
Patch
Comment 2 Alexander Pavlov (apavlov) 2011-04-29 03:11:43 PDT
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.
Comment 3 Pavel Feldman 2011-04-29 07:25:26 PDT
Created attachment 91677 [details]
Patch
Comment 4 Alexander Pavlov (apavlov) 2011-04-29 07:44:00 PDT
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
Comment 5 Pavel Feldman 2011-04-29 08:20:00 PDT
Created attachment 91684 [details]
Patch
Comment 6 Pavel Feldman 2011-04-29 08:23:53 PDT
Created attachment 91685 [details]
Patch
Comment 7 Pavel Feldman 2011-04-29 08:25:31 PDT
(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.
Comment 8 Pavel Feldman 2011-04-29 08:35:31 PDT
Committed r85324: <http://trac.webkit.org/changeset/85324>