* 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
Created attachment 267672 [details] Patch
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 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
(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.
Created attachment 267682 [details] Patch
(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.
<rdar://problem/24012864>
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 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.
Created attachment 267978 [details] Patch
Created attachment 267979 [details] Patch Whoops. Forgot to change the date.
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
Created attachment 267985 [details] Patch Forgot --binary. Sorry.
Comment on attachment 267985 [details] Patch Clearing flags on attachment: 267985 Committed r194437: <http://trac.webkit.org/changeset/194437>
All reviewed patches have been landed. Closing bug.