|
Description
Harshil Ratnu
2021-06-10 10:07:03 PDT
Created attachment 431163 [details]
WIP v0.0 -First Draft
Created attachment 431883 [details]
Patch v1.0 -First Draft For Review
Created attachment 431903 [details]
A short demo of the project
A screen recording to show the functionality provided by this new documentation tool.
Comment on attachment 431883 [details] Patch v1.0 -First Draft For Review View in context: https://bugs.webkit.org/attachment.cgi?id=431883&action=review I've left quite a few comments below. This is not an exhaustive list of things to look at, but some of these changes will move enough stuff around that notes I could give could very well end up being different after these changes. > Source/WebInspectorUI/ChangeLog:1 > +2021-06-21 Harshil Ratnu <hratnu@apple.com> Please manually break lines around the 120 character mark. > Source/WebInspectorUI/ChangeLog:9 > + Overview: Add contextual documentation for all supported CSS properties in Styles and Computed panel within Web Inspector. > + Details: Add an info button which appears next to the property field on Hover. Clicking on the info button shows a popover with the documentation for the property. Tabbing out or clicking anywhere other than the value field dismisses the popover. For a new feature like this, I like to lay out the architecture here. The information here isn't wrong, but it doesn't paint a full picture for how the feature works internally, only externally to the user. > Source/WebInspectorUI/ChangeLog:53 > + (WI.Popover.prototype.handleEvent): Missing explanation. > Source/WebInspectorUI/ChangeLog:64 > + Add a method - _addInfoButton which gets called for all property fields and also gets called when property name is changed and updated by clicking out of the property name field or tabbing into the next value field. > + Add a method - willDismissPopover to make some changes when the documentation popover is about to dismiss. > + Add a method - getContextualDocumentation which creates an instance of the ContextualDocumentation.js class and presents it. Instead of all at the end, place relevant notes underneath the line they belong to. > Source/WebInspectorUI/UserInterface/Main.html:276 > + <script src= "Views/VSCodeCustomDataCSS.js"></script> This should be placed below in alphabetical order with all the other `Views/`. > Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:60 > +/* commented the above code and changed it below to align the go-to-arrow with the documentation-popover-info-button */ We don't usually leave comments like this in the source. Instead, add an explanation in the changelog for this file. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:25 > + Many of these CSS classes could be more concise and instead rely on the cascade to be applied correctly. For example, instead of `.documentation-popover-info-button-common` and `.documentation-popover-info-button`, combine them into `.documentation-popover-info-button`, and then have another class, .unavailable, with a CSS rule `.documentation-popover-info-button.unavailable` that changes the image. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:67 > +.documentation-popover-name-header{ Ditto :25 The container inside the popover already has a unique class, so these should target things like `.documentation-popover-container > something > .name-header`, instead of being monolithic class names. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:71 > +.documentation-popover-syntax-para{ Ditto :25 > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:78 > +.documentation-popover-syntax-title{ Ditto :25 > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:82 > +.documentation-popover-ref-link{ Ditto :25 > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:123 > +} I think this is a stray `}`? > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:26 > +WI.ContextualDocumentation = class ContextualDocumentation{ NIT: Need a space between class name and `{`. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:34 > + //Static Style: Need a space between `//` and `Static`. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:55 > + if (WI.ContextualDocumentation.valueMap) > + return WI.ContextualDocumentation.valueMap; > + > + WI.ContextualDocumentation.valueMap = {}; > + for(let prop of VSCodeCustomDataCSS.properties){ > + let temp = {} > + > + temp.description = prop.description; > + if(prop.hasOwnProperty("syntax")){ > + temp.syntax = prop.syntax; > + } > + > + if(prop.hasOwnProperty("references")){ > + temp.url = prop.references[0].url; > + } > + > + WI.ContextualDocumentation.valueMap[prop.name] = temp; > + } > + return WI.ContextualDocumentation.valueMap; Why is this necessary? Can we instead of automatically format the VSCode data as part of importing it to be a map keyed by property name instead of an array of objects? > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:63 > + //Public ditto :34 > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:69 > + this._field._documentationInfoDiv = document.createElement("div"); > + this._field._documentationInfoDiv.className = "documentation-popover-container"; Why is `_documentationInfoDiv` a private member of the field? Shouldn't this class hold onto it instead? > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:71 > + this._search(this._field._property.canonicalName,this._field._property.name); Style: Needs a space after the comma. Instead of having a side-effect of search being that it populates some property on field, could `_search` be reworked into something like `_createDocumentationElement(property)` which returns a documentation view for you to then add to the popover. `_property` is not a public member of _field, so it shouldn't be accessed here. If you need it, add a getter for it. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:73 > + this._popover.present(bounds.pad(2), [WI.RectEdge.MIN_Y,WI.RectEdge.MIN_X]); Two questions. First, can we move the pad to above where we assign the bounds? And secondly, why 2? > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:74 > + this._field._valueElement.classList.add("popover-ignore-auto-dismiss"); Please don't access a private member of `_field` this way, instead either add a public getter for `valueElement` or a function to add/remove that class from the applicable elements in `_field`. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:85 > + _search(propertyName,prefixedPropertyName){ See :71 > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:86 > + let obj = WI.ContextualDocumentation.getPropertiesMap() We like to avoid generic object names and abbreviations. Because this value will never change, it can be `const` instead of `let`. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:92 > + if(propertyName in obj) > + property = propertyName; > + else if(prefixedPropertyName in obj){ > + property = prefixedPropertyName; Instead of taking both a prefixed and unprefixed name, we should take the `CSSProperty` here, and use the values we need from it. Style: Needs spaces on outside of parenthesis. Style: No {} needed for single-line if statements. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:95 > + let desc = obj[property].description Please don't abbreviate variable names in cases like this. Acronyms like `URL` are fine, but `desc` should be `description`. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:102 > + _show(name,desc,syntax,url,target) See :71. I'd make this the logic for` _createDocumentationElement(propertyName)`, and then have it call `_search`, or even just roll `_search` into this function, since it would be the only place it is used. Essentially, create a new element at the start of this function, and return it at the end for `present()` to use. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:104 > + let nameHeader = this._field._documentationInfoDiv.appendChild(document.createElement("p")); Elements should generally have `Element` at the end of their name, for example `nameHeaderElement`. In this case, I'd probably call these elements `nameElement`, `descriptionElement`, `syntaxTitleElement`, `syntaxBodyElement`, and `referenceLinkElement`. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:106 > + nameHeader.innerText = name; Please use `textContent` instead of `innerText`, since the rendered appearance of the text is not important while setting the text here. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:110 > + descPara.innerText = desc; Ditto :106 > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:112 > + if(syntax){ Style: Needs a space on either side of `(syntax)`. Style: following lines needs indented until the end of the `if` statement. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:118 > + syntaxTitle.innerText = `${name}`; Ditto :106 > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:125 > + if(url){ Ditto :112 > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:129 > + refLink.innerHTML = `<a href=${url} >Go To MDN Page</a>` > + refLink.innerText = "MDN Reference" Instead of setting the inner HTML, we should set the `refLink.textContent` to the text, and also set the `refLink.href`, instead of adding another `a` tag inside your first one. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:130 > + refLink.href= url; Style: Needs a space before `=` > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:139 > +WI.ContextualDocumentation.valueMap = null; See :37-55 > Source/WebInspectorUI/UserInterface/Views/Main.css:606 > + Oops? > Source/WebInspectorUI/UserInterface/Views/Popover.js:170 > + { Oops? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:-31 > - We should keep this newline. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:-33 > - Ditto :31 > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:107 > + > + Style: Only need one newline between functions > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:108 > + getContextualDocumentation() Naming: maybe `presentContextualDocumentation`? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:166 > + > + Oops? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:199 > + > + Oops? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:212 > + this._addInfoButton() > + > + > + > + > if (this._isEditable() && this._property.enabled) { Style: Only need at most one line between this code. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:528 > + _addInfoButton() I'd love if we could stick to a consistent name here, like `contextualDocumentationButton`, as it makes it clearer that this is part of that feature. This comment applies to variables like the `_infoElement` as well. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:530 > + Style: Unnecessary newline. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:531 > + if(this._infoElement && this._contentElement?.contains(this._infoElement)){ Style: Needs spaces on outside of outer parenthesis. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:534 > + //remove the info button if property name was edited to something invalid and then the user tabbed out or clicked out cause otherwise inspector will crash As I am often reminded, "A comment should be a complete sentence with a period." (and a capital letter at the start). No need to mention the inspector will "crash". At most, outline the condition and action we take. This comment should go above the if (...) statement, as it should describe what is about to happen, and generally not what has happened. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:537 > + if(!this._property.isVariable){ Ditto :531 Instead of a nested if, turn this into an early return e.g. ``` if (this._property.isVariable) return; ``` It will make the code easier to read by not having unnecessary levels of indentation. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:548 > + let isPropertyNameValid = false; > + let isPropertyDocumented = false; > + > + if (WI.CSSCompletions.cssNameCompletions) > + isPropertyNameValid = WI.CSSCompletions.cssNameCompletions.isValidPropertyName(this._property.name); > + > + if(isPropertyNameValid){ > + isPropertyDocumented = WI.ContextualDocumentation.isSupported(this._property.name) || WI.ContextualDocumentation.isSupported(this._property.canonicalName); > + }else{ > + return; > + } I would express all of this different to make it clearer what is going on. ``` if (!WI.CSSCompletions.cssNameCompletions?.isValidPropertyName(this._property.name)) return; let propertyHasDocumentation = WI.ContextualDocumentation.isSupported(this._property.name) || WI.ContextualDocumentation.isSupported(this._property.canonicalName); ``` > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:551 > + if(!this._infoElement){ //this makes sure we don't add a button to line that already had it What if we already have a button, but it's the disabled version? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:553 > + this._infoElement.style.display = "none"; See :583-594 > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:554 > + this._infoElement.setAttribute("title","Click to show documentation") We have a system for localizing strings that you should use for the content of this attribute (and other places above that I've missed). Anywhere there is text the user will see, it needs to be localized for a number of different languages. Search for uses of WI.UIString to see examples of proper usage. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:561 > + event.stopPropagation(); > + event.preventDefault(); Are both these necessary? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:567 > + > + Style: Unnecessary newlines > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:571 > + What if the infoElement already exists? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:577 > + // this._infoElement.disabled = true; Oops? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:578 > + Style: Unnecessary newline. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:594 > + this._contentElement.addEventListener("mouseover",(event)=>{ > + if(this._infoElement){ > + this._infoElement.style.display = "inline-block"; > + } > + }) > + > + > + this._contentElement.addEventListener("mouseout",(event)=>{ > + if(this._infoElement){ > + this._infoElement.style.display = "none"; > + } > + }) This should probably be in CSS, not JS. e.g. ``` .spreadsheet-style-declaration-editor.properties > .property > .content> .documentation-popover-info-button { display: none; } .spreadsheet-style-declaration-editor.properties > .property > .content:hover > .documentation-popover-info-button { display: inline-block; } .content:hover > .documentation-popover-info-button { display: inline-block; } ``` This will also solve the issue that you can end up with more than one event listener doing this attached. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:202 > + { Oops? > Source/WebInspectorUI/UserInterface/Views/VSCodeCustomDataCSS.js:1 > +/* MIT License This entire file really belongs in `External/`. We should connect some time this week, as that will require changes to the build phase of WebInspectorUI to properly include it. Comment on attachment 431883 [details] Patch v1.0 -First Draft For Review View in context: https://bugs.webkit.org/attachment.cgi?id=431883&action=review >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:594 >> + }) > > This should probably be in CSS, not JS. e.g. > ``` > .spreadsheet-style-declaration-editor.properties > .property > .content> .documentation-popover-info-button { > display: none; > } > .spreadsheet-style-declaration-editor.properties > .property > .content:hover > .documentation-popover-info-button { > display: inline-block; > } > > .content:hover > .documentation-popover-info-button { > display: inline-block; > } > ``` > This will also solve the issue that you can end up with more than one event listener doing this attached. Oops - I messed up my example, it should be ``` .spreadsheet-style-declaration-editor.properties > .property > .content> .documentation-popover-info-button { display: none; } .spreadsheet-style-declaration-editor.properties > .property > .content:hover > .documentation-popover-info-button { display: inline-block; } ``` Comment on attachment 431883 [details] Patch v1.0 -First Draft For Review View in context: https://bugs.webkit.org/attachment.cgi?id=431883&action=review >> Source/WebInspectorUI/UserInterface/Main.html:276 >> + <script src= "Views/VSCodeCustomDataCSS.js"></script> > > This should be placed below in alphabetical order with all the other `Views/`. We should use a generic name for the data source. Perhaps something like `ContextualDocumentationDataset` or `ContextualDocumentationDB` The fact that it comes form VSCode is an implementation detail. If we change the contents of this dataset to something else, the filename and references should not need to be changed. >> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:60 >> +/* commented the above code and changed it below to align the go-to-arrow with the documentation-popover-info-button */ > > We don't usually leave comments like this in the source. Instead, add an explanation in the changelog for this file. Yes, code that is not used should not be shipped. Over time, it gets ambiguous why something lingers around. `git blame` can be used to look back though a file's history if someone needs how/when something changed. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:426 > + console.log("trigged blur") Oops? Remove leftover `console.log()` >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:554 >> + this._infoElement.setAttribute("title","Click to show documentation") > > We have a system for localizing strings that you should use for the content of this attribute (and other places above that I've missed). Anywhere there is text the user will see, it needs to be localized for a number of different languages. Search for uses of WI.UIString to see examples of proper usage. The map of localized strings is in `Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js`. A tip: you can replace plain strings in your code with `UI.String()` and the required parameters. Then, run the provided script to automatically update `localizedStrings.js` instead of doing it manually: `/Tools/Scripts/extract-localizable-js-strings --utf8 Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js Source/WebInspectorUI/UserInterface` Comment on attachment 431883 [details] Patch v1.0 -First Draft For Review View in context: https://bugs.webkit.org/attachment.cgi?id=431883&action=review >> Source/WebInspectorUI/ChangeLog:1 >> +2021-06-21 Harshil Ratnu <hratnu@apple.com> > > Please manually break lines around the 120 character mark. OK. Will Remember. >> Source/WebInspectorUI/ChangeLog:53 >> + (WI.Popover.prototype.handleEvent): > > Missing explanation. Just an extra space. Fixed. >> Source/WebInspectorUI/ChangeLog:64 >> + Add a method - getContextualDocumentation which creates an instance of the ContextualDocumentation.js class and presents it. > > Instead of all at the end, place relevant notes underneath the line they belong to. Got it. >>> Source/WebInspectorUI/UserInterface/Main.html:276 >>> + <script src= "Views/VSCodeCustomDataCSS.js"></script> >> >> This should be placed below in alphabetical order with all the other `Views/`. > > We should use a generic name for the data source. Perhaps something like `ContextualDocumentationDataset` or `ContextualDocumentationDB` > > The fact that it comes form VSCode is an implementation detail. > If we change the contents of this dataset to something else, the filename and references should not need to be changed. Yeah that makes sense. The only reason I gave it that name VSCodeCustomDataCSS was for it to match with it's original repository name. >>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:60 >>> +/* commented the above code and changed it below to align the go-to-arrow with the documentation-popover-info-button */ >> >> We don't usually leave comments like this in the source. Instead, add an explanation in the changelog for this file. > > Yes, code that is not used should not be shipped. Over time, it gets ambiguous why something lingers around. > `git blame` can be used to look back though a file's history if someone needs how/when something changed. Noted. Old code removed from this file. Change mentioned in the ChangeLog. >> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:123 >> +} > > I think this is a stray `}`? This is the closing brace for line 112. >> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:55 >> + return WI.ContextualDocumentation.valueMap; > > Why is this necessary? Can we instead of automatically format the VSCode data as part of importing it to be a map keyed by property name instead of an array of objects? I guess we can. But the right time to do that seems to be when we are working on the update script as then we will anyway be processing the VSCodeCustomData file. Currently we are importing that file as it is from the git repository and there is no preprocessing be done on it. >> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:69 >> + this._field._documentationInfoDiv.className = "documentation-popover-container"; > > Why is `_documentationInfoDiv` a private member of the field? Shouldn't this class hold onto it instead? Fixed. >> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:73 >> + this._popover.present(bounds.pad(2), [WI.RectEdge.MIN_Y,WI.RectEdge.MIN_X]); > > Two questions. First, can we move the pad to above where we assign the bounds? And secondly, why 2? first: what is the right syntax to do that? second: pad = 2 because that is the perfect padding which makes sure that when the popover appears on top/bottom of the property name, it doesn't point >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:551 >> + if(!this._infoElement){ //this makes sure we don't add a button to line that already had it > > What if we already have a button, but it's the disabled version? "disable" version is also handled in this case as it also is an infoElement just with a different background image >>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:554 >>> + this._infoElement.setAttribute("title","Click to show documentation") >> >> We have a system for localizing strings that you should use for the content of this attribute (and other places above that I've missed). Anywhere there is text the user will see, it needs to be localized for a number of different languages. Search for uses of WI.UIString to see examples of proper usage. > > The map of localized strings is in `Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js`. > > A tip: you can replace plain strings in your code with `UI.String()` and the required parameters. > Then, run the provided script to automatically update `localizedStrings.js` instead of doing it manually: > > `/Tools/Scripts/extract-localizable-js-strings --utf8 Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js Source/WebInspectorUI/UserInterface` Okay. Will fix it in a future patch. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:561 >> + event.preventDefault(); > > Are both these necessary? Only works properly if both of these events are stopped. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:571 >> + > > What if the infoElement already exists? accounted for by 531-534 >> Source/WebInspectorUI/UserInterface/Views/VSCodeCustomDataCSS.js:1 >> +/* MIT License > > This entire file really belongs in `External/`. We should connect some time this week, as that will require changes to the build phase of WebInspectorUI to properly include it. Okay. Will fix it in a future patch. Created attachment 432130 [details]
Patch v1.1 -Updated Styling, Uniform Nomenclature, Removed Nested Conditionals
Comment on attachment 432130 [details] Patch v1.1 -Updated Styling, Uniform Nomenclature, Removed Nested Conditionals View in context: https://bugs.webkit.org/attachment.cgi?id=432130&action=review > Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:3 > +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1" viewBox="0 0 10 10"> 10×10 seems small. We normally have 15×15 or 16×16 icons. Why so small? > Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:18 > + fill: hsla(0, 0%, 0%, 0.5); Does this look good in dark mode? > Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:44 > + Style: unnecessary newline > Source/WebInspectorUI/UserInterface/Main.html:681 > + <script src="Views/ContextualDocumentationDB.js"></script> Where is this file? > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:32 > +.documentation-popover p { We generally try to use immediate descendant combinators wherever possible to lessen the likelihood that future changes will clash with past changes. As such, can this be `.documentation-popver > p`? ditto with other selectors below > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:33 > + margin-bottom: 5px !important; We try to avoid using `!important` unless there is literally no other choice (and even then usually there is). Please rework to not use `!important`. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:47 > + color:var(--syntax-highlight-boolean-color); Style: missing space after `:` > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:76 > + top: 1.3px; This seems oddly specific. In almost all cases CSS doesn't really care about anything other than `0.5` increments. Is there a reason you need `0.3` exactly? Maybe a `vertical-align: middle;` (or something along those lines) would work instead? > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:101 > +:focus .selected .contextual-documentation-button, > +.selected:focus .contextual-documentation-button { Please be more specific as to the context of `:focus` and `.selected`. Is it a `.property` or a `.tree-element` or what? We don't want general CSS selectors like this because if a `.contextual-documentation-button` is added to other parts of Web Inspector that have a radically different structure it would cause (potentially undiscovered) problems. A general rule of thumb is to only include styles that apply to the node in isolation in that node's file (e.g. `.contextual-documentation-button` relates to `ContextualDocumentation.css`) and have more specific usage styles in the CSS file corresponding to that usage (e.g. `SpreadsheetStyleProperty.css`). > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:111 > + Style: unnecessary newline > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:118 > + filter: unset; We generally try to avoid using `unset` and instead prefer to just not set the style in the first place (e.g. only applying `filter: invert();` when not `.selected:focus`). > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:120 > + Style: unnecessary newline > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:27 > + Style: unnecessary newline > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:34 > + // Static Style: needs four spaces for indentation > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:42 > + for (let prop of VSCodeCustomDataCSS.properties) { Can we format this data ahead of time when it's imported into the WebKit repo? I doubt it would take much python to do it. Also, we should call this something else. Where the data comes from is not important. Ideally, if you do pre-formatting, you could just have the result be output into a file that contains `WI.ContextualDocumentation.propertyMap = new Map([ ... ]);` > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:43 > + let temp = {} Style: missing `;` > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:46 > + if (prop.hasOwnProperty("syntax")) { Style: no `{` or `}` for single line control flow > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:50 > + if (prop.hasOwnProperty("references")) { ditto (:46) > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:73 > + this._popover.present(bounds.pad(2), [WI.RectEdge.MIN_Y,WI.RectEdge.MIN_X]); Style: missing space after `,` > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:74 > + this._field.valueElement.classList.add("popover-ignore-auto-dismiss"); It's really odd that this is added in this file but then removed in another file. That's probably not going to be clear to future engineers. Why not just have this inside `WI.SpreadsheetStyleProperty.prototype.presentContextualDocumentation`? This would also remove the need to expose `get valueElement`, which is kinda an implementation detail of `WI.SpreadsheetStyleProperty`. Also, please use the named constant for this `WI.Popover.IgnoreAutoDismissClassName`. Most importantly tho, this also means that clicking outside of the `WI.Popover` or scrolling will prevent it from dismissing. Is that really what we want? > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:78 > + dismiss(){ Style: `{` on new line > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:86 > + let details = {}; Style: please move this further down closer to where it's actually used > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:87 > + const propertyMap = WI.ContextualDocumentation.getPropertiesMap() ditto (:43) > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:98 > + details.description = propertyMap[propertyName].description > + details.syntax = propertyMap[propertyName].syntax > + details.url = propertyMap[propertyName].url ditto (:43) > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:107 > + let nameElement = documentationElement.appendChild(document.createElement("p")); Style: extra space > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:112 > + descriptionElement.className = "description-para"; Style: we don't abbreviate names like this Also, no need to add a class if it's not used. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:117 > + let syntaxPara = documentationElement.appendChild(document.createElement("p")); > + syntaxPara.className = "syntax-para"; ditto (:112) > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:121 > + syntaxTitleElement.textContent = `${details.name}`; No need for a template string here. You can just use ` = details.name;``. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:124 > + syntaxBodyElement.className ="syntax-body"; No need to add a class if it's not used. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:125 > + syntaxBodyElement.textContent = `: ${details.syntax}`; Why two spaces after the `:`? Also, based on the demo video (attachment 431903 [details]), I'm not sure what value the actual CSS syntax would have for someone working with CSS. To use `display` as an example, I don't think most CSS developers know what `<display-outside>` actually means and would likely immediately jump to MDN to find that answer. If the goal of this is to help developers learn/understand CSS without having to leave Web Inspector, I think we should include definitions for these other keywords here too or even entirely replace the keyword with its definition inline. Some things we could probably keep (e.g. `<length>`, `<percentage>`, etc.) but I think most of these kinds of specific keywords would not really be all that useful. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:132 > + referenceLinkElement.className = "reference-link" > + referenceLinkElement.href = details.url > + referenceLinkElement.textContent = "MDN Reference" ditto (:43) > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:137 > + Style: unnecessary newline > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:138 > +} Style: missing `;` > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:140 > +WI.ContextualDocumentation.propertyMap = null; Since nobody else is supposed to access `WI.ContextualDocumentation.propertyMap`, please make it "private" by prefixing with a `_`. ``` WI.ContextualDocumentation._propertyMap = null; ``` > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:109 > + presentContextualDocumentation() This can be "private" (add a `_` prefix and move to the `// Private` section) since it's not used outside of this object. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:315 > - let propertyNameIsValid = false; > + let isPropertyNameValid = false; These kinds of changes don't really add anything and just make it harder to track down regressions by adding another `git blame` hop. Please undo. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:417 > + { Style: unnecessary spaces > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:524 > + this._contentElement.removeChild(this._contextualDocumentationButton) Style: missing `;` > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:529 > + return ditto (:524) > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:531 > + if (!WI.CSSCompletions.cssNameCompletions?.isValidPropertyName(this._property.name)) I don't think it's possible for `WI.CSSCompletions.cssNameCompletions` to still be `null` by this point as it's one of the first bits of data requested by Web Inspector from the inspected page. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:535 > + Style: unnecessary newline > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:540 > + this._contextualDocumentationButton.addEventListener("mousedown",(event)=>{ Style: missing space after `,` Style: missing spaces before and after `=>` > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:541 > + ditto (:535) > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:542 > + if(this._valueTextField?.editing){ Style: missing space after `if` Style: missing space before `{` > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:548 > + this.presentContextualDocumentation(true); Why the `true`? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:549 > + }) ditto (:524) > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:550 > + ditto (:417) > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:553 > + this._contextualDocumentationButton.setAttribute("title","Documentation not available") You can just do `.title = `. This should be a `WI.UIString` so it can be localized for other languages. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:555 > + this._contextualDocumentationButton.classList.add("contextual-documentation-button"); > + this._contextualDocumentationButton.classList.add("unavailable"); Since you just created this element, you can just use `className` with a manually space separated string. ``` this._contextualDocumentationButton.className = "contextual-documentation-button unavailable"; ``` FYI `classList.add` supports multiple arguments at once Comment on attachment 432130 [details] Patch v1.1 -Updated Styling, Uniform Nomenclature, Removed Nested Conditionals View in context: https://bugs.webkit.org/attachment.cgi?id=432130&action=review >> Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:3 >> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1" viewBox="0 0 10 10"> > > 10×10 seems small. We normally have 15×15 or 16×16 icons. Why so small? Since the ContextualDocumentationButton is in-line with the rest of the text it's size needs to be restricted to be so small. If these icons are any bigger then when the user hovers on top of any line where they should appear, that makes the particular line pop-up a little visually as it gets a little extra height. >> Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:18 >> + fill: hsla(0, 0%, 0%, 0.5); > > Does this look good in dark mode? Yes the styling is made the same as go-to-arrow button in the computed panel and it works with dark mode equally well. >> Source/WebInspectorUI/UserInterface/Main.html:681 >> + <script src="Views/ContextualDocumentationDB.js"></script> > > Where is this file? In this patch: didn't show up sorry. Will add to next version of this patch. Originally: Git Repository - VSCodeCustomDataCSS Locally:Should be present in the External Folder in a future patch. Working on it. >> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:33 >> + margin-bottom: 5px !important; > > We try to avoid using `!important` unless there is literally no other choice (and even then usually there is). Please rework to not use `!important`. Done. Used a more specific rule. >> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:76 >> + top: 1.3px; > > This seems oddly specific. In almost all cases CSS doesn't really care about anything other than `0.5` increments. Is there a reason you need `0.3` exactly? Maybe a `vertical-align: middle;` (or something along those lines) would work instead? I did not know the 0.5 increment rule. 1.3 makes it perfectly inline but in 1.5 either the difference isn't noticeable hence changed to that for now. >> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:101 >> +.selected:focus .contextual-documentation-button { > > Please be more specific as to the context of `:focus` and `.selected`. Is it a `.property` or a `.tree-element` or what? We don't want general CSS selectors like this because if a `.contextual-documentation-button` is added to other parts of Web Inspector that have a radically different structure it would cause (potentially undiscovered) problems. A general rule of thumb is to only include styles that apply to the node in isolation in that node's file (e.g. `.contextual-documentation-button` relates to `ContextualDocumentation.css`) and have more specific usage styles in the CSS file corresponding to that usage (e.g. `SpreadsheetStyleProperty.css`). A lot of this code was copies over from go-to-arrow button. In the next patch this section has been cleared significantly to leave only the relevant code. >> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:42 >> + for (let prop of VSCodeCustomDataCSS.properties) { > > Can we format this data ahead of time when it's imported into the WebKit repo? I doubt it would take much python to do it. > > Also, we should call this something else. Where the data comes from is not important. Ideally, if you do pre-formatting, you could just have the result be output into a file that contains `WI.ContextualDocumentation.propertyMap = new Map([ ... ]);` Working on preprocessing data that will be part of the update script(to update this database from time to time) in a future patch. >> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:74 >> + this._field.valueElement.classList.add("popover-ignore-auto-dismiss"); > > It's really odd that this is added in this file but then removed in another file. That's probably not going to be clear to future engineers. Why not just have this inside `WI.SpreadsheetStyleProperty.prototype.presentContextualDocumentation`? This would also remove the need to expose `get valueElement`, which is kinda an implementation detail of `WI.SpreadsheetStyleProperty`. > > Also, please use the named constant for this `WI.Popover.IgnoreAutoDismissClassName`. > > Most importantly tho, this also means that clicking outside of the `WI.Popover` or scrolling will prevent it from dismissing. Is that really what we want? Beautiful Suggestion! This is now handled in WI.SpreadsheetStyleProperty.prototype.presentContextualDocumentation and the getter has been removed. The behavior is such that the popover will only persist if the user clicked in the value field of that property which is what we want so that the user has reference to the syntax from the popover while editing the value. Clicking anywhere else will dismiss the popover. >> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:125 >> + syntaxBodyElement.textContent = `: ${details.syntax}`; > > Why two spaces after the `:`? > > Also, based on the demo video (attachment 431903 [details]), I'm not sure what value the actual CSS syntax would have for someone working with CSS. To use `display` as an example, I don't think most CSS developers know what `<display-outside>` actually means and would likely immediately jump to MDN to find that answer. If the goal of this is to help developers learn/understand CSS without having to leave Web Inspector, I think we should include definitions for these other keywords here too or even entirely replace the keyword with its definition inline. Some things we could probably keep (e.g. `<length>`, `<percentage>`, etc.) but I think most of these kinds of specific keywords would not really be all that useful. two spaces: typo sorry. implementation detail: You are right the syntax in some cases is too complicated to be useful to the everyday user. However with the limited time that we have(Internship ends next week) in this and the limitations that the current data source presents, we cannot possible change the documentation quality. This would however be a great update in the future when the documentation quality can be given more time. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:548 >> + this.presentContextualDocumentation(true); > > Why the `true`? Old Design. Fixed in next patch. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:553 >> + this._contextualDocumentationButton.setAttribute("title","Documentation not available") > > You can just do `.title = `. > > This should be a `WI.UIString` so it can be localized for other languages. Localization added in next patch. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:555 >> + this._contextualDocumentationButton.classList.add("unavailable"); > > Since you just created this element, you can just use `className` with a manually space separated string. > ``` > this._contextualDocumentationButton.className = "contextual-documentation-button unavailable"; > ``` > FYI `classList.add` supports multiple arguments at once Thanks! Fixed. Created attachment 432292 [details]
Patch v1.2 -Localizatiion, Database moved in External Folder, Other refactoring and styling changes
Comment on attachment 432292 [details] Patch v1.2 -Localizatiion, Database moved in External Folder, Other refactoring and styling changes View in context: https://bugs.webkit.org/attachment.cgi?id=432292&action=review > Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:186 > +my $shouldCombineMain = 1; #defined $ENV{'COMBINE_INSPECTOR_RESOURCES'} && ($ENV{'COMBINE_INSPECTOR_RESOURCES'} eq 'YES'); oops > Source/WebInspectorUI/UserInterface/External/ContextualDocumentationDatabase/ContextualDocumentationDatabase.js:47 > + "syntax": "normal | <baseline-position> | <content-distribution> | <overflow-position>? <content-position>", I'm still not convinced that this is a good approach. Perhaps for now we should remove this (or put it behind an experimental feature) until we can come up with a better way to express this information. > Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:3 > +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1" viewBox="0 0 10 10"> My point about this being small was more about the fact that other icons used in the Styles panel are 15×15 (e.g. `CubicBezier.svg`). > Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:59 > - position: absolute; > - bottom: 0; > - width: var(--disclosure-button-size); > - height: var(--disclosure-button-size); > + width: 10px; > + height: 10px; > + position: relative; > + z-index: 1; > + top: 1.5px; > + margin-left: 2px; Why did this change? > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:55 > +:matches(.spreadsheet-style-declaration-editor.properties, .computed-property-item) > .property > .content> .contextual-documentation-button { Do we actually need `:matches(.spreadsheet-style-declaration-editor.properties, .computed-property-item) > .property > .content >`? `.contextual-documentation-button` is already a unique name, so there shouldn't be any need for more a more specific selector. What I was trying to describe earlier was that if you have any _additional_ modifications from the base style that are specific for some other circumstance, those modifications should go in the file most closely related to that. To give an example here, the base style `.contextual-documentation-button` shouldn't have any `display` and instead you should move those styles to `Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleDeclarationEditor.css` and/or `Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css` so that if we decide to create a `WI.ContextualDocumentationButton` that's _always_ visible elsewhere in Web Inspector then these styles won't be relevant/necessary. Style: space before `>` > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:64 > + background-color: transparent; > + background-repeat: no-repeat; > + background-position: center; > + background-size: 10px 10px; You can replace all of these with just a `background: none`. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:69 > + top: 1.5px; Does this work on 1x/non-retina screens? I don't think it will, so I'd recommend making this ``` width: 11px; height: 11px; top: 1px; vertical-align: -1px; ``` That's similar to what we do with other things. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:73 > +:matches(.spreadsheet-style-declaration-editor.properties, .computed-property-item) > .property > .content:hover> .contextual-documentation-button { Can we make this so that hovering over the empty space next to the property also shows the button? I think that's possible by moving the `.content:hover` to `.property:hover` instead. ditto (:73) > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:77 > +:matches(.spreadsheet-style-declaration-editor.properties, .computed-property-item) > .property > .content> .contextual-documentation-button:active { ditto (:73) > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:85 > +:matches(.spreadsheet-style-declaration-editor.properties, .computed-property-item) > .property > .content> .contextual-documentation-button.unavailable { ditto (:73) > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:94 > + .contextual-documentation-button{ Style: space before `{` > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:26 > +WI.ContextualDocumentation = class ContextualDocumentation { This needs a better name that clarifies what it does, like `WI.ContextualDocumentationPopover`. Right now, this reads like a model object, not a view object. Style: `{` on new line > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:29 > + this._field = delegate; This should be called `_delegate` to match the name of the parameter. Also, as a general piece of feedback, `_field` is too generic and doesn't really explain what is expected. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:41 > + for (let prop of ContextualDocumentationDatabase.properties) { I thought there was a plan to preprocess this? > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:88 > + if (property.canonicalName in propertyMap) > + propertyName = property.canonicalName; > + else if (property.name in propertyMap) > + propertyName = property.name; What happens if neither is in the `propertyMap`? Or is that not a possible path (if so we should `console.assert(propertyName);`)? > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:112 > + let syntaxPara = documentationElement.appendChild(document.createElement("p")); > + syntaxPara.className = "syntax"; Style: we don't abbreviate variable names like this > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:126 > + referenceLinkElement.textContent = "MDN Reference"; this should probably be a `WI.UIString` > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:515 > + // This makes sure we don't add a button to line that already had it. This comment doesn't add any value if one just reads the below code. I'd remove it. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:519 > + if (this._contextualDocumentationButton && this._contentElement?.contains(this._contextualDocumentationButton)) { > + this._contentElement.removeChild(this._contextualDocumentationButton); > + this._contextualDocumentationButton = null; > + } You can simplify this. ``` if (this._contextualDocumentationButton) { this._contextualDocumentationButton.remove(); this._contextualDocumentationButton = null; } ``` > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:524 > + if (!WI.CSSCompletions.cssNameCompletions.isValidPropertyName(this._property.name)) Do we also want to check `this._property.canonicalName`? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:528 > + this._contextualDocumentationButton = this._contentElement.appendChild(document.createElement("button")); I just realized that `.contextual-documentation-button` is not actually related to a `WI.ContextualDocumentationButton` and is just something that's created for this class. As such, you should not have a special `Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css` file just for these styles. All those styles should really be inside `Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleDeclarationEditor.css` and/or `Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css` as that's where the other `.spreadsheet-style-property` styles are. We should keep CSS close together for JS that's similarly close together. Also, since this happens in both the `if` and `else`, I'd pull this out above the `if` to avoid repeated code. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:530 > + this._contextualDocumentationButton.className = "contextual-documentation-button"; ditto (:528) > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:531 > + this._contextualDocumentationButton.addEventListener("mousedown", (event) => { Why not `"click"`? Also please make sure this is only for left-click as right now a right-click will trigger this too. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:541 > + this._contextualDocumentationButton = this._contentElement.appendChild(document.createElement("button")); ditto (:528) > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:542 > + this._contextualDocumentationButton.title = WI.UIString("Documentation not available", "Tooltip for unavailable documentation @ Contextual Documentation Popup"); This isn't really very helpful for a developer. Why are we showing a button at all if we don't have documentation for it? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:543 > + this._contextualDocumentationButton.className = "contextual-documentation-button unavailable"; based on my comment on line 528, you'd want to change this to `.classList.add("unavailable")` > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:549 > + this._contextualDocumentation = new WI.ContextualDocumentation(this); This needs a better name (like `this._contextualDocumentationPopover`) just like how `WI.ContextualDocumentation` needs a better name. This means we create a new `this._contextualDocumentationPopover` each time the developer clicks on the `this._contextualDocumentationButton`. I think you should use `this._contextualDocumentation ??= new WI.ContextualDocumentation(this);` so that we only create once. Comment on attachment 432292 [details] Patch v1.2 -Localizatiion, Database moved in External Folder, Other refactoring and styling changes View in context: https://bugs.webkit.org/attachment.cgi?id=432292&action=review >> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:186 >> +my $shouldCombineMain = 1; #defined $ENV{'COMBINE_INSPECTOR_RESOURCES'} && ($ENV{'COMBINE_INSPECTOR_RESOURCES'} eq 'YES'); > > oops Yup. Fixed. >> Source/WebInspectorUI/UserInterface/External/ContextualDocumentationDatabase/ContextualDocumentationDatabase.js:47 >> + "syntax": "normal | <baseline-position> | <content-distribution> | <overflow-position>? <content-position>", > > I'm still not convinced that this is a good approach. Perhaps for now we should remove this (or put it behind an experimental feature) until we can come up with a better way to express this information. Sorry I still would like to defend this feature. This is the same syntax VS Code shows in their IDE and also the same syntax that Firefox used to show in their Nightly Browser at some point. To the developers who would understand complex syntax values it still means something and provides valueable quick reference. And in cases where it is not that useful there is always the option to click on MDN Reference to go to the actual page and read more. Moreover there are a lot of scernarios where the syntax is really useful and hence we should focus on those upsides. No Documentation is perfect but we still must provide whatever information we can and hopefully one day we can improve this documentation quality and makes it easier to understand. In the time being, if we realize via telemetry or other sources that syntax is not a helpful field to our users, we can always just comment out the line in code responsible for adding it to the popover. I do not favor removing it entirely from this feature as this has been a core part of this project and everyone in the process of development and demos has shown keen interest in this detail. >> Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:3 >> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1" viewBox="0 0 10 10"> > > My point about this being small was more about the fact that other icons used in the Styles panel are 15×15 (e.g. `CubicBezier.svg`). GoToArrow.svg has the same size and since this sits right next to the go-to-arrow in computed panel it was made the same. >> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:59 >> + margin-left: 2px; > > Why did this change? Go to Arrow was not in line with the text in computed panel, It was slightly higher vertically >> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:55 >> +:matches(.spreadsheet-style-declaration-editor.properties, .computed-property-item) > .property > .content> .contextual-documentation-button { > > Do we actually need `:matches(.spreadsheet-style-declaration-editor.properties, .computed-property-item) > .property > .content >`? `.contextual-documentation-button` is already a unique name, so there shouldn't be any need for more a more specific selector. > > What I was trying to describe earlier was that if you have any _additional_ modifications from the base style that are specific for some other circumstance, those modifications should go in the file most closely related to that. To give an example here, the base style `.contextual-documentation-button` shouldn't have any `display` and instead you should move those styles to `Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleDeclarationEditor.css` and/or `Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css` so that if we decide to create a `WI.ContextualDocumentationButton` that's _always_ visible elsewhere in Web Inspector then these styles won't be relevant/necessary. > > Style: space before `>` Do we need :matches(.spreadsheet-style-declaration-editor.properties, .computed-property-item) ? If we are okay specifying It as .property > .content> .contextual-documentation-button { ... } then we don't. We can also separate them out in their individual files like ```` in SpreadsheetCSSStyleDeclarationEditorcss .spreadsheet-style-declaration-editor.properties > .property > .content > .contextual-documentation-button { display: none; } in ComputedStylesSection.css .computed-property-item > .property > .content > .contextual-documentation-button { display: none; } ``````````` However in that case any change in behavior for this button will have to be made in both files always. To avoid that I have placed this code entirely in SpreadsheetCSSStyleDeclarationEditor.css for now. Kindly let me know if you would have it otherwise. >> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.css:69 >> + top: 1.5px; > > Does this work on 1x/non-retina screens? I don't think it will, so I'd recommend making this > ``` > width: 11px; > height: 11px; > top: 1px; > vertical-align: -1px; > ``` > That's similar to what we do with other things. Increasing the size more than 10*10 makes the hover action jumpy. >> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:41 >> + for (let prop of ContextualDocumentationDatabase.properties) { > > I thought there was a plan to preprocess this? Planning on doing that in the next patch. Update Script was taking some time and I wanted feedback on other things in the meantime. >> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:88 >> + propertyName = property.name; > > What happens if neither is in the `propertyMap`? Or is that not a possible path (if so we should `console.assert(propertyName);`)? Its not a possible path as then the addInfoButton would not have appeared and we wouldn't have searched for the property to begin with. >> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentation.js:126 >> + referenceLinkElement.textContent = "MDN Reference"; > > this should probably be a `WI.UIString` The link leads to untranslated documentation on MDN. Likewise, all of the tooltip content is in English. This sets the expectation for the user that they’re not going to get something in their own language at the destination. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:524 >> + if (!WI.CSSCompletions.cssNameCompletions.isValidPropertyName(this._property.name)) > > Do we also want to check `this._property.canonicalName`? I believe the function isValidPropertyName doesn't need that check and already accounts for that as this is the same conditional that we use for painting the warning sign at the end of line if the propertyNameIsNotValid >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:542 >> + this._contextualDocumentationButton.title = WI.UIString("Documentation not available", "Tooltip for unavailable documentation @ Contextual Documentation Popup"); > > This isn't really very helpful for a developer. Why are we showing a button at all if we don't have documentation for it? Maciej requested this. Originally we weren't showing any button at all for the properties where we do not have a documentation and where the property name was invalid. But Maciej wanted us to still have a button if the property was valid but we did not have it's documentation so as to differentiate the behavior and preventing the user from thinking that they did a mistake hence they can't see a button. He then in the second demo looked over this design and approved it. Created attachment 432421 [details]
Patch v1.3 - preprocessed database, refactored and moved code to right files,renamed files
Comment on attachment 432421 [details] Patch v1.3 - preprocessed database, refactored and moved code to right files,renamed files View in context: https://bugs.webkit.org/attachment.cgi?id=432421&action=review > Source/WebInspectorUI/ChangeLog:19 > + Add Localizations for tooltip strings that are exposed to the user It generally isn't necessary for /every/ file and function to have notes if note would be easily inferred by a future reader. In this case, changes to the localized strings file isn't really noteworthy, as the strings also appear below in your code, where they are more important. You can get rid of this note (but keep the file listed here). > Source/WebInspectorUI/ChangeLog:22 > + Add details for the ContextualDocumentationDatabase from External Folder to build properly Much like comments, it is generally preferable that these notes are complete sentences. I think for most of these, you are just missing the ending period. This note applied throughout the change log. I won't note it on every line to avoid being overly redundant. > Source/WebInspectorUI/ChangeLog:25 > + Add documentation of all css properties in a JSON object. This file is sourced from VSCodeCustomData NIT: CSS is a capitalized acronym. > Source/WebInspectorUI/ChangeLog:29 > + Add the LICENSE File for VSCode's data ditto :19 > Source/WebInspectorUI/ChangeLog:32 > + Add a file for the new info button ditto :19 > Source/WebInspectorUI/ChangeLog:35 > + Link the new files added to be loaded from Main.html ditto :19 > Source/WebInspectorUI/ChangeLog:43 > + Add styling specific to the documentation buttons of Computed Panel Style: Only need one space between `specific` and `to`. > Source/WebInspectorUI/ChangeLog:52 > + Define styling for content within the documentation popover ditto :19 - Unlike the targeted changes you made to existing stylesheets above that warranted some explanation, in my opinion it is inferred by the naming of the file and selectors what this spreadsheet will do. > Source/WebInspectorUI/ChangeLog:68 > + Add all basics styles for the contextualDocumentationButton ditto :52 > Source/WebInspectorUI/ChangeLog:84 > + Responsible for adding the info button add the end of the property. NIT: `... next value field and is responsible for adding...`? > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:327 > +/* Tooltip to show documentation @ Contextual Documentation Popup */ > +localizedStrings["Click to show documentation"] = "Click to show documentation"; Generally the key for the string will be in the format of `Text @ Location in interface`, e.g. `Click to show documentation @ Contextual Documentation Popover Button`. The comment for the string can then be a bit more freeform e.g. `Tooltip for the button to show contextual documentation for CSS properties.` > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:506 > +/* Tooltip for unavailable documentation @ Contextual Documentation Popup */ > +localizedStrings["Documentation not available"] = "Documentation not available"; ditto :326-327 > Source/WebInspectorUI/UserInterface/External/ContextualDocumentationDatabase/ContextualDocumentationDatabase.js:2 > + "-moz-animation": { Do we need the `-moz-`, `-ms-`, and `-o-` prefixed properties since I don't believe we would ever show documentation for those, only the standard documentation? I think `-webkit-` is the only prefix you need. > Source/WebInspectorUI/UserInterface/External/ContextualDocumentationDatabase/ContextualDocumentationDatabase.js:691 > + "-webkit-background-composite": {}, Is there any value in keeping empty properties in the processed data, since we shouldn't show documentation in this case anyways? If not, let's remove it as well. (I came back here after finishing review the rest, and it does look like this will be an issue currently. I'd recommend removing empty properties entirely so you correctly don't show documentation for them.) > Source/WebInspectorUI/UserInterface/External/ContextualDocumentationDatabase/ContextualDocumentationDatabase.js:3248 > +}; Thank you for alphabetizing these entries! > Source/WebInspectorUI/UserInterface/Main.html:93 > + <link rel="stylesheet" href="Views/ContextualDocumentationPopover.css"> Style: Please place this in alphabetical order. > Source/WebInspectorUI/UserInterface/Main.html:682 > + <script src="Views/ContextualDocumentationPopover.js"></script> ditto :93 > Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:55 > - width: var(--disclosure-button-size); > - height: var(--disclosure-button-size); > + width: 10px; > + height: 10px; Why are we making this smaller than the 15px it used to be? It feels like if this button fit, the new on should fit as well at the same height? > Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:57 > + z-index: 1; Why? > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:30 > + this._delegate = delegate; I would add a `console.assert(delegate)` at the start of this constructor, since it appears to be required, and other things called delegates are sometimes optional in WI. Honestly, I'm not sure delegate is the right word for this case. If the delegate were providing the popover's contents, it might make sense, but as it stands it's not really a delegate. Not sure what I would call it right this moment... > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:39 > + let bounds = WI.Rect.rectFromClientRect(this._delegate._nameElement.getBoundingClientRect()); If you want to access the nameElement, you need to make it public, as right now you are accessing a private member from a different class. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:53 > + _searchDocumentationDetails(property) NIT: `_getDocumentationDetails(property)`? > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:66 > + details.description = ContextualDocumentationDatabase[propertyName].description; > + details.syntax = ContextualDocumentationDatabase[propertyName].syntax; > + details.url = ContextualDocumentationDatabase[propertyName].url; I think it's possible to throw an exception here if a valid property name made it here that wasn't in the database. I would change each of these to `ContextualDocumentationDatabase[propertyName]?.description` and so on so that if the property doesn't exist for some reason, you get back something nullish, instead of throwing an exception accessing a property of something that is potentially undefined. Alternatively, add a check before you `let details = {}` above that checks of the property exists in the database first, just in case. e.g. ``` if (!ContextualDocumentationDatabase[propertyName]) return {}; ``` Additionally, if you believe it should be impossible to get to this point with a property name that isn't in the documentation, please add an assert above the previously suggested if to assert it. e.g. `console.assert(ContextualDocumentationDatabase[propertyName]`). We like to use asserts anytime we make this type of assumption, so that if we make code changes later that breaks the assumption, we realize it (hopefully) before a customer does. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:80 > + let descriptionElement = documentationElement.appendChild(document.createElement("p")); > + descriptionElement.textContent = details.description; You should either handle properties without a description, or remove such properties from the database file. I'd opt for the second option. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:98 > + referenceLinkElement.href = details.url; > + referenceLinkElement.textContent = "MDN Reference"; NIT: I'd reverse these two lines to better match the order you do the other elements in. > Source/WebInspectorUI/UserInterface/Views/Main.css:345 > + width: 10px; > + height: 10px; ditto ComputedStylesDetailsPanel.js:54-55 > Source/WebInspectorUI/UserInterface/Views/Main.css:348 > + top: 1.5px; I feel like Devin had a suggestion in his first review pass regarding vertical alignment without an explicit `top`. Did you try it? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:528 > + if (this._property.name in ContextualDocumentationDatabase || this._property.canonicalName in ContextualDocumentationDatabase) { I actually had to look this one up; I wasn't familiar with the `in` keyword in this context. Can we change these to `ContextualDocumentationDatabase.hasOwnProperty(this._property.name)` and the sane for `canonicalName` to make sure we don't accidentally inherit something that looks like a CSS property from the Object prototype. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:529 > + this._contextualDocumentationButton.title = WI.UIString("Click to show documentation", "Tooltip to show documentation @ Contextual Documentation Popup"); See my note of strings near the top of this review... > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:538 > + this._contextualDocumentationButton.addEventListener("click", (event) => { > + if (this._valueTextField?.editing) { > + event.stopPropagation(); > + event.preventDefault(); > + this._valueTextField.discardCompletion(); > + } > + > + this._presentContextualDocumentation(); > + }); Not 100% sure on this, but I'm not immediately aware of other places where we inline event listeners like this. Instead, can you add a `_handleContextualDocumentationButtonClicked(event)` function to this class, and then here just do `this._contextualDocumentationButton.addEventListener("click", this._handleContextualDocumentationButtonClicked.bind(this));` > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:540 > + this._contextualDocumentationButton.title = WI.UIString("Documentation not available", "Tooltip for unavailable documentation @ Contextual Documentation Popup"); ditto :529 Comment on attachment 432421 [details] Patch v1.3 - preprocessed database, refactored and moved code to right files,renamed files View in context: https://bugs.webkit.org/attachment.cgi?id=432421&action=review > Source/WebInspectorUI/UserInterface/External/ContextualDocumentationDatabase/ContextualDocumentationDatabase.js:1 > +ContextualDocumentationDatabase = { Are we planning to upload the script that creates this too? If not, why not? >> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:55 >> + height: 10px; > > Why are we making this smaller than the 15px it used to be? It feels like if this button fit, the new on should fit as well at the same height? Yeah. Also, AFAICT this is unrelated to this change, so please undo this and make it into a separate bug/patch. > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.css:32 > +.popover .documentation-popover > p { Is the `.popover` needed? >> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:98 >> + referenceLinkElement.textContent = "MDN Reference"; > > NIT: I'd reverse these two lines to better match the order you do the other elements in. Also, if you're not going to have things in the UI that do to NOT want to localize then please use `WI.unlocalizedString`. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:95 > +:matches(.spreadsheet-style-declaration-editor.properties, .computed-property-item) > .property:hover > .content> .contextual-documentation-button { ditto (:91) Also, I think you can combine this with the above by using ``` .spreadsheet-style-declaration-editor > .property:not(:hover) > .content > .contextual-documentation-button { display: none; } ``` Comment on attachment 432421 [details] Patch v1.3 - preprocessed database, refactored and moved code to right files,renamed files View in context: https://bugs.webkit.org/attachment.cgi?id=432421&action=review >> Source/WebInspectorUI/UserInterface/External/ContextualDocumentationDatabase/ContextualDocumentationDatabase.js:1 >> +ContextualDocumentationDatabase = { > > Are we planning to upload the script that creates this too? If not, why not? Yes we will upload the script to create this too, however in a different patch as that process is halted because our teammate who handles script is on vacation and my internship ends soon. So even though the script is ready it is prematures and needs to be integrated with the web inspector in the right place. Hence I will pass down the script to the team to integrate. >> Source/WebInspectorUI/UserInterface/External/ContextualDocumentationDatabase/ContextualDocumentationDatabase.js:2 >> + "-moz-animation": { > > Do we need the `-moz-`, `-ms-`, and `-o-` prefixed properties since I don't believe we would ever show documentation for those, only the standard documentation? I think `-webkit-` is the only prefix you need. yes currently we only support -webkit- but maybe we should support these values too soon. All we need is a method to generate unprefixed version of these properties(maybe we already have and it is not in my knowledge) and hence I believe we should just keep them in the database. Do we prefer them removed from the database ? >> Source/WebInspectorUI/UserInterface/External/ContextualDocumentationDatabase/ContextualDocumentationDatabase.js:691 >> + "-webkit-background-composite": {}, > > Is there any value in keeping empty properties in the processed data, since we shouldn't show documentation in this case anyways? If not, let's remove it as well. > > (I came back here after finishing review the rest, and it does look like this will be an issue currently. I'd recommend removing empty properties entirely so you correctly don't show documentation for them.) I was unaware that such properties (without any details) existed. Thanks for pointing out. This has been fixed in the script and in the next patch. >> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.css:32 >> +.popover .documentation-popover > p { > > Is the `.popover` needed? Yes this was done to make this rule more specific than the " .popover p + p { margin-top :0.5 em } " rule > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:1 > +/* Done. >> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:39 >> + let bounds = WI.Rect.rectFromClientRect(this._delegate._nameElement.getBoundingClientRect()); > > If you want to access the nameElement, you need to make it public, as right now you are accessing a private member from a different class. Fixed in next patch. Creating bounds in SpreadsheetStyleProperty.js now and passing it as an argument to present. >> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:53 >> + _searchDocumentationDetails(property) > > NIT: `_getDocumentationDetails(property)`? yes that does sound better. >> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:80 >> + descriptionElement.textContent = details.description; > > You should either handle properties without a description, or remove such properties from the database file. I'd opt for the second option. removed from database Comment on attachment 432421 [details] Patch v1.3 - preprocessed database, refactored and moved code to right files,renamed files View in context: https://bugs.webkit.org/attachment.cgi?id=432421&action=review >>> Source/WebInspectorUI/UserInterface/External/ContextualDocumentationDatabase/ContextualDocumentationDatabase.js:2 >>> + "-moz-animation": { >> >> Do we need the `-moz-`, `-ms-`, and `-o-` prefixed properties since I don't believe we would ever show documentation for those, only the standard documentation? I think `-webkit-` is the only prefix you need. > > yes currently we only support -webkit- but maybe we should support these values too soon. All we need is a method to generate unprefixed version of these properties(maybe we already have and it is not in my knowledge) and hence I believe we should just keep them in the database. Do we prefer them removed from the database ? Wait my bad. Does anyone ever work with these prefix versions in web inspector? No , right ? Comment on attachment 432421 [details] Patch v1.3 - preprocessed database, refactored and moved code to right files,renamed files View in context: https://bugs.webkit.org/attachment.cgi?id=432421&action=review >>>> Source/WebInspectorUI/UserInterface/External/ContextualDocumentationDatabase/ContextualDocumentationDatabase.js:2 >>>> + "-moz-animation": { >>> >>> Do we need the `-moz-`, `-ms-`, and `-o-` prefixed properties since I don't believe we would ever show documentation for those, only the standard documentation? I think `-webkit-` is the only prefix you need. >> >> yes currently we only support -webkit- but maybe we should support these values too soon. All we need is a method to generate unprefixed version of these properties(maybe we already have and it is not in my knowledge) and hence I believe we should just keep them in the database. Do we prefer them removed from the database ? > > Wait my bad. Does anyone ever work with these prefix versions in web inspector? No , right ? We cross out other vendor prefixed properties anyways, as the property doesn't do anything in WebKit, so showing any documentation for it feels disingenuous, particularly documentation for the prefixed version of the property for a different vendor. We should filter out all the vendor prefixes (except `-webkit-*`) since they shouldn't be shown at any time since they don't document anything our engine supports. Comment on attachment 432421 [details] Patch v1.3 - preprocessed database, refactored and moved code to right files,renamed files View in context: https://bugs.webkit.org/attachment.cgi?id=432421&action=review >>>>> Source/WebInspectorUI/UserInterface/External/ContextualDocumentationDatabase/ContextualDocumentationDatabase.js:2 >>>>> + "-moz-animation": { >>>> >>>> Do we need the `-moz-`, `-ms-`, and `-o-` prefixed properties since I don't believe we would ever show documentation for those, only the standard documentation? I think `-webkit-` is the only prefix you need. >>> >>> yes currently we only support -webkit- but maybe we should support these values too soon. All we need is a method to generate unprefixed version of these properties(maybe we already have and it is not in my knowledge) and hence I believe we should just keep them in the database. Do we prefer them removed from the database ? >> >> Wait my bad. Does anyone ever work with these prefix versions in web inspector? No , right ? > > We cross out other vendor prefixed properties anyways, as the property doesn't do anything in WebKit, so showing any documentation for it feels disingenuous, particularly documentation for the prefixed version of the property for a different vendor. We should filter out all the vendor prefixes (except `-webkit-*`) since they shouldn't be shown at any time since they don't document anything our engine supports. Yeah Web Inspector only supports things prefixed with `-webkit-`, `-khtml-`, and `-apple-` as supported. We probably shouldn't have documentation for those properties. See `WI.CSSManager.prototype.canonicalNameForPropertyName` and nearby functions for specifics. >>> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.css:32 >>> +.popover .documentation-popover > p { >> >> Is the `.popover` needed? > > Yes this was done to make this rule more specific than the " .popover p + p { margin-top :0.5 em } " rule Gotcha. In that case, I think we should be consistent in this entire file and have everything be `.popover .documentation-popover-content`. Note that I added the `-content` as this matches what we do with other UI like this (e.g. `WI.CookiePopover`, `WI.BreakpointPopover`, etc.). > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:91 > +:matches(.spreadsheet-style-declaration-editor.properties, .computed-property-item) > .property > .content > .contextual-documentation-button { (I just realized that I never wrote the comment I had in mind here, which would make the `ditto (:91)` below make sense) We shouldn't have styles for completely unrelated UI (e.g. `.computed-property-item`) in this file. I realize that this would mean duplicating these styles, but seeing as how these styles are simple and the UI of `.spreadsheet-style-declaration-editor` and `.computed-property-item` is fairly different we should not merge them like this (unless `WI.SpreadsheetCSSStyleDeclarationEditor` and `WI.ComputedStyleSection` shared a common prototype, which they do not). Created attachment 432655 [details]
Patch v1.4 -Trimmed database of unwanted properties, console.assert() checks added, uniform icon sizing
Created attachment 432657 [details]
Update Script for ContextualDocumentationDatabase
The Script is written in Python. It provides for a quick way to download the most up to date data from VSCodeCustomData git Repository and prepare it for replacing the existing database.
The script also takes care :
1) Removing all unwanted details about a property and keeping only the name, description, sytax and URL
2) Alphabetizing the properties and details
3) Removing all unwanted (properties with no useful details) and undocumented(properties with prefixes that aren't supported by Web Inspector like -moz-, -ms- , -o- ) properties.
Running Instructions:
1) Go to the directory containing the script
2) Run terminal command - python updateContextualDocumentationDatabase.py
Comment on attachment 432655 [details] Patch v1.4 -Trimmed database of unwanted properties, console.assert() checks added, uniform icon sizing View in context: https://bugs.webkit.org/attachment.cgi?id=432655&action=review Looking good! Almost there =D I think after these we'll be in a good place to land :) > Source/WebInspectorUI/UserInterface/Base/Setting.js:215 > + showContextualSyntax: new WI.Setting("show-contextual-syntax", true), I thought we agreed to default disabled? also, I think this needs a more clear name like `showCSSPropertySyntaxInDocumentationPopover` > Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:21 > + #unavailable { i think you can remove this now > Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:29 > + #selected { This seems unused. Remove? > Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:33 > + #selected-active { ditto :(29) > Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:46 > + <svg id="selected"><use xlink:href="#info"/></svg> > + <svg id="selected-active"><use xlink:href="#info"/></svg> ditto :(29) > Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:47 > + <svg id="unavailable"><use xlink:href="#noinfo"/></svg> ditto (:21) > Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:53 > +.sidebar > .panel.details.css-style > .content > .computed .computed-style-variables .property .go-to-arrow { `.sidebar > .panel.details.css-style > .content > .computed > .details-section.computed-style-variables .property .go-to-arrow` > Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:59 > +.sidebar > .panel.details.css-style > .content > .computed .computed-property-item .property .go-to-arrow { The styles containing `.computed-property-item` should be moved to `ComputedStyleSection.css` as that's where all of the other `.computed-property-item` styles are. > Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:66 > +.sidebar > .panel.details.css-style > .content > .computed .computed-property-item:not(:hover) > .property .go-to-arrow, > .sidebar > .panel.details.css-style > .content > .computed .property:not(:hover) .go-to-arrow { This doesn't quite work the way I'd expect it. It causes the go-to arrow to appear anywhere when hovering anywhere on the line in the Variables section, but not the Properties section. I think you need something more like this ``` .sidebar > .panel.details.css-style > .content > .computed > .details-section.computed-style-variables .property:not(:hover) .go-to-arrow ``` > Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:70 > +.computed-property-item > .property > .content > .contextual-documentation-button { This should also have `.sidebar > .panel.details.css-style > .content > .computed` to match all of the above. > Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:76 > +.computed-property-item:not(:hover) > .property > .content > .contextual-documentation-button { ditto (:70) > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.css:26 > + .popover .documentation-popover-content { Style: unnecessary leading space > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:26 > +WI.ContextualDocumentationPopover = class ContextualDocumentationPopover I just realized that this doesn't actually inherit from `WI.Popover`. That's what all the other `WI.*Popover` classes do. ``` WI.ContextualDocumentationPopover = class ContextualDocumentationPopover extends WI.Popover { constructor(cssProperty, delegate) { console.assert(cssProperty instanceof WI.CSSProperty, cssProperty); super(delegate); this._cssProperty = cssProperty; this._targetElement = null; this.windowResizeHandler = this._presentOverTargetElement.bind(this); } // Public show(targetElement) { this._targetElement = targetElement; let documentationDetails = this._getDocumentationDetails(this._cssProperty); let documentationElement = this._createDocumentationElement(documentationDetails); this._popover.content = documentationElement; this._presentOverTargetElement(); } // Private _presentOverTargetElement() { if (!this._targetElement) return; let targetFrame = WI.Rect.rectFromClientRect(this._targetElement.getBoundingClientRect()); this.present(targetFrame.pad(2), [WI.RectEdge.MIN_X, WI.RectEdge.MIN_Y, WI.RectEdge.MAX_Y]); } ... ``` This structure will make it so that if the Web Inspector window is resized, the popover will remain in the right spot. It's also a more familiar pattern to those who've worked with other `WI.Popover` subclasses elsewhere in Web Inspector. (And it better matches the fact that this has the word "Popover" in its name) You'll need to adjust the callsites as well ``` _presentContextualDocumentation() { this._contextualDocumentationPopover ??= new WI.ContextualDocumentationPopover(this._property, this); this._contextualDocumentationPopover.show(this._nameElement); ... ``` > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:40 > + let documentationDetails = this._getDocumentationDetails(this._delegate.property); This is a really odd way of using a delegate. Normally, in order to keep code more readable (e.g. "who is calling this function") we prefix any delegate function with the name of the thing calling it (e.g. `contextualDocumentationPopoverProperty`). Delegates are really meant to be more of a "this object can be _anything_ so long as it has these specifically named functions" and not some specific kind of object. Given my comment on :26 tho, you'll likely change this anyways. This is more of a FYI :) > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:68 > + return details; NIT: you could inline this whole object ``` let propertyDocumentation = ContextualDocumentationDatabase[propertyName]; console.assert(propertyDocumentation, propertyName, ContextualDocumentationDatabase); return { name: propertyName, description: propertyDocumentation.description, syntax: propertyDocumentation.syntax, url: propertyDocumentation.url, }; ``` > Source/WebInspectorUI/UserInterface/Views/Main.css:350 > + vertical-align: bottom; This doesn't look good in the Styles panel. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:91 > +.spreadsheet-style-declaration-editor > .property:not(:hover) > .content .contextual-documentation-button, NIT: this could/should have a `>` between `.content` and `.contextual-documentation-button` > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:108 > + willDismissPopover() Style: this should be moved to a new `// Popover delegate` "section" (just like how there's one for `// SpreadsheetTextField delegate`) right above `// Private` > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:548 > + > + Style: unnecessary newlines Comment on attachment 432655 [details] Patch v1.4 -Trimmed database of unwanted properties, console.assert() checks added, uniform icon sizing View in context: https://bugs.webkit.org/attachment.cgi?id=432655&action=review > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:197 > + this._addContextualDocumentationButton(); Should this go down on line 226 so the button is appended between a possible `*/` and a possible go-to button, or is there a reason it comes before a potential `*/`? Created attachment 432738 [details]
Patch v1.5 - ContextualDocumentationPopover inherits WI.Popover, removed unwanted code from infoIcon.svg, moved css to more specific files
Comment on attachment 432738 [details] Patch v1.5 - ContextualDocumentationPopover inherits WI.Popover, removed unwanted code from infoIcon.svg, moved css to more specific files View in context: https://bugs.webkit.org/attachment.cgi?id=432738&action=review r=me, nice work! A few remaining Style/NIT comments, but nothing too serious :) > Source/WebInspectorUI/UserInterface/Base/Setting.js:215 > + showCSSPropertySyntaxInDocumentationPopover: new WI.Setting("show-css-property-syntax-in-documentation-popover", false), Style: this should go before anything else `showC*` to be alphabetical (capital < lowercase) > Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:28 > + <path id="noinfo" d="M5,10 C7.761,10 10,7.761 10,5 C10,2.239 7.761,0 5,0 C2.239,0 -1.77635684e-15,2.239 -1.77635684e-15,5 C-1.77635684e-15,7.761 2.239,10 5,10 Z M1.500,4.250 L8.500,4.250 L8.500,5.750 L1.500,5.750 L1.500,4.250 Z"/> this isn't used > Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:32 > + <svg id="normal"><use xlink:href="#info"/></svg> > + <svg id="active"><use xlink:href="#info"/></svg> Thinking about this a bit more, I think you can actually simplify this by just having `fill="currentColor"` ``` <?xml version="1.0" encoding="utf-8"?> <!-- Copyright © 2021 Apple Inc. All rights reserved. --> <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1" viewBox="0 0 10 10"> <path fill="currentColor" d="M5,10 C7.761,10 10,7.761 10,5 C10,2.239 7.761,0 5,0 C2.239,0 0,2.239 0,5 C0,7.761 2.239,10 5,10 Z M5.182,3.790 C5.375,3.790 5.528,3.855 5.632,3.986 C5.711,4.085 5.759,4.206 5.774,4.348 L5.780,4.458 L5.779,7.278 L6.374,7.278 C6.467,7.278 6.552,7.297 6.629,7.333 L6.703,7.376 L6.771,7.432 C6.879,7.535 6.934,7.666 6.934,7.819 C6.934,7.976 6.879,8.110 6.771,8.214 C6.684,8.295 6.581,8.344 6.464,8.361 L6.374,8.367 L3.952,8.367 C3.797,8.367 3.662,8.315 3.555,8.213 C3.447,8.110 3.392,7.976 3.392,7.819 C3.392,7.666 3.447,7.535 3.555,7.432 C3.640,7.350 3.744,7.301 3.861,7.284 L3.952,7.278 L4.571,7.278 L4.571,4.879 L4.053,4.879 C3.962,4.879 3.878,4.860 3.801,4.824 L3.728,4.781 L3.660,4.726 C3.549,4.623 3.492,4.489 3.492,4.332 C3.492,4.178 3.549,4.046 3.660,3.943 C3.747,3.862 3.849,3.813 3.964,3.796 L4.053,3.790 L5.182,3.790 Z M4.955,1.230 C5.205,1.230 5.419,1.318 5.590,1.493 C5.763,1.667 5.849,1.884 5.849,2.137 C5.849,2.382 5.762,2.596 5.591,2.771 C5.419,2.948 5.205,3.038 4.955,3.038 C4.709,3.038 4.497,2.948 4.322,2.771 C4.148,2.596 4.060,2.381 4.060,2.137 C4.060,1.884 4.148,1.668 4.322,1.492 C4.497,1.318 4.709,1.230 4.955,1.230 Z"/> </svg> ``` and then having the usage also have CSS `color: hsla(0, 0%, 0%, 0.5)`. Not sure if that would work with CSS `background-image` tho 🤔 > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.css:43 > + font-size: 10.2px; `10.2px`? Why not just `10px`? > Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:80 > + url: propertyDocumentation.url Style: missing ending `,` > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:528 > + if (ContextualDocumentationDatabase.hasOwnProperty(this._property.name) || ContextualDocumentationDatabase.hasOwnProperty(this._property.canonicalName)) { NIT: I'd make this into an early return Comment on attachment 432738 [details] Patch v1.5 - ContextualDocumentationPopover inherits WI.Popover, removed unwanted code from infoIcon.svg, moved css to more specific files View in context: https://bugs.webkit.org/attachment.cgi?id=432738&action=review >> Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:28 >> + <path id="noinfo" d="M5,10 C7.761,10 10,7.761 10,5 C10,2.239 7.761,0 5,0 C2.239,0 -1.77635684e-15,2.239 -1.77635684e-15,5 C-1.77635684e-15,7.761 2.239,10 5,10 Z M1.500,4.250 L8.500,4.250 L8.500,5.750 L1.500,5.750 L1.500,4.250 Z"/> > > this isn't used Removed this. >> Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:32 >> + <svg id="active"><use xlink:href="#info"/></svg> > > Thinking about this a bit more, I think you can actually simplify this by just having `fill="currentColor"` > ``` > <?xml version="1.0" encoding="utf-8"?> > <!-- Copyright © 2021 Apple Inc. All rights reserved. --> > <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1" viewBox="0 0 10 10"> > <path fill="currentColor" d="M5,10 C7.761,10 10,7.761 10,5 C10,2.239 7.761,0 5,0 C2.239,0 0,2.239 0,5 C0,7.761 2.239,10 5,10 Z M5.182,3.790 C5.375,3.790 5.528,3.855 5.632,3.986 C5.711,4.085 5.759,4.206 5.774,4.348 L5.780,4.458 L5.779,7.278 L6.374,7.278 C6.467,7.278 6.552,7.297 6.629,7.333 L6.703,7.376 L6.771,7.432 C6.879,7.535 6.934,7.666 6.934,7.819 C6.934,7.976 6.879,8.110 6.771,8.214 C6.684,8.295 6.581,8.344 6.464,8.361 L6.374,8.367 L3.952,8.367 C3.797,8.367 3.662,8.315 3.555,8.213 C3.447,8.110 3.392,7.976 3.392,7.819 C3.392,7.666 3.447,7.535 3.555,7.432 C3.640,7.350 3.744,7.301 3.861,7.284 L3.952,7.278 L4.571,7.278 L4.571,4.879 L4.053,4.879 C3.962,4.879 3.878,4.860 3.801,4.824 L3.728,4.781 L3.660,4.726 C3.549,4.623 3.492,4.489 3.492,4.332 C3.492,4.178 3.549,4.046 3.660,3.943 C3.747,3.862 3.849,3.813 3.964,3.796 L4.053,3.790 L5.182,3.790 Z M4.955,1.230 C5.205,1.230 5.419,1.318 5.590,1.493 C5.763,1.667 5.849,1.884 5.849,2.137 C5.849,2.382 5.762,2.596 5.591,2.771 C5.419,2.948 5.205,3.038 4.955,3.038 C4.709,3.038 4.497,2.948 4.322,2.771 C4.148,2.596 4.060,2.381 4.060,2.137 C4.060,1.884 4.148,1.668 4.322,1.492 C4.497,1.318 4.709,1.230 4.955,1.230 Z"/> > </svg> > ``` > and then having the usage also have CSS `color: hsla(0, 0%, 0%, 0.5)`. Not sure if that would work with CSS `background-image` tho 🤔 Keeping it in the current format to keep it similar to GoToArrow.svg >> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.css:43 >> + font-size: 10.2px; > > `10.2px`? Why not just `10px`? Since syntax uses a different font family than the description and MDN reference link we had to find the perfect number to make these look visually similar in the popover. 10 was a little small and 10.5 too big. Created attachment 432752 [details]
Patch v1.6 Finishing touches
Created attachment 432756 [details]
Patch v1.7 Finishing touches + a comment
Created attachment 432759 [details]
Patch v1.8 Finishing touches + a comment + super minor things
Created attachment 432760 [details]
Patch v1.9 ChangeLog Fixed
Committed r279510 (239362@main): <https://commits.webkit.org/239362@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 432760 [details]. |