Bug 147230 - Web Inspector: Invalid selectors can be applied to the stylesheet
Summary: Web Inspector: Invalid selectors can be applied to the stylesheet
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-07-23 11:30 PDT by Devin Rousso
Modified: 2015-07-28 12:24 PDT (History)
8 users (show)

See Also:


Attachments
Patch (13.36 KB, patch)
2015-07-23 20:56 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (10.39 KB, patch)
2015-07-27 12:08 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
After Patch is applied (Resources) (81.65 KB, image/png)
2015-07-27 12:11 PDT, Devin Rousso
no flags Details
Patch (10.46 KB, patch)
2015-07-27 23:54 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2015-07-23 11:30:05 PDT
* STEPS TO REPRODUCE:
1. Go to http://www.apple.com
2. Inspect the page and click on the <body> tag
3. Copy the selector below and paste it over one of the "body" selectors in the styles sidebar:

body { color: red; } body

4. Select a different node and then reselect the <body> tag to refresh the styles sidebar

Expected Result:
The selector above is invalid and shouldn't be applied.  Instead, the rule should revert to its original selector.

Actual Result:
A new rule is created above the edited rule with the text:

body {
    color: red;
}
Comment 1 Radar WebKit Bug Importer 2015-07-23 11:30:30 PDT
<rdar://problem/21965500>
Comment 2 Devin Rousso 2015-07-23 20:56:17 PDT
Created attachment 257426 [details]
Patch
Comment 3 Timothy Hatcher 2015-07-24 11:35:08 PDT
Comment on attachment 257426 [details]
Patch

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

Cool! Just some nits.

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:325
> -    changeRuleSelector(rule, selector)
> +    changeRuleSelector(rule, selector, callback)

You didn't use a callback. Remove.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.css:164
> +    width: 12px;
> +    height: 12px;
> +    content: url(../Images/Warning.svg);

Can you post a screenshot? It might be odd to change to a smaller icon. We could show it bigger if needed.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:524
> +        for (var selectorElement of this._selectorElements)

How are the _selectorElements here if you edited? Wouldn't they be gone until a commit and match happened again? I think the title of the parent of the _selectorElements would be better, since they all get the same title.
Comment 4 Devin Rousso 2015-07-27 11:55:08 PDT
Comment on attachment 257426 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:524
>> +        for (var selectorElement of this._selectorElements)
> 
> How are the _selectorElements here if you edited? Wouldn't they be gone until a commit and match happened again? I think the title of the parent of the _selectorElements would be better, since they all get the same title.

I have modified this to apply the title to _selectorElement, but I still have to remove the title on all of _selectorElement's children as most of them have a title that overrides the parent.
Comment 5 Devin Rousso 2015-07-27 12:08:55 PDT
Created attachment 257577 [details]
Patch
Comment 6 Devin Rousso 2015-07-27 12:11:28 PDT
Created attachment 257578 [details]
After Patch is applied (Resources)

This is what will display when the selector is changed to something invalid.  Hovering the warning icon will show "Using the previous selector <previous selector>." and hovering the selector text will show "The selector '<current selector>' is invalid.".
Comment 7 Timothy Hatcher 2015-07-27 13:27:22 PDT
Comment on attachment 257578 [details]
After Patch is applied (Resources)

I would expect the commas to be red too. This should match the red color we use for errors in the console. We should consider showing the error icon instead of warning then. Weird to see yellow/orange icon and red text.
Comment 8 Devin Rousso 2015-07-27 23:54:16 PDT
Created attachment 257637 [details]
Patch
Comment 9 WebKit Commit Bot 2015-07-28 12:24:37 PDT
Comment on attachment 257637 [details]
Patch

Clearing flags on attachment: 257637

Committed r187500: <http://trac.webkit.org/changeset/187500>
Comment 10 WebKit Commit Bot 2015-07-28 12:24:41 PDT
All reviewed patches have been landed.  Closing bug.