152456 – Web Inspector: Styling of invalid selector persists when changing the selected node

RESOLVED FIXED 152456

Web Inspector: Styling of invalid selector persists when changing the selected node

https://bugs.webkit.org/show_bug.cgi?id=152456

Summary Web Inspector: Styling of invalid selector persists when changing the selecte...

Devin Rousso

Reported 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

Attachments
Patch (13.24 KB, patch) 2015-12-18 18:18 PST, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (13.38 KB, patch) 2015-12-18 22:21 PST, Devin Rousso	bburg: review+	Details Formatted Diff Diff
Patch (14.25 KB, patch) 2015-12-28 20:15 PST, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (14.09 KB, patch) 2015-12-28 20:17 PST, Devin Rousso	commit-queue: commit-queue-	Details Formatted Diff Diff
Patch (14.17 KB, patch) 2015-12-29 09:17 PST, Devin Rousso	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Devin Rousso

Comment 1 2015-12-18 18:18:58 PST

Created attachment 267672 [details] Patch

Joseph Pecoraro

Comment 2 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.

Devin Rousso

Comment 3 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

Joseph Pecoraro

Comment 4 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.

Devin Rousso

Comment 5 2015-12-18 22:21:56 PST

Created attachment 267682 [details] Patch

Blaze Burg

Comment 6 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.

Radar WebKit Bug Importer

Comment 7 2015-12-27 14:54:19 PST

<rdar://problem/24012864>

Blaze Burg

Comment 8 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.

Devin Rousso

Comment 9 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.

Devin Rousso

Comment 10 2015-12-28 20:15:49 PST

Created attachment 267978 [details] Patch

Devin Rousso

Comment 11 2015-12-28 20:17:26 PST

Created attachment 267979 [details] Patch Whoops. Forgot to change the date.

WebKit Commit Bot

Comment 12 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

Devin Rousso

Comment 13 2015-12-29 09:17:49 PST

Created attachment 267985 [details] Patch Forgot --binary. Sorry.

WebKit Commit Bot

Comment 14 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>

WebKit Commit Bot

Comment 15 2015-12-29 10:16:10 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware All

OS All

Product WebKit

Component Web Inspector

Assignee

Devin Rousso

Reported

2015-12-18 18:12 PST

Modified

2015-12-29 10:16 PST History

CC List

8 users Show

URL

Keywords InRadar

Depends on

Blocks