Bug 147572

Created attachment 258089 [details] Patch

Created attachment 258201 [details] Patch

Created attachment 258331 [details] Patch

Comment on attachment 258331 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258331&action=review Looking good, a few things need to be cleaned up before landing. If you could post a screenshot of the widget in action, that would be useful for review (this patch doesn't add any instantiations to the inspector UI). > Source/WebInspectorUI/ChangeLog:8 > + * UserInterface/Models/CSSRule.js: You should add a high level description of the patch. Change notes for existing classes would speed up review as well. > Source/WebInspectorUI/UserInterface/Models/CSSRule.js:185 > + var mediaText = ""; s/var/let/ > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:201 > + this.text = this._initialText; Should this set this.modified = false? > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:280 > + var numMediaQueries = 0; let mediaQueriesCount > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:292 > + styleText += " ".repeat(numMediaQueries) + this._ownerRule.selectorText; It seems like this string pasting code would be easy to regress. Can you add a little test that dumps the generated rule string for some common cases? > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:294 > + styleText += this._selectorElement.textContent; err, this is a model class, right? Why does it own a DOM element? > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:-458 > - _generateCSSRuleString() Oh, I guess this was just moved around. You should still clean up the let and variable names. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorItem.css:36 > + background-color: rgb(34, 131, 246) !important; We have moved to using hsl() for inspector colors. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorItem.css:82 > + /* TODO: Fix cursor bar and highlight height */ We don't use TODO, either fix it or make it a FIXME. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorItem.css:89 > + cursor bar from displaying at the beginning of the em-dash */ Nit: trailing period. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorItem.css:90 > + content: " \2014"; I would add /* em-dash */ at the end. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorItem.js:26 > +WebInspector.VisualStyleSelectorItem = class VisualStyleSelectorItem extends WebInspector.GeneralTreeElement I found it weird that this extends TreeElement but doesn't have that suffix. Not sure if there are other cases of this, or if we want to start the trend. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorItem.js:33 > + console.assert(style.ownerRule); Add an instanceof check. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorItem.js:101 > + this._listItemNode.classList.remove("editable"); Is a super call expected here? > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorItem.js:108 > + var titleText = this._mainTitleElement.textContent; s/var/let/ > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorItem.js:155 > + this._checkboxElement.title = this._checkboxElement.checked ? WebInspector.UIString("Click to disable the selected rule") : WebInspector.UIString("Click to enable the selected rule"); Nit: I would use if/else, as this line wraps very long. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorItem.js:167 > + if (event.keyCode === 13) { What is 13? > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorItem.js:169 > + return; return not necessary. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorItem.js:194 > +} Nit: trailing ; > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.css:32 > + border-bottom: 1px solid rgb(179, 179, 179); Nit: use hsl > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.css:42 > + width: calc(100% - 85px); sexy. Does this work? > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.css:69 > + border-left: 1px solid lightgrey; Nit: don't use lightgrey keyword. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.css:80 > + opacity: 0.7; You might want to use -webkit-filter instead of opacity, which always seems to come back and cause problems with layering. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.css:103 > + background-color: rgb(242, 242, 242); Nit (many places): don't use rgb() or keywords. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:2 > + * Copyright (C) 2013, 2Apple Inc. All rights reserved. err, what? > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:30 > + var selectorSection = {element: document.createElement("div")}; hmm, is this trickery required to avoid TDZ errors? > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:38 > + this._delegate = delegate || null Nit: trailing ; > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:76 > + this._nodeStyles.__lastSelectedRule = style; Use a symbol instead of __prop. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:102 > + selector.addEventListener(WebInspector.VisualStyleSelectorItem.Event.StyleTextReset, this._styleTextReset.bind(this)); no need to bind, just pass `this` as third arg. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:226 > + this._currentSelector.className = "current-selector " + selectedTreeElement.iconClassName; classList.add(...) > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:228 > + var text = selectedTreeElement.mainTitle; let selectorText = ... > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:235 > + this.dispatchEventToListeners(WebInspector.VisualStyleSelectorSection.Event.SelectorChanged); Should this fire if the tree element is deselected, or is there always one element selected? If the latter is true, add an assertion at the top of this method. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:254 > + var enabled = event && event.data && event.data.enabled; const styleEnabled > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:255 > + var style = this.currentRule(); is this a single style declaration or a rule? Not clear from these variable names. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:256 > + if (!style) Should this be an assertion? > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:259 > + var text = style.text; let styleText = ... > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:263 > + var replacement = ""; // Comment or uncomment the style text. let newStyleText = ...; > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:270 > + style[WebInspector.VisualStyleDetailsPanel.StyleDisabled] = !enabled; Whoa, what's going on here? This should use a getter/setter.

Comment on attachment 258331 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258331&action=review >> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:292 >> + styleText += " ".repeat(numMediaQueries) + this._ownerRule.selectorText; > > It seems like this string pasting code would be easy to regress. Can you add a little test that dumps the generated rule string for some common cases? Yeah I'll add that. >> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:294 >> + styleText += this._selectorElement.textContent; > > err, this is a model class, right? Why does it own a DOM element? This was accidentally included. I'll remove it. >> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorItem.js:26 >> +WebInspector.VisualStyleSelectorItem = class VisualStyleSelectorItem extends WebInspector.GeneralTreeElement > > I found it weird that this extends TreeElement but doesn't have that suffix. Not sure if there are other cases of this, or if we want to start the trend. Good point. I'll change it to WebInspector.VisualStyleSelectorTreeItem. >> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorItem.js:101 >> + this._listItemNode.classList.remove("editable"); > > Is a super call expected here? No. This is just called by TreeElement (if it exists) whenever that element is deselected. >> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorItem.js:167 >> + if (event.keyCode === 13) { > > What is 13? 13 is Enter >> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.css:42 >> + width: calc(100% - 85px); > > sexy. Does this work? Yeah it does. >> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:2 >> + * Copyright (C) 2013, 2Apple Inc. All rights reserved. > > err, what? Whoops. >> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:30 >> + var selectorSection = {element: document.createElement("div")}; > > hmm, is this trickery required to avoid TDZ errors? Yeah. I also needed it to be selectorSection.element for it to work with DetailsSection as it expects a class with that property. >> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:226 >> + this._currentSelector.className = "current-selector " + selectedTreeElement.iconClassName; > > classList.add(...) I actually want to replace the entire classList since the iconClassName may change between different treeElements. >> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:235 >> + this.dispatchEventToListeners(WebInspector.VisualStyleSelectorSection.Event.SelectorChanged); > > Should this fire if the tree element is deselected, or is there always one element selected? If the latter is true, add an assertion at the top of this method. There is always one tree element selected. >> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:255 >> + var style = this.currentRule(); > > is this a single style declaration or a rule? Not clear from these variable names. currentStyle is much clearer. I'll change that. >> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:270 >> + style[WebInspector.VisualStyleDetailsPanel.StyleDisabled] = !enabled; > > Whoa, what's going on here? This should use a getter/setter. This is used in https://bugs.webkit.org/show_bug.cgi?id=147570. Its a symbol that tracks whether that particular rule has been disabled.

Comment on attachment 258331 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258331&action=review >> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:292 >> + styleText += " ".repeat(numMediaQueries) + this._ownerRule.selectorText; > > It seems like this string pasting code would be easy to regress. Can you add a little test that dumps the generated rule string for some common cases? Yeah I'll add that. >> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:294 >> + styleText += this._selectorElement.textContent; > > err, this is a model class, right? Why does it own a DOM element? This was accidentally included. I'll remove it. >> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorItem.js:26 >> +WebInspector.VisualStyleSelectorItem = class VisualStyleSelectorItem extends WebInspector.GeneralTreeElement > > I found it weird that this extends TreeElement but doesn't have that suffix. Not sure if there are other cases of this, or if we want to start the trend. Good point. I'll change it to WebInspector.VisualStyleSelectorTreeItem. >> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorItem.js:101 >> + this._listItemNode.classList.remove("editable"); > > Is a super call expected here? No. This is just called by TreeElement (if it exists) whenever that element is deselected. >> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorItem.js:167 >> + if (event.keyCode === 13) { > > What is 13? 13 is Enter >> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.css:42 >> + width: calc(100% - 85px); > > sexy. Does this work? Yeah it does. >> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:2 >> + * Copyright (C) 2013, 2Apple Inc. All rights reserved. > > err, what? Whoops. >> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:30 >> + var selectorSection = {element: document.createElement("div")}; > > hmm, is this trickery required to avoid TDZ errors? Yeah. I also needed it to be selectorSection.element for it to work with DetailsSection as it expects a class with that property. >> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:226 >> + this._currentSelector.className = "current-selector " + selectedTreeElement.iconClassName; > > classList.add(...) I actually want to replace the entire classList since the iconClassName may change between different treeElements. >> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:235 >> + this.dispatchEventToListeners(WebInspector.VisualStyleSelectorSection.Event.SelectorChanged); > > Should this fire if the tree element is deselected, or is there always one element selected? If the latter is true, add an assertion at the top of this method. There is always one tree element selected. >> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:255 >> + var style = this.currentRule(); > > is this a single style declaration or a rule? Not clear from these variable names. currentStyle is much clearer. I'll change that. >> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:270 >> + style[WebInspector.VisualStyleDetailsPanel.StyleDisabled] = !enabled; > > Whoa, what's going on here? This should use a getter/setter. This is used in https://bugs.webkit.org/show_bug.cgi?id=147570. Its a symbol that tracks whether that particular rule has been disabled.

You should also add aria roles so that these sections are accessible.

Comment on attachment 258331 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258331&action=review >>>> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:270 >>>> + style[WebInspector.VisualStyleDetailsPanel.StyleDisabled] = !enabled; >>> >>> Whoa, what's going on here? This should use a getter/setter. >> >> This is used in https://bugs.webkit.org/show_bug.cgi?id=147570. Its a symbol that tracks whether that particular rule has been disabled. > > This is used in https://bugs.webkit.org/show_bug.cgi?id=147570. Its a symbol that tracks whether that particular rule has been disabled. Oh, I see. I think we have settled on suffixing these with Symbol, so that this is more obvious. i.e., WebInspector.VisualStyleDetailsPanel.StyleDisabledSymbol

Created attachment 258460 [details] Patch

Comment on attachment 258460 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258460&action=review This patch looks ready, modulo the line comments and accessibility support. Once you add the latter or explain why it's not necessary, i'll r+ the patch. > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:47 > + this.modified = false; Please make it this._modified and add a getter. Or, add a getter and don't keep the member---you should be able to recompute modified as this.text === this._initialText. > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:191 > + this.modified = modified; Here, you can still fire the event in this branch. > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:200 > + this.text = this._initialText; I think this._modified would be reset here too? > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.css:110 > +.details-section.visual-style-selector-section > .content > .selectors > .selector-list > .section-divider > .disclosure-button, You could use :matches to shorten this selector list. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:2 > + * Copyright (C) 2015, Apple Inc. All rights reserved. No comma. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:123 > + for (let existingRule of uniqueRules) { Is there a way we can use a Map and compute a unique key rather than doing this n^2 comparison? For example, it could get the rule's id and join its array elements into a string. Even better would be to add a toString() method to CSSId, but I don't recall whether CSSId is an inspector model class or if we use the raw protocol payload. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:193 > + let divider = new WebInspector.GeneralTreeElement("section-divider", WebInspector.UIString("Inherited from %s").format(WebInspector.displayNameForNode(inherited.node))); Please format the string into a local on a separate line for readability. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:224 > + this._currentSelectorElement.className = "current-selector " + selectedTreeElement.iconClassName; I would add a comment that we intentionally clear past class names, so this doesn't get bulk converted by accident. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:252 > + let styleEnabled = event && event.data && event.data.enabled; Move this closer to first use. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorTreeItem.css:74 > + -webkit-user-modify: read-write-plaintext-only; Cool, didn't know about that. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorTreeItem.js:30 > + let iconClassName; I thought this wasn't supposed to work.. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorTreeItem.js:33 > + console.assert(style.ownerRule instanceof WebInspector.CSSRule); add style.ownerRule as an argument, so it gets logged when the assertion fails. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorTreeItem.js:72 > + // Overrides from TreeElement (Private) I would just say // Protected since it's obviously making a super call.

Comment on attachment 258460 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258460&action=review >> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:47 >> + this.modified = false; > > Please make it this._modified and add a getter. Or, add a getter and don't keep the member---you should be able to recompute modified as this.text === this._initialText. I'll add a getter and make it this._modified, but I still want a member variable. The idea behind this._modified and the event firing below is to only fire the event if the modified state changes, not every single time the text is modified. If the text is already modified from the initial state, there is no reason to fire another event since it has already been marked as modified. If instead the text is reset back to the initial value, then an event should be fired (and again if the text is modified later). >> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:191 >> + this.modified = modified; > > Here, you can still fire the event in this branch. What do you mean by that? >> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:200 >> + this.text = this._initialText; > > I think this._modified would be reset here too? I actually don't want to reset this._modified because that will interfere with the logic in "set text()" above and prevent WebInspector.CSSStyleDeclaration.Event.InitialTextModified from firing. >> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:123 >> + for (let existingRule of uniqueRules) { > > Is there a way we can use a Map and compute a unique key rather than doing this n^2 comparison? For example, it could get the rule's id and join its array elements into a string. Even better would be to add a toString() method to CSSId, but I don't recall whether CSSId is an inspector model class or if we use the raw protocol payload. That's a really good idea! >> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorTreeItem.js:30 >> + let iconClassName; > > I thought this wasn't supposed to work.. This works just fine. What doesn't work is using "this" before the super() call below.

Comment on attachment 258460 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258460&action=review >>> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:47 >>> + this.modified = false; >> >> Please make it this._modified and add a getter. Or, add a getter and don't keep the member---you should be able to recompute modified as this.text === this._initialText. > > I'll add a getter and make it this._modified, but I still want a member variable. The idea behind this._modified and the event firing below is to only fire the event if the modified state changes, not every single time the text is modified. If the text is already modified from the initial state, there is no reason to fire another event since it has already been marked as modified. If instead the text is reset back to the initial value, then an event should be fired (and again if the text is modified later). Oh, I see. Maybe we should think of a better name for this variable, like this._haveModifiedText. >>> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:191 >>> + this.modified = modified; >> >> Here, you can still fire the event in this branch. > > What do you mean by that? Oh, I meant that if get modified() was not relying on a member variable, you could still use the same branch for firing the InitialTextModified event.

Created attachment 258623 [details] Patch

Comment on attachment 258623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258623&action=review r=me with a few things noted inline. We can tackle accessibility once it's all glued together. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:94 > + // Reverse the array to allow ensure that splicing the array will not mess with the order. Nit: allow ensure > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:224 > + selectorText += " \u2014 " + mediaText; Add end of line comment: // em-dash. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorTreeItem.js:109 > + get _selectorText() This should either not be a getter, or should be public/protected. I think it would be fine as a public getter.

Created attachment 258639 [details] Patch

Comment on attachment 258639 [details] Patch Clearing flags on attachment: 258639 Committed r188226: <http://trac.webkit.org/changeset/188226>

All reviewed patches have been landed. Closing bug.