[Webkit-unassigned] [Bug 226883] Web Inspector: add contextual documentation for CSS properties

[bugzilla-daemon at webkit.org]

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

--- Comment #20 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 432421
  --> https://bugs.webkit.org/attachment.cgi?id=432421
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).

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210630/e77742e2/attachment.htm>