Bug 152456

Summary: Web Inspector: Styling of invalid selector persists when changing the selected node
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, graouts, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
bburg: review+
Patch
none
Patch
commit-queue: commit-queue-
Patch none

Description Devin Rousso 2015-12-18 18:12:40 PST
* STEPS TO REPRODUCE:
1. Open Web Inspector to the "Elements" tab with the "Styles - Rules" panel open on https://www.apple.com/
2. Select the "<body>" element and append "{" to the first non-inline style selector and hit enter
3. The modified selector will turn red and have a warning icon appear instead of the type icon (this is expected)
4. Change nodes to something other than "<body>" and then re-select "<body>"

* EXPECTED:
There is no strikethrough on the selector of the previously modified rule

* ACTUAL:
The selector changed to the pre-modified value, but the strikethrough and error icon persist
Comment 1 Devin Rousso 2015-12-18 18:18:58 PST
Created attachment 267672 [details]
Patch
Comment 2 Joseph Pecoraro 2015-12-18 20:22:21 PST
Comment on attachment 267672 [details]
Patch

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

Looks good to me. I'd prefer if Tim reviewed as he has more experience in the Style sidebar.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:456
> +        if (this._hasInvalidSelector) {
> +            this.refresh();

This is new and should be described somewhere in the ChangeLog.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorTreeItem.js:222
> +    _handleIconElementClicked(event) {

Style: Brace should be on its own line.
Comment 3 Devin Rousso 2015-12-18 21:06:21 PST
Comment on attachment 267672 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:456
>> +            this.refresh();
> 
> This is new and should be described somewhere in the ChangeLog.

Would it be better to have it in the changelog or as a comment right above the if statement?

>> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorTreeItem.js:222
>> +    _handleIconElementClicked(event) {
> 
> Style: Brace should be on its own line.

hehe sorry forgot about that
Comment 4 Joseph Pecoraro 2015-12-18 22:04:38 PST
(In reply to comment #3)
> Comment on attachment 267672 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267672&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:456
> >> +            this.refresh();
> > 
> > This is new and should be described somewhere in the ChangeLog.
> 
> Would it be better to have it in the changelog or as a comment right above
> the if statement?

My logic tends to be: if even someone familiar with this code might have trouble remembering why this this is here then put a comment here. Otherwise, if it is obvious to someone reading this class that we would need a refresh here, just put it in the ChangeLog.

How I justify this is that comments in source code tend to get stale / incorrect. So for those quirky, not easy to understand cases comments do make sense to give context / explanation / reasoning. Otherwise, it probably doesn't deserve a comment that needs maintenance.
Comment 5 Devin Rousso 2015-12-18 22:21:56 PST
Created attachment 267682 [details]
Patch
Comment 6 BJ Burg 2015-12-27 14:54:07 PST
(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 267672 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=267672&action=review
> > 
> > >> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:456
> > >> +            this.refresh();
> > > 
> > > This is new and should be described somewhere in the ChangeLog.
> > 
> > Would it be better to have it in the changelog or as a comment right above
> > the if statement?
> 
> My logic tends to be: if even someone familiar with this code might have
> trouble remembering why this this is here then put a comment here.
> Otherwise, if it is obvious to someone reading this class that we would need
> a refresh here, just put it in the ChangeLog.
> 
> How I justify this is that comments in source code tend to get stale /
> incorrect. So for those quirky, not easy to understand cases comments do
> make sense to give context / explanation / reasoning. Otherwise, it probably
> doesn't deserve a comment that needs maintenance.

I put low-level details or explain counterintuitive/obscure things in comments. Otherwise, changelog is better for almost everything.
Comment 7 Radar WebKit Bug Importer 2015-12-27 14:54:19 PST
<rdar://problem/24012864>
Comment 8 BJ Burg 2015-12-27 18:54:45 PST
Comment on attachment 267682 [details]
Patch

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

Nice work, Devin. Just a few naming nits and style suggestions.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.css:76
>      filter: brightness(0.9);

Please add appropriate :hover styles for the cursor. It doesn't look clickable on TOT.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.css:79
> +.style-declaration-section > .header > .icon.toggle-able:active {

If the selector is invalid, then the rule won't ever apply, so there's no reason to add an active style. (edit: looks like we use the unmodified value in this case, which is guaranteed to have parsed or it wouldn't have shown up. never mind!)

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:455
> +        if (this._hasInvalidSelector) {

I would add a comment here that this will cause a revert to the unmodified selector text. It was not obvious to me, even having just read the changelog :)

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:565
> +    _markSelector(event = false)

The ' = false' seems unnecessary here.

I don't like this method name because it doesn't necessarily add a mark. I'd call it _updateSelectorIcon or something like that, and update the visual sidebar section to match it (or just revert that renaming).

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:575
> +        this._iconElement.title = WebInspector.UIString("The selector '%s' is invalid.\nClick to revert to the previous selector.").format(this._selectorElement.textContent.trim());

Please add an appropriate hover cursor to match the tooltip, this doesn't look clickable right now.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:-576
> -    get _hasInvalidSelector()

Nice.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorTreeItem.css:77
> +    filter: brightness(0.9);

Please add a cursor change.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorTreeItem.js:208
> +    _markSelector(event = false)

the ' = false' seems unnecessary here.
Comment 9 Devin Rousso 2015-12-28 12:55:14 PST
Comment on attachment 267682 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.css:76
>>      filter: brightness(0.9);
> 
> Please add appropriate :hover styles for the cursor. It doesn't look clickable on TOT.

As far as I understood with Tim, we didn't like adding "cursor: pointer;" unless the element is clearly a button (from what I can tell, the only time we use "cursor: pointer;" is for links).  I think that the hover styles as they are now are enough of an indicator that they are clickable.  If we add "cursor: pointer;", since some of the rules do not have clickable icons (user agent styles), it might be confusing to have some be "cursor: pointer;" and others not.

>> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorTreeItem.css:77
>> +    filter: brightness(0.9);
> 
> Please add a cursor change.

See above.
Comment 10 Devin Rousso 2015-12-28 20:15:49 PST
Created attachment 267978 [details]
Patch
Comment 11 Devin Rousso 2015-12-28 20:17:26 PST
Created attachment 267979 [details]
Patch

Whoops.  Forgot to change the date.
Comment 12 WebKit Commit Bot 2015-12-29 07:03:45 PST
Comment on attachment 267979 [details]
Patch

Rejecting attachment 267979 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 267979, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
file without the binary data in line: "Binary files a/Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js and b/Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js differ".  Be sure to use the --binary flag when invoking "git diff" with diffs containing binary files. at /Volumes/Data/EWS/WebKit/Tools/Scripts/VCSUtils.pm line 805, <ARGV> line 59.

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 25 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/622192
Comment 13 Devin Rousso 2015-12-29 09:17:49 PST
Created attachment 267985 [details]
Patch

Forgot --binary.  Sorry.
Comment 14 WebKit Commit Bot 2015-12-29 10:16:06 PST
Comment on attachment 267985 [details]
Patch

Clearing flags on attachment: 267985

Committed r194437: <http://trac.webkit.org/changeset/194437>
Comment 15 WebKit Commit Bot 2015-12-29 10:16:10 PST
All reviewed patches have been landed.  Closing bug.