Bug 152456

Summary:

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

Product:

WebKit

Reporter:

Devin Rousso <hi>

Component:

Web Inspector

Assignee:

Devin Rousso <hi>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bburg, commit-queue, graouts, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer

Priority:

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

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.